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

[Avi Kivity <avi@redhat.com>]

On 01/08/2012 06:41 PM, Alexander Graf wrote:
> On 08.01.2012, at 17:26, Avi Kivity wrote:
>
> > On 01/08/2012 06:21 PM, Alexander Graf wrote:
> >>> 
> >>> 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.
> >> 
> >> Yes, but the end of the exit handler is basically the beginning of the entry loop. If we already know that we're in a heavy weight exit, we don't check for signals anymore:
> >> 
> >>        if (!(r & RESUME_HOST)) {
> >>                /* To avoid clobbering exit_reason, only check for signals if
> >>                 * we aren't already exiting to userspace for some other
> >>                 * reason. */
> >>                if (signal_pending(current)) {
> >>                        run->exit_reason = KVM_EXIT_INTR;
> >>                        r = (-EINTR << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
> >>                        kvmppc_account_exit(vcpu, SIGNAL_EXITS);
> >>                }
> >>        }
> >> 
> >> That way we do basically what you're suggesting, but are a bit more explicit, having checking code at the beginning and end and some conditional checking if it's required or not.
> >> 
> >> 
> > 
> > What I'm thinking of is unifying all of this.  But ppc driving the exit
> > handler from assembly doesn't allow this.
>
> Exactly :). The place you're thinking of having the code at is in asm code. And it has to be - the whole point of lightweight exits is that we don't have to restore all registers, which we would have to do if we returned from __kvmppc_vcpu_run. And that's basically what you'd need to keep this in the same code, right?

Right.  We could also have common code which is #ifdefed out for those
who don't like it.

>
> > The goal is not just to save code, but to make sure that the various
> > assumptions that generic code (like a future kvm_vcpu_kick()) makes are
> > satisfied implicitly, by having just one copy.
>
> Well, yes. I agree that I'd prefer to have it in one place but I can't think of a way to make this really technically possible by keeping the code optimized. Fortunately there are very few assumptions based on having the code structured in exactly the way you describe above though.
>
> The reason our kvm_vcpu_kick() implementation is slightly different from x86 is certainly not because we're doing the lightweight exits the way we do :).

I'm looking at it from a different angle.  If most of the code is common
the natural place to add a helper is in common code.  If it's not, then
you add it locally.  Eventually (=now) everyone has their own set of
helpers, and it's really hard to unify them because you can't keep track
of all of the assumptions that are embedded deeply within the code.

It' easier to add stuff that it is to delete stuff.

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