Re: [PATCH v2 4/9] KVM: arm/arm64: replace vcpu->arch.pause with a vcpu request

[Christoffer Dall <cdall@linaro.org>]

On Tue, Apr 04, 2017 at 06:24:36PM +0200, Paolo Bonzini wrote:
> 
> 
> On 04/04/2017 18:04, Christoffer Dall wrote:
> >> For pause, only the requester should do the clearing.
> 
> This suggests that maybe this should not be a request.  The request
> would be just the need to act on a GIC command, exactly as before this patch.

Maybe the semantics should be:

requester:                                vcpu:
----------                                -----
make_requet(vcpu, KVM_REQ_PAUSE);
                                          handles the request by
					  clearing it and setting
					  vcpu->pause = true;
wait until vcpu->pause == true
make_request(vcpu, KVM_REQ_UNPAUSE);
                                          vcpus 'wake up' clear the
					  UNPAUSE request and set
					  vcpu->pause = false;

The benefit would be that we get to re-use the complicated "figure out
the VCPU mode and whether or not we should send an IPI and get the
barriers right" stuff.

> 
> What I don't understand is:
> 
> >> With this patch, while the vcpu will still initially enter
> >> the guest, it will exit immediately due to the IPI sent by the vcpu
> >> kick issued after making the vcpu request.
> 
> Isn't this also true of KVM_REQ_VCPU_EXIT that was used before?
> 
> So this:
> 
> +			vcpu->arch.power_off || kvm_request_pending(vcpu)) {
> +			WRITE_ONCE(vcpu->mode, OUTSIDE_GUEST_MODE);
> 
> is the crux of the fix, you can keep using vcpu->arch.pause.

Probably; I feel like there's a fix here which should be a separate
patch from using a different requests instead of the KVM_REQ_VCPU_EXIT +
the pause flag.

> 
> By the way, vcpu->arch.power_off can go away from this "if" too because
> KVM_RUN and KVM_SET_MP_STATE are mutually exclusive through the vcpu mutex.

But we also allow setting the power_off flag from the in-kernel PSCI
emulation in the context of another VCPU thread.

> The earlier check is enough:
> 
>                  if (vcpu->arch.power_off || vcpu->arch.pause)
>                          vcpu_sleep(vcpu);
> 
> 
> >> +		/*
> >> +		 * Indicate we're in guest mode now, before doing a final
> >> +		 * check for pending vcpu requests. The general barrier
> >> +		 * pairs with the one in kvm_arch_vcpu_should_kick().
> >> +		 * Please see the comment there for more details.
> >> +		 */
> >> +		WRITE_ONCE(vcpu->mode, IN_GUEST_MODE);
> >> +		smp_mb();
> > 
> > There are two changes here:
> > 
> > there's a change from a normal write to a WRITE_ONCE and there's also a
> > change to that adds a memory barrier.  I feel like I'd like to know if
> > these are tied together or two separate cleanups.  I also wonder if we
> > could split out more general changes from the pause thing to have a
> > better log of why we changed the run loop?
> 
> You probably should just use smp_store_mb here.
> 

That looks cleaner at least.

Thanks,
-Christoffer