Re: [Qemu-devel] [PATCH] apic: fixup fallthrough to PIC

[Mark Asselstine]

On February 26, 2013 21:03:46 Jan Kiszka wrote:
> On 2013-02-26 20:48, Mark Asselstine wrote:
> > Commit 0e21e12bb311c4c1095d0269dc2ef81196ccb60a [Don't route PIC
> > interrupts through the local APIC if the local APIC config says so.]
> > missed a check to ensure the local APIC is enabled. Since if the local
> > APIC is disabled it doesn't matter what the local APIC config says.
> > 
> > If this check isn't done and the guest has disabled the local APIC the
> > guest will receive a general protection fault, similar to what is seen
> > here:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2012-12/msg02304.html
> > 
> > The GPF is caused by an attempt to service interrupt 0xffffffff. This
> > comes about since cpu_get_pic_interrupt() calls apic_accept_pic_intr()
> > (with the local APIC disabled apic_get_interrupt() returns -1).
> > apic_accept_pic_intr() returns 0 and thus the interrupt number which
> > is returned from cpu_get_pic_interrupt(), and which is attempted to be
> > serviced, is -1.
> > 
> > Signed-off-by: Mark Asselstine <address@hidden>
> > ---
> > The GPF only happens on occasion when you shutdown a linux guest
> > using 'poweroff -f' but I am able to get it to reproduce nearly
> > 100% of the time by attaching gdb to the Qemu backend, breaking
> > at 'lapic_shutdown' and stepping through the remainder of the
> > shutdown.
> > 
> > Mark
> > 
> >  hw/apic.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index fd14b73..051bd3e 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -591,6 +591,8 @@ int apic_accept_pic_intr(DeviceState *d)
> > 
> >      if (!s)
> >      
> >          return -1;
> > 
> > +    if (!(s->spurious_vec & APIC_SV_ENABLE))
> > +        return -1;
> > 
> >      lvt0 = s->lvt[APIC_LVT_LINT0];
> 
> The check is correct, but please adjust the coding style to QEMU standard:
> 
> if (x) {
>     ...
> }
> 

I was just mirroring the existing code in apic_get_interrupt() but agreed, the 
one liner reads better. I will send an V2 with the change shortly.

Thanks,
Mark

> And you can merge your check with the one above.
> 
> Thanks,
> Jan