Re: [PATCH v3] KVM: s390: Add new reset vcpu API

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2577 bytes --]

On 1/8/20 3:28 PM, Christian Borntraeger wrote:
> 
> 
> On 05.12.19 13:09, Janosch Frank wrote:
> [...]
>> +4.123 KVM_S390_CLEAR_RESET
>> +
>> +Capability: KVM_CAP_S390_VCPU_RESETS
>> +Architectures: s390
>> +Type: vcpu ioctl
>> +Parameters: none
>> +Returns: 0
>> +
>> +This ioctl resets VCPU registers and control structures that QEMU
>> +can't access via the kvm_run structure. The clear reset is a superset
>> +of the initial reset and additionally clears general, access, floating
>> +and vector registers.
> 
> As Thomas outlined, make it more obvious that userspace does the remaining
> parts. I do not think that we want the kernel to do the things (unless it
> helps you in some way for the ultravisor guests)

Ok, will do

> 
> [...]
>>  
>> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>> +{
>> +	kvm_clear_async_pf_completion_queue(vcpu);
>> +	kvm_s390_clear_local_irqs(vcpu);
>> +	return 0;
> 
> Shouldnt we also do 
>         if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
>                 kvm_s390_vcpu_stop(vcpu);
> 
> here?

Isn't userspace cpu state ctrl basically a prereq anyway for getting
those new ioctls?

> 
> 
>> +}
>> +
>>  static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
>>  {
>>  	kvm_s390_vcpu_initial_reset(vcpu);
>> @@ -4363,9 +4371,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>>  		r = kvm_arch_vcpu_ioctl_set_initial_psw(vcpu, psw);
>>  		break;
>>  	}
>> +
>> +	case KVM_S390_CLEAR_RESET:
>> +		/* fallthrough */
>>  	case KVM_S390_INITIAL_RESET:
>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>  		break;
> 
> Then we could also do a fallthrough here and do:
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d9e6bf3..c715ae3 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2867,10 +2867,6 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)
>         vcpu->arch.sie_block->pp = 0;
>         vcpu->arch.sie_block->fpf &= ~FPF_BPBC;
>         vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
> -       kvm_clear_async_pf_completion_queue(vcpu);
> -       if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm))
> -               kvm_s390_vcpu_stop(vcpu);
> -       kvm_s390_clear_local_irqs(vcpu);
>  }
>  
>  void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
> 
> 
> 
> 
>> +	case KVM_S390_NORMAL_RESET:
>> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
>> +		break;
>>  	case KVM_SET_ONE_REG:
>>  	case KVM_GET_ONE_REG: {



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]