Re: [PATCH v2 2/9] KVM: Add documentation for VCPU requests

[Christoffer Dall <cdall@linaro.org>]

On Wed, Apr 05, 2017 at 04:11:40PM +0200, Radim Krčmář wrote:
> 2017-04-04 19:23+0200, Christoffer Dall:
> > On Tue, Apr 04, 2017 at 07:06:00PM +0200, Andrew Jones wrote:
> >> On Tue, Apr 04, 2017 at 05:24:03PM +0200, Christoffer Dall wrote:
> >> > On Fri, Mar 31, 2017 at 06:06:51PM +0200, Andrew Jones wrote:
> >> > > +and will definitely see the request, or is outside guest mode, but has yet
> >> > > +to do its final request check, and therefore when it does, it will see the
> >> > > +request, then things will work.  However, the transition from outside to
> >> > > +inside guest mode, after the last request check has been made, opens a
> >> > > +window where a request could be made, but the VCPU would not see until it
> >> > > +exits guest mode some time later.  See the table below.
> >> > 
> >> > This text, and the table below, only deals with the details of entering
> >> > the guest.  Should we talk about kvm_vcpu_exiting_guest_mode() and
> >> > anything related to exiting the guest?
> >> 
> >> I think all !IN_GUEST_MODE should behave the same, so I was avoiding
> >> the use of EXITING_GUEST_MODE and OUTSIDE_GUEST_MODE, which wouldn't be
> >> hard to address, but then I'd also have to address
> >> READING_SHADOW_PAGE_TABLES, which may complicate the document more than
> >> necessary.  I'm not sure we need to address a VCPU exiting guest mode,
> >> other than making sure it's clear that a VCPU that exits must check
> >> requests before it enters again.
> > 
> > But the problem is that kvm_make_all_cpus_request() only sends IPIs to
> > CPUs where the mode was different from OUTSIDE_GUEST_MODE, so there it's
> > about !OUTSIDE_GUEST_MODE rather than !IN_GUEST_MODE, so there's some
> > subtlety here which I feel like it's dangerous to paper over.
> 
> Right, that needs fixing in the code.

Really?  I thought Paolo said that this is the intended behavior and
semantics; non-urgent requests that should just be serviced before the
next guest entry.

Now I'm confused again.  What did I miss?

> 
> guest_mode is just an optimization that allows us to skip sending the
> IPI when the VCPU is known to handle the request as soon as possible.
> 
>   IN_GUEST_MODE: we must force VM exit or the request could never be
>     handled
>   EXITING_GUEST_MODE: another request already forces the VM exit and
>     we're just waiting for the VCPU to notice our request
>   OUTSIDE_GUEST_MODE: KVM is going to notice our request without any
>     intervention
>   READING_SHADOW_PAGE_TABLES: same as OUTSIDE_GUEST_MODE -- rename to
>     unwieldly OUTSIDE_GUEST_MODE_READING_SHADOW_PAGE_TABLES?

Again, I thought Paolo was arguing that EXITING_GUEST_MODE makes the
whole thing work because you check that after checking requests?

> 
> The kick is needed only in IN_GUEST_MODE and a wake up is needed in case
> where the guest is halted OUTSIDE_GUEST_MODE ...

> Hm, maybe we should add a halt state too?

Wouldn't that be swait_active(&vcpu->wq) ?   You could add a wrapper
though.

What I think you need is a way to distinguish the semantics of calling
kvm_make_all_cpus_request(), perhaps by adding a 'bool wake_up'
parameter.

I also feel like it would be more reliable or easier to understand if
kvm_make_all_cpus_request() called kvm_vcpu_kick() somehow, but there
may be such an established and understood use of the differences between
the two by other architectures that it's worse to introduce the churn of
changing it.  I don't know.

Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm