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

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Apr 07, 2017 at 07:47:29PM +0800, Paolo Bonzini wrote:
> 
> 
> On 06/04/2017 22:14, Christoffer Dall wrote:
> >> So it seems like there are no races after all in KVM/ARM code
> >
> > No races after Drew's fix has been applied to set vcpu->mode =
> > IN_GUEST_MODE, before checking the pause flag, correct?  (I think that's
> > what the spin model below is modeling).
> 
> Yes.  All of them include the vcpu->mode = IN_GUEST_MODE assignment and
> (implicitly because spin is sequentially consistent) the memory barrier.
> 
> >> After this experiment, I think I like Drew's KVM_REQ_PAUSE more than I did
> >> yesterday.  However, yet another alternative is to leave pause/power_off as
> >> they are, while taking some inspiration from his patch to do some cleanups:
> >>
> >> 1) change the "if"
> >>
> >>                 if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
> >>                         vcpu->arch.power_off || vcpu->arch.pause) {
> >>
> >> to test kvm_requests_pending instead of pause/power_off
> >>
> >> 2) clear KVM_REQ_VCPU_EXIT before the other "if":
> >>
> >>                 if (vcpu->arch.power_off || vcpu->arch.pause)
> >>                         vcpu_sleep(vcpu);
> >
> > I like using requests as only requests from one thread to the VCPU
> > thread, and not to maintain specific state about a VCPU.
> > 
> > The benefit of Drew's approach is that since these pieces of state are
> > boolean, you can have just a single check in the critical path in the
> > run loop instead of having to access multiple fields.
> 
> I think you'd still need two checks for KVM_REQ_PAUSE/KVM_REQ_POWEROFF:
> one to check whether to sleep, and one to check whether to abort the
> vmentry.
> 
> Pause and power_off could be merged into a single bitmask if necessary, too.

True, a sort of flags.

I think at this point, I'll let Drew decide what looks cleanest when he
writes up the next revision and review those.

Thanks for the thorough help and checking!
-Christoffer