diff mbox

[1/2] seabios: Add Local APIC NMI Structure to ACPI MADT (was: Re: [PATCH] qemu: Fix inject-nmi)

Message ID 4E928B59.2060906@cn.fujitsu.com
State New
Headers show

Commit Message

Lai Jiangshan Oct. 10, 2011, 6:06 a.m. UTC 
From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

ACPI NMI Structure describes LINT pin (LINT0 or LINT1) information to
which NMI is connected, and it is needed by OS to initialize local APIC.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Reviewed-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 src/acpi.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Kenji Kaneshige Oct. 28, 2011, 12:08 p.m. UTC | #1 
Avi, Jan,

Could you comment on these patches?

Inject-NMI doesn't work on Windows guest without these patches. Windows seems
to setup LVT based on ACPI NMI structure information which is missing in current
seabios. LVT LINT1 are never unmasked by Windows guest without the patches.

Those patches were already reviewed by seabios people, but need ack from qemu/kvm
side.

Regards,
Kenji Kaneshige



(2011/10/10 15:06), Lai Jiangshan wrote:
> From: Kenji Kaneshige<kaneshige.kenji@jp.fujitsu.com>
> 
> ACPI NMI Structure describes LINT pin (LINT0 or LINT1) information to
> which NMI is connected, and it is needed by OS to initialize local APIC.
> 
> Signed-off-by: Kenji Kaneshige<kaneshige.kenji@jp.fujitsu.com>
> Reviewed-by: Lai Jiangshan<laijs@cn.fujitsu.com>
> ---
>   src/acpi.c |   22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> Index: seabios/src/acpi.c
> ===================================================================
> --- seabios.orig/src/acpi.c
> +++ seabios/src/acpi.c
> @@ -134,6 +134,14 @@ struct madt_intsrcovr {
>       u16 flags;
>   } PACKED;
> 
> +struct madt_local_nmi {
> +    ACPI_SUB_HEADER_DEF
> +    u8  processor_id;           /* ACPI processor id */
> +    u16 flags;                  /* MPS INTI flags */
> +    u8  lint;                   /* Local APIC LINT# */
> +} PACKED;
> +
> +
>   /*
>    * ACPI 2.0 Generic Address Space definition.
>    */
> @@ -288,7 +296,9 @@ build_madt(void)
>       int madt_size = (sizeof(struct multiple_apic_table)
>                        + sizeof(struct madt_processor_apic) * MaxCountCPUs
>                        + sizeof(struct madt_io_apic)
> -                     + sizeof(struct madt_intsrcovr) * 16);
> +                     + sizeof(struct madt_intsrcovr) * 16
> +                     + sizeof(struct madt_local_nmi));
> +
>       struct multiple_apic_table *madt = malloc_high(madt_size);
>       if (!madt) {
>           warn_noalloc();
> @@ -340,7 +350,15 @@ build_madt(void)
>           intsrcovr++;
>       }
> 
> -    build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> +    struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> +    local_nmi->type         = APIC_LOCAL_NMI;
> +    local_nmi->length       = sizeof(*local_nmi);
> +    local_nmi->processor_id = 0xff; /* all processors */
> +    local_nmi->flags        = 0;
> +    local_nmi->lint         = 1; /* LINT1 */
> +    local_nmi++;
> +
> +    build_header((void*)madt, APIC_SIGNATURE, (void*)local_nmi - (void*)madt, 1);
>       return madt;
>   }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>
Gleb Natapov Oct. 28, 2011, 12:19 p.m. UTC | #2 
On Fri, Oct 28, 2011 at 09:08:18PM +0900, Kenji Kaneshige wrote:
> Avi, Jan,
> 
> Could you comment on these patches?
> 
> Inject-NMI doesn't work on Windows guest without these patches. Windows seems
> to setup LVT based on ACPI NMI structure information which is missing in current
> seabios. LVT LINT1 are never unmasked by Windows guest without the patches.
> 
> Those patches were already reviewed by seabios people, but need ack from qemu/kvm
> side.
> 
> Regards,
> Kenji Kaneshige
> 
> 
> 
Acked-by: Gleb Natapov <gleb@redhat.com>

> (2011/10/10 15:06), Lai Jiangshan wrote:
> > From: Kenji Kaneshige<kaneshige.kenji@jp.fujitsu.com>
> > 
> > ACPI NMI Structure describes LINT pin (LINT0 or LINT1) information to
> > which NMI is connected, and it is needed by OS to initialize local APIC.
> > 
> > Signed-off-by: Kenji Kaneshige<kaneshige.kenji@jp.fujitsu.com>
> > Reviewed-by: Lai Jiangshan<laijs@cn.fujitsu.com>
> > ---
> >   src/acpi.c |   22 ++++++++++++++++++++--
> >   1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > Index: seabios/src/acpi.c
> > ===================================================================
> > --- seabios.orig/src/acpi.c
> > +++ seabios/src/acpi.c
> > @@ -134,6 +134,14 @@ struct madt_intsrcovr {
> >       u16 flags;
> >   } PACKED;
> > 
> > +struct madt_local_nmi {
> > +    ACPI_SUB_HEADER_DEF
> > +    u8  processor_id;           /* ACPI processor id */
> > +    u16 flags;                  /* MPS INTI flags */
> > +    u8  lint;                   /* Local APIC LINT# */
> > +} PACKED;
> > +
> > +
> >   /*
> >    * ACPI 2.0 Generic Address Space definition.
> >    */
> > @@ -288,7 +296,9 @@ build_madt(void)
> >       int madt_size = (sizeof(struct multiple_apic_table)
> >                        + sizeof(struct madt_processor_apic) * MaxCountCPUs
> >                        + sizeof(struct madt_io_apic)
> > -                     + sizeof(struct madt_intsrcovr) * 16);
> > +                     + sizeof(struct madt_intsrcovr) * 16
> > +                     + sizeof(struct madt_local_nmi));
> > +
> >       struct multiple_apic_table *madt = malloc_high(madt_size);
> >       if (!madt) {
> >           warn_noalloc();
> > @@ -340,7 +350,15 @@ build_madt(void)
> >           intsrcovr++;
> >       }
> > 
> > -    build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> > +    struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> > +    local_nmi->type         = APIC_LOCAL_NMI;
> > +    local_nmi->length       = sizeof(*local_nmi);
> > +    local_nmi->processor_id = 0xff; /* all processors */
> > +    local_nmi->flags        = 0;
> > +    local_nmi->lint         = 1; /* LINT1 */
> > +    local_nmi++;
> > +
> > +    build_header((void*)madt, APIC_SIGNATURE, (void*)local_nmi - (void*)madt, 1);
> >       return madt;
> >   }
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 

--
			Gleb.
Jun Koi Oct. 28, 2011, 12:48 p.m. UTC | #3 
2011/10/28 Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> Avi, Jan,
>
> Could you comment on these patches?
>
> Inject-NMI doesn't work on Windows guest without these patches.

sorry but i am really curious here: why Windows still works well even
if it desnt see the inject-NMI?
or there are still invisible side-effects that we are not awere of???

thanks,
Jun
Avi Kivity Oct. 30, 2011, 2:44 p.m. UTC | #4 
On 10/10/2011 08:06 AM, Lai Jiangshan wrote:
> From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
>
> ACPI NMI Structure describes LINT pin (LINT0 or LINT1) information to
> which NMI is connected, and it is needed by OS to initialize local APIC.
>
>

Kevin, can you please review these patches?
Avi Kivity Oct. 30, 2011, 2:44 p.m. UTC | #5 
On 10/30/2011 04:44 PM, Avi Kivity wrote:
> On 10/10/2011 08:06 AM, Lai Jiangshan wrote:
> > From: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> >
> > ACPI NMI Structure describes LINT pin (LINT0 or LINT1) information to
> > which NMI is connected, and it is needed by OS to initialize local APIC.
> >
> >
>
> Kevin, can you please review these patches?
>

Oh, I see the patch comment says you already did, in which case I'll ask
you to apply them.
Kenji Kaneshige Oct. 31, 2011, 8 a.m. UTC | #6 
(2011/10/28 21:48), Jun Koi wrote:
> 2011/10/28 Kenji Kaneshige<kaneshige.kenji@jp.fujitsu.com>:
>> Avi, Jan,
>>
>> Could you comment on these patches?
>>
>> Inject-NMI doesn't work on Windows guest without these patches.
>
> sorry but i am really curious here: why Windows still works well even
> if it desnt see the inject-NMI?
> or there are still invisible side-effects that we are not awere of???

Without this patch, LVT LINT1 is not configured by Windows guest because
seabios MADT has no ACPI NMI structure which is used by Windows to setup
LVT. So NMI interrupt would not be sent to CPUs when NMI signal happens
on LINT1. But qemu/kvm inject-nmi feature had a bug that it sent NMI to
CPUs without emulating LAPIC LVT. As a result, NMI interrupt is sent to
all the CPUs even though LVT LINT1 is not configured. This is why
inject-nmi behaves as if it works well on Windows guest.

Regards,
Kenji Kaneshige
diff mbox

Patch

Index: seabios/src/acpi.c
===================================================================
--- seabios.orig/src/acpi.c
+++ seabios/src/acpi.c
@@ -134,6 +134,14 @@  struct madt_intsrcovr {
     u16 flags;
 } PACKED;
 
+struct madt_local_nmi {
+    ACPI_SUB_HEADER_DEF
+    u8  processor_id;           /* ACPI processor id */
+    u16 flags;                  /* MPS INTI flags */
+    u8  lint;                   /* Local APIC LINT# */
+} PACKED;
+
+
 /*
  * ACPI 2.0 Generic Address Space definition.
  */
@@ -288,7 +296,9 @@  build_madt(void)
     int madt_size = (sizeof(struct multiple_apic_table)
                      + sizeof(struct madt_processor_apic) * MaxCountCPUs
                      + sizeof(struct madt_io_apic)
-                     + sizeof(struct madt_intsrcovr) * 16);
+                     + sizeof(struct madt_intsrcovr) * 16
+                     + sizeof(struct madt_local_nmi));
+
     struct multiple_apic_table *madt = malloc_high(madt_size);
     if (!madt) {
         warn_noalloc();
@@ -340,7 +350,15 @@  build_madt(void)
         intsrcovr++;
     }
 
-    build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
+    struct madt_local_nmi *local_nmi = (void*)intsrcovr;
+    local_nmi->type         = APIC_LOCAL_NMI;
+    local_nmi->length       = sizeof(*local_nmi);
+    local_nmi->processor_id = 0xff; /* all processors */
+    local_nmi->flags        = 0;
+    local_nmi->lint         = 1; /* LINT1 */
+    local_nmi++;
+
+    build_header((void*)madt, APIC_SIGNATURE, (void*)local_nmi - (void*)madt, 1);
     return madt;
 }