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

[Thomas Huth <thuth@redhat.com>]

On 10/01/2020 09.43, Janosch Frank wrote:
> On 1/10/20 8:14 AM, Janosch Frank wrote:
>> On 1/10/20 8:03 AM, Thomas Huth wrote:
>>> On 09/01/2020 18.51, Janosch Frank wrote:
>>>> On 1/9/20 6:08 PM, Cornelia Huck wrote:
>>>>> On Thu,  9 Jan 2020 10:56:01 -0500
>>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>>
>>>>>> The architecture states that we need to reset local IRQs for all CPU
>>>>>> resets. Because the old reset interface did not support the normal CPU
>>>>>> reset we never did that on a normal reset.
>>>>>>
>>>>>> Let's implement an interface for the missing normal and clear resets
>>>>>> and reset all local IRQs, registers and control structures as stated
>>>>>> in the architecture.
>>>>>>
>>>>>> Userspace might already reset the registers via the vcpu run struct,
>>>>>> but as we need the interface for the interrupt clearing part anyway,
>>>>>> we implement the resets fully and don't rely on userspace to reset the
>>>>>> rest.
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>>
>>>>>> I dropped the reviews, as I changed quite a lot.  
>>>>>>
>>>>>> Keep in mind, that now we'll need a new parameter in normal and
>>>>>> initial reset for protected virtualization to indicate that we need to
>>>>>> do the reset via the UV call. The Ultravisor does only accept the
>>>>>> needed reset, not any subset resets.
>>>>>
>>>>> In the interface, or externally?
>>>>
>>>> ?
>>>>
>>>>>
>>>>> [Apologies, but the details of the protected virt stuff are no longer
>>>>> in my cache.
>>>> Reworded explanation:
>>>> I can't use a fallthrough, because the UV will reject the normal reset
>>>> if we do an initial reset (same goes for the clear reset). To address
>>>> this issue, I added a boolean to the normal and initial reset functions
>>>> which tells the function if it was called directly or was called because
>>>> of the fallthrough.
>>>>
>>>> Only if called directly a UV call for the reset is done, that way we can
>>>> keep the fallthrough.
>>>
>>> Sounds complicated. And do we need the fallthrough stuff here at all?
>>> What about doing something like:
>>
>> That would work and I thought about it, it just comes down to taste :-)
>> I don't have any strong feelings for a specific implementation.
> 
> To be more specific:
> 
> 
> Commit c72db49c098bceb8b73c2e9d305caf37a41fb3bf
> Author: Janosch Frank <frankja@linux.ibm.com>
> Date:   Thu Jan 9 04:37:50 2020 -0500
> 
>     KVM: s390: protvirt: Add UV cpu reset calls
> 
>     For protected VMs, the VCPU resets are done by the Ultravisor, as KVM
>     has no access to the VCPU registers.
> 
>     As the Ultravisor will only accept a call for the reset that is
>     needed, we need to fence the UV calls when chaining resets.
> 
>     Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 63dc2bd97582..d5876527e464 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3476,8 +3476,11 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct
> kvm_vcpu *vcpu,
>  	return r;
>  }
> 
> -static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> +static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu, bool
> chain)
>  {
> +	int rc = 0;
> +	u32 ret;
> +
>  	vcpu->arch.sie_block->gpsw.mask = ~PSW_MASK_RI;
>  	vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID;
>  	memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb));
> @@ -3487,11 +3490,21 @@ static int
> kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
>  		kvm_s390_vcpu_stop(vcpu);
>  	kvm_s390_clear_local_irqs(vcpu);
> 
> -	return 0;
> +	if (kvm_s390_pv_handle_cpu(vcpu) && !chain) {
> +		rc = uv_cmd_nodata(kvm_s390_pv_handle_cpu(vcpu),
> +				   UVC_CMD_CPU_RESET, &ret);
> +		VCPU_EVENT(vcpu, 3, "PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x",
> +			   vcpu->vcpu_id, ret >> 16, ret & 0x0000ffff);
> +	}
> +
> +	return rc;
>  }
[...]
> @@ -4738,12 +4767,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> 
>  	case KVM_S390_CLEAR_RESET:
>  		r = kvm_arch_vcpu_ioctl_clear_reset(vcpu);
> +		if (r)
> +			break;
>  		/* fallthrough */
>  	case KVM_S390_INITIAL_RESET:
> -		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> +		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu, ioctl !=
> KVM_S390_INITIAL_RESET);
> +		if (r)
> +			break;
>  		/* fallthrough */
>  	case KVM_S390_NORMAL_RESET:
> -		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> +		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu, ioctl !=
> KVM_S390_NORMAL_RESET);
>  		break;
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
> 

As you said, it's mostly a matter of taste, but at least in my eyes this
approach with fallthroughs and the additional parameter looks rather
harder to understand compared to what I've suggested.

 Thomas