Re: [PATCH 13/50] KVM: PPC: booke: check for signals in kvmppc_vcpu_run

[Avi Kivity <avi@redhat.com>]

On 01/08/2012 05:37 PM, Alexander Graf wrote:
> On 08.01.2012, at 16:22, Avi Kivity wrote:
>
> > On 01/08/2012 05:11 PM, Alexander Graf wrote:
> >> On 08.01.2012, at 14:18, Avi Kivity wrote:
> >> 
> >>> On 01/04/2012 03:10 AM, Alexander Graf wrote:
> >>>> From: Scott Wood <scottwood@freescale.com>
> >>>> 
> >>>> Currently we check prior to returning from a lightweight exit,
> >>>> but not prior to initial entry.
> >>>> 
> >>>> book3s already does a similar test.
> >>>> 
> >>>> Signed-off-by: Scott Wood <scottwood@freescale.com>
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>> arch/powerpc/kvm/booke.c |   10 +++++++++-
> >>>> 1 files changed, 9 insertions(+), 1 deletions(-)
> >>>> 
> >>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> >>>> index b642200..9c78589 100644
> >>>> --- a/arch/powerpc/kvm/booke.c
> >>>> +++ b/arch/powerpc/kvm/booke.c
> >>>> @@ -322,11 +322,19 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
> >>>> 	}
> >>>> 
> >>>> 	local_irq_disable();
> >>>> +
> >>>> +	if (signal_pending(current)) {
> >>>> +		kvm_run->exit_reason = KVM_EXIT_INTR;
> >>>> +		ret = -EINTR;
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> 	kvm_guest_enter();
> >>>> 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> >>>> 	kvm_guest_exit();
> >>>> -	local_irq_enable();
> >>>> 
> >>>> 
> >>> 
> >>> In general a single check prior to entry is sufficient (well, in
> >>> addition to the one in kvm_vcpu_block()).
> >> 
> >> Yes, and IIUC this is the single check prior to entry. On lightweight exit, we don't return from __kvmppc_vcpu_run, but only call kvmppc_handle_exit() and if that exits that we return to the guest, we stay inside of __kvmppc_vcpu_run and don't return from here.
> > 
> > It means you check twice per heavyweight exit, no?  Once here, and once
> > when kvmppc_handle_exit() returns.  If, instead, you move the check to
> > just before the lightweight entry, you check just once per entry, like x86.
>
> You mean we check twice in case a heavyweight exit occurs right after another heavyweight exit? We need to check whether a signal is pending to determine heavyweight exits, so we definitely have to check at the end of the exit handlers:
>
>   - check signal (none pending)
>   - enter guest
>   - exit guest because an external interrupt occurs (lightweight)
>   - check signal (pending now because we got preempted)
>   -> make exit heavyweight
>
> If however there already is a signal pending before we enter the guest and the guest is running an endless loop and the host doesn't have any timers configured because it's just waiting for the signal to be handled, we would be in a dead loop. So we have to check before entry either way.

Yes, but you're checking in the wrong place.

What you have now is:

   if (!signal_pending())
         do
              enter guest
         while (!signal_pending && is_lightweight_exit)

What x86 does is

   while (!signal_pending() && can_reenter)
           can_reenter = enter guest

well, that's not actually what x86 does, but what it should be doing. 
The point is checking for signals after an exit is meaningless.  You've
exited, so the guest can't be holding off a signal for the host.  The
check after the exit is really meant for the next entry, but it doesn't
cover the first entry, so you have another check before that.

-- 
error compiling committee.c: too many arguments to function