Re: [PATCH 2/2] KVM: PPC: Book3S: Call into C interrupt handlers

[Paul Mackerras <paulus@xxxxxxxxx>]

On Thu, Apr 26, 2012 at 12:19:03PM +0200, Alexander Graf wrote:

> So switch the code over to call into the Linux C handlers from C code,
> speeding up everything along the way.

I have to say this patch makes me pretty uneasy.  There are a few
things that look wrong to me, but more than that, it seems to me that
there would be a lot of careful thought needed to make sure that the
approach is bullet-proof.

The first thing is that you are filling in the registers, and in
particular r1, in a subroutine, so you are potentially making regs.r1
point to a stack frame that no longer exists by the time we look at it
inside do_IRQ or timer_interrupt.  So, for example, a stack trace
could go completely off the rails at that point.  Quite possibly gcc
will inline the kvmppc_fill_pt_regs function, which would probably
save you, but I don't think you should rely on that.

The second thing is, why do you save just r1, ip, msr, and lr?  Why
those ones and no others?  A performance monitor interrupt might well
decide to save all the registers away plus a stack trace, and to see
all the GPRs as 0 could be very confusing.

Thirdly, if preemption is enabled, it could well be that the interrupt
will wake up a higher priority task which should run before we
continue.  On 64-bit you are probably saved by the soft_irq_enable
calls, which will (I think) call schedule() if a reschedule is
pending, but on 32-bit soft_irq_enable does nothing.

Fourthly, as Ben said, you should be setting regs->trap.

Have you measured a performance improvement with this patch?  If so
how big was it?

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html