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
