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

[Jan Kiszka]

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) {
    ...
}

And you can merge your check with the one above.

Thanks,
Jan

signature.asc
Description: OpenPGP digital signature