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
