apic: fixup fallthrough to PIC

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 <mark.asselstine@windriver.com>
---
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(+)

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 <mark.asselstine@windriver.com>
> ---
> 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) {
    ...
}

And you can merge your check with the one above.

Thanks,
Jan

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 <mark.asselstine@windriver.com>
> > ---
> > 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