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

[Cornelia Huck <cohuck@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

On Fri, 10 Jan 2020 09:43:33 +0100
Janosch Frank <frankja@linux.ibm.com> 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:  

> >>> 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.  

(...)

> +	if (kvm_s390_pv_handle_cpu(vcpu) && !chain) {

I find this 'chain' thingy a bit unwieldy...

> +		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;
>  }

(...)

> 
>  	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);

...especially looking at the invocations.

>  		break;
>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
> 

<bikeshed>
What about the following?

static void _do_normal_reset(struct kvm_vcpu *vcpu)
{
	/* do normal reset */
}

static int kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu)
{
	_do_normal_reset(vcpu);
        if (kvm_s390_pv_handle_cpu(vcpu)) {
		/* do protected virt normal reset */
	}
}

static void _do_initial_reset(struct kvm_vcpu *vcpu)
{
	/* do initial reset */
}

static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
{
	_do_initial_reset(vcpu);
	if (kvm_set_pv_handle_cpu(vcpu)) {
		/* do protected virt initial reset */
	}
	_do_normal_reset(vcpu);
}

static void _do_clear_reset(struct kvm_vcpu *vcpu)
{
	/* do clear reset */
}

static int kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu)
{
	_do_clear_reset(vcpu);
	if (kvm_set_pv_handle_cpu(vcpu)) {
		/* do protected virt clear reset */
	}
	_do_initial_reset(vcpu);
	_do_normal_reset(vcpu);
}

And call the *_ioctl_* functions directly without fallthrough.

The nice thing about this is that it makes the call chain explicit and
does not require parameters.

The drawback is that we need more functions, and that it looks a bit
overcomplicated before the pv stuff is added.

</bikeshed>

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