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

[Christoffer Dall <cdall@linaro.org>]

On Wed, Apr 05, 2017 at 10:46:07PM +0200, Radim Krčmář wrote:
> 2017-04-05 20:29+0200, Paolo Bonzini:
> > On 05/04/2017 19:45, Christoffer Dall wrote:
> >>>> 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.
> > 
> > Indeed, that's right...
> > 
> >>> 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?
> > 
> > ... but apparently I was wrong here, see my email from this morning.
> 
> Ok, I'll prepare a patch that uses kvm_arch_vcpu_should_kick() in
> kvm_make_all_cpus_request().
> 
> >>> 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.
> > 
> > Yes.  I think the wrapper is probably unnecessary.
> 
> The state would only make sense if it were different -- there is a time
> when the VCPU is halted, but swait_active(&vcpu->wq) returns false.
> Probably doesn't have any good use now, 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.
> > 
> > That would be fine.
> 
> I think the wakeup is more tied to the request type than to the place
> calling it, 

agreed

> so we could say which requests are "lazy" and don't do
> wakeup and have kvm_make_all_cpus_request() without an extra argument.

Sounds good to me.

We could encode the lazy/non-lazy thing in a bit in the request number
that gets masked off before clearing/checking the request bit, but
perhaps there are nicer solutions.

> 
> In the end, I would like if kvm_make_all_cpus_request() was an optimized
> variant of
> 
>   kvm_for_each_vcpu(i, vcpu, kvm) {
>     kvm_make_request(vcpu, request);
>     kvm_vcpu_kick(vcpu);
>   }

Yes, I would like this too.

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