[PATCH] acpi: add Local APIC NMI Structure to MADT.

[Kenji Kaneshige]

Add Local APIC NMI Structure to ACPI MADT.

Signed-off-by: Kenji Kaneshige 
---
 src/acpi.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 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) * MaxCountCPUs);
+
     struct multiple_apic_table *madt = malloc_high(madt_size);
     if (!madt) {
         warn_noalloc();
@@ -340,7 +350,17 @@ build_madt(void)
         intsrcovr++;
     }

-    build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
+    struct madt_local_nmi *local_nmi = (void*)intsrcovr;
+    for (i = 0; i < MaxCountCPUs; i++) {
+        local_nmi->type         = APIC_LOCAL_NMI;
+        local_nmi->length       = sizeof(*local_nmi);
+        local_nmi->processor_id = i;
+        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;
 }

[Kevin O'Connor]

On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> Add Local APIC NMI Structure to ACPI MADT.

Thanks.  Can you give a brief description of what this is needed by?

Also, is this for kvm, qemu, or some other emulator?

-Kevin

[Kenji Kaneshige]

(2011/09/23 13:31), Kevin O'Connor wrote:
> On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > Add Local APIC NMI Structure to ACPI MADT.
>
> Thanks.  Can you give a brief description of what this is needed by?
>
> Also, is this for kvm, qemu, or some other emulator?

This describes LINT pin (LINT0 or LINT1) to which NMI is connected,
and this information is needed by OS to initialize local APIC.
Current seabios provides this information through MP configuration
table but doesn't provide it through ACPI MADT. I found this when
I reviewed "inject NMI" feature in qemu/kvm.

By the way, I think usually NMI is connected to all processors'
LINT1 as MultiProcessor spec indicates, and my patch assumes this.
But, I noticed that seabios's MP configuration table provides only
a NMI information connected to BSP. Is there any special reason
about this?

Regards,
Kenji Kaneshige

[Kevin O'Connor]

On Mon, Sep 26, 2011 at 07:58:56PM +0900, Kenji Kaneshige wrote:
> (2011/09/23 13:31), Kevin O'Connor wrote:
> > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > Add Local APIC NMI Structure to ACPI MADT.
> >
> > Thanks.  Can you give a brief description of what this is needed by?
> >
> > Also, is this for kvm, qemu, or some other emulator?
>
> This describes LINT pin (LINT0 or LINT1) to which NMI is connected,
> and this information is needed by OS to initialize local APIC.
> Current seabios provides this information through MP configuration
> table but doesn't provide it through ACPI MADT. I found this when
> I reviewed "inject NMI" feature in qemu/kvm.
>
> By the way, I think usually NMI is connected to all processors'
> LINT1 as MultiProcessor spec indicates, and my patch assumes this.
> But, I noticed that seabios's MP configuration table provides only
> a NMI information connected to BSP. Is there any special reason
> about this?

I'm afraid I'm not very knowledgable on the intricacies of LINTx
setup.

Your patch looks okay to me, but I would like to see an Ack from one
of the QEmu or KVM maintainers as well.

-Kevin

[Kenji Kaneshige]

(2011/09/30 8:43), Kevin O'Connor wrote:
> On Mon, Sep 26, 2011 at 07:58:56PM +0900, Kenji Kaneshige wrote:
> > (2011/09/23 13:31), Kevin O'Connor wrote:
> > > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > > Add Local APIC NMI Structure to ACPI MADT.
> > >
> > > Thanks.  Can you give a brief description of what this is needed by?
> > >
> > > Also, is this for kvm, qemu, or some other emulator?
> >
> > This describes LINT pin (LINT0 or LINT1) to which NMI is connected,
> > and this information is needed by OS to initialize local APIC.
> > Current seabios provides this information through MP configuration
> > table but doesn't provide it through ACPI MADT. I found this when
> > I reviewed "inject NMI" feature in qemu/kvm.
> >
> > By the way, I think usually NMI is connected to all processors'
> > LINT1 as MultiProcessor spec indicates, and my patch assumes this.
> > But, I noticed that seabios's MP configuration table provides only
> > a NMI information connected to BSP. Is there any special reason
> > about this?
>
> I'm afraid I'm not very knowledgable on the intricacies of LINTx
> setup.
>
> Your patch looks okay to me, but I would like to see an Ack from one
> of the QEmu or KVM maintainers as well.

Ok, thank you. I will try.

Regards,
Kenji Kaneshige

[Gleb Natapov]

On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> Add Local APIC NMI Structure to ACPI MADT.
> Signed-off-by: Kenji Kaneshige
>
> src/acpi.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 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) * MaxCountCPUs);
> struct multiple_apic_table *madt = malloc_high(madt_size);
>    if (!madt) {
>        warn_noalloc();
>
> @@ -340,7 +350,17 @@ build_madt(void)
>          intsrcovr++;
>      }
> build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> for (i = 0; i < MaxCountCPUs; i++) {
>    local_nmi->type         = APIC_LOCAL_NMI;
>    local_nmi->length       = sizeof(*local_nmi);
>    local_nmi->processor_id = i;

Spec says that value 0xFF signifies that this applies to all processors
in the machine, so you need to create only one APIC_LOCAL_NMI entry with
0xFF as a processor id.
>    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;
>
> }
>
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios

[Gleb Natapov]

On Mon, Sep 26, 2011 at 07:58:56PM +0900, Kenji Kaneshige wrote:
> (2011/09/23 13:31), Kevin O'Connor wrote:
> > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > Add Local APIC NMI Structure to ACPI MADT.
> >
> > Thanks.  Can you give a brief description of what this is needed by?
> >
> > Also, is this for kvm, qemu, or some other emulator?
>
> This describes LINT pin (LINT0 or LINT1) to which NMI is connected,
> and this information is needed by OS to initialize local APIC.
> Current seabios provides this information through MP configuration
> table but doesn't provide it through ACPI MADT. I found this when
> I reviewed "inject NMI" feature in qemu/kvm.
>
> By the way, I think usually NMI is connected to all processors'
> LINT1 as MultiProcessor spec indicates, and my patch assumes this.
> But, I noticed that seabios's MP configuration table provides only
> a NMI information connected to BSP. Is there any special reason
> about this?

Should be changed to 0xff too.

[Kenji Kaneshige]

(2011/10/02 17:31), Gleb Natapov wrote:
> On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > Add Local APIC NMI Structure to ACPI MADT.
> > Signed-off-by: Kenji Kaneshige
> >
> > src/acpi.c |   24 ++++++++++++++++++++++--
> >   1 file changed, 22 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) * MaxCountCPUs);
> > struct multiple_apic_table *madt = malloc_high(madt_size);
> > if (!madt) {
> >     warn_noalloc();
> >
> > @@ -340,7 +350,17 @@ build_madt(void)
> >           intsrcovr++;
> >       }
> > build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> > struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> > for (i = 0; i<  MaxCountCPUs; i++) {
> >    local_nmi->type         = APIC_LOCAL_NMI;
> >    local_nmi->length       = sizeof(*local_nmi);
> >    local_nmi->processor_id = i;
>
> Spec says that value 0xFF signifies that this applies to all processors
> in the machine, so you need to create only one APIC_LOCAL_NMI entry with
> 0xFF as a processor id.

Thank you for your comment. Actually I had the same idea. But according
to the revision number in FADT, seabios uses ACPI1.0 spec which doesn't
support the value '0xFF'. Could you double check?

Regards,
Kenji Kaneshige

[Kenji Kaneshige]

(2011/10/02 17:50), Gleb Natapov wrote:
> On Mon, Sep 26, 2011 at 07:58:56PM +0900, Kenji Kaneshige wrote:
> > (2011/09/23 13:31), Kevin O'Connor wrote:
> > > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > > Add Local APIC NMI Structure to ACPI MADT.
> > >
> > > Thanks.  Can you give a brief description of what this is needed by?
> > >
> > > Also, is this for kvm, qemu, or some other emulator?
> >
> > This describes LINT pin (LINT0 or LINT1) to which NMI is connected,
> > and this information is needed by OS to initialize local APIC.
> > Current seabios provides this information through MP configuration
> > table but doesn't provide it through ACPI MADT. I found this when
> > I reviewed "inject NMI" feature in qemu/kvm.
> >
> > By the way, I think usually NMI is connected to all processors'
> > LINT1 as MultiProcessor spec indicates, and my patch assumes this.
> > But, I noticed that seabios's MP configuration table provides only
> > a NMI information connected to BSP. Is there any special reason
> > about this?
>
> Should be changed to 0xff too.

Thank you. Will send the patch soon.

Regards,
Kenji Kaneshige

[Gleb Natapov]

On Mon, Oct 03, 2011 at 10:47:11AM +0900, Kenji Kaneshige wrote:
> (2011/10/02 17:31), Gleb Natapov wrote:
> > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > Add Local APIC NMI Structure to ACPI MADT.
> > > Signed-off-by: Kenji Kaneshige
> > >
> > > src/acpi.c |   24 ++++++++++++++++++++++--
> > >  1 file changed, 22 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) * MaxCountCPUs);
> > > struct multiple_apic_table *madt = malloc_high(madt_size);
> > >    if (!madt) {
> > >        warn_noalloc();
> > >
> > > @@ -340,7 +350,17 @@ build_madt(void)
> > >          intsrcovr++;
> > >      }
> > > build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> > > struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> > > for (i = 0; i<  MaxCountCPUs; i++) {
> > >    local_nmi->type         = APIC_LOCAL_NMI;
> > >    local_nmi->length       = sizeof(*local_nmi);
> > >    local_nmi->processor_id = i;
> >
> > Spec says that value 0xFF signifies that this applies to all processors
> > in the machine, so you need to create only one APIC_LOCAL_NMI entry with
> > 0xFF as a processor id.
>
> Thank you for your comment. Actually I had the same idea. But according
> to the revision number in FADT, seabios uses ACPI1.0 spec which doesn't
> support the value '0xFF'. Could you double check?

Seabios ACPI contains bits from various versions. And since 0xFF as a
wildcard destination is mention in mptable spec, which predates ACPI1.0
by a couple of years, it is safe to assume that even for ACPI1.0 it is
correct value to use.

[Kenji Kaneshige]

(2011/10/03 16:17), Gleb Natapov wrote:
> On Mon, Oct 03, 2011 at 10:47:11AM +0900, Kenji Kaneshige wrote:
> > (2011/10/02 17:31), Gleb Natapov wrote:
> > > On Thu, Sep 22, 2011 at 09:58:58PM +0900, Kenji Kaneshige wrote:
> > > > Add Local APIC NMI Structure to ACPI MADT.
> > > > Signed-off-by: Kenji Kaneshige
> > > >
> > > > src/acpi.c |   24 ++++++++++++++++++++++--
> > > >   1 file changed, 22 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) * MaxCountCPUs);
> > > > struct multiple_apic_table *madt = malloc_high(madt_size);
> > > > if (!madt) {
> > > >     warn_noalloc();
> > > >
> > > > @@ -340,7 +350,17 @@ build_madt(void)
> > > >           intsrcovr++;
> > > >       }
> > > > build_header((void*)madt, APIC_SIGNATURE, (void*)intsrcovr - (void*)madt, 1);
> > > > struct madt_local_nmi *local_nmi = (void*)intsrcovr;
> > > > for (i = 0; i<   MaxCountCPUs; i++) {
> > > >    local_nmi->type         = APIC_LOCAL_NMI;
> > > >    local_nmi->length       = sizeof(*local_nmi);
> > > >    local_nmi->processor_id = i;
> > >
> > > Spec says that value 0xFF signifies that this applies to all processors
> > > in the machine, so you need to create only one APIC_LOCAL_NMI entry with
> > > 0xFF as a processor id.
> >
> > Thank you for your comment. Actually I had the same idea. But according
> > to the revision number in FADT, seabios uses ACPI1.0 spec which doesn't
> > support the value '0xFF'. Could you double check?
>
> Seabios ACPI contains bits from various versions. And since 0xFF as a
> wildcard destination is mention in mptable spec, which predates ACPI1.0
> by a couple of years, it is safe to assume that even for ACPI1.0 it is
> correct value to use.

Ok, thank you. I'll update the patch.

Regards,
Kenji Kaneshige