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

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

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

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.

> 
> static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
> {
> 	...
> }
> 
> static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
> {
> 	kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> 	...
> }
> 
> static int kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
> {
> 	kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> 	...
> }
> 
> ...
> 
> 	case KVM_S390_CLEAR_RESET:
> 		r = kvm_arch_vcpu_ioctl_clear_reset(vcpu);
> 		if (!r && protected) {
> 			r = uv_cmd_nodata(...,
>  				UVC_CMD_CPU_RESET_CLEAR, ...);
> 		}
> 		break;
>  	case KVM_S390_INITIAL_RESET:
>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
> 		if (!r && protected) {
> 			r = uv_cmd_nodata(...,
>  				UVC_CMD_CPU_RESET_INITIAL, ...);
> 		}
> 	case KVM_S390_NORMAL_RESET:
> 		r = kvm_arch_vcpu_ioctl_normal_reset(vcpu);
> 		if (!r && protected) {
> 			r = uv_cmd_nodata(...,
>  				UVC_CMD_CPU_RESET, ...);
> 		}
>  		break;
> 
> ... or does that not work due to some other constraints that I've missed?
> 
>  Thomas
> 



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