Re: [PATCH v3 24/35] acpi: x86: madt: use build_append_int_noprefix() API to compose MADT table

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 22 Sep 2021 12:20:54 +0200
Eric Auger <eauger@redhat.com> wrote:

> On 9/7/21 4:48 PM, Igor Mammedov wrote:
> > Drop usage of packed structures and explicit endian conversions
> > when building MADT table for arm/x86 and use endian agnostic
> > build_append_int_noprefix() API to build it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: marcel.apfelbaum@gmail.com
> > CC: eauger@redhat.com
> > ---
> >  include/hw/acpi/acpi-defs.h |  64 ------------------
> >  hw/i386/acpi-common.c       | 127 ++++++++++++++++++------------------
> >  2 files changed, 65 insertions(+), 126 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index af4fa412a5..3f174ba208 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -165,17 +165,6 @@ typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
> >  
> >  /* Values for Type in APIC sub-headers */
> >  
> > -#define ACPI_APIC_PROCESSOR          0
> > -#define ACPI_APIC_IO                 1
> > -#define ACPI_APIC_XRUPT_OVERRIDE     2
> > -#define ACPI_APIC_NMI                3
> > -#define ACPI_APIC_LOCAL_NMI          4
> > -#define ACPI_APIC_ADDRESS_OVERRIDE   5
> > -#define ACPI_APIC_IO_SAPIC           6
> > -#define ACPI_APIC_LOCAL_SAPIC        7
> > -#define ACPI_APIC_XRUPT_SOURCE       8
> > -#define ACPI_APIC_LOCAL_X2APIC       9
> > -#define ACPI_APIC_LOCAL_X2APIC_NMI      10
> >  #define ACPI_APIC_GENERIC_CPU_INTERFACE 11
> >  #define ACPI_APIC_GENERIC_DISTRIBUTOR   12
> >  #define ACPI_APIC_GENERIC_MSI_FRAME     13
> > @@ -192,59 +181,6 @@ typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
> >  
> >  /* Sub-structures for MADT */
> >  
> > -struct AcpiMadtProcessorApic {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint8_t  processor_id;           /* ACPI processor id */
> > -    uint8_t  local_apic_id;          /* Processor's local APIC id */
> > -    uint32_t flags;
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtProcessorApic AcpiMadtProcessorApic;
> > -
> > -struct AcpiMadtIoApic {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint8_t  io_apic_id;             /* I/O APIC ID */
> > -    uint8_t  reserved;               /* Reserved - must be zero */
> > -    uint32_t address;                /* APIC physical address */
> > -    uint32_t interrupt;              /* Global system interrupt where INTI
> > -                                 * lines start */
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtIoApic AcpiMadtIoApic;
> > -
> > -struct AcpiMadtIntsrcovr {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint8_t  bus;
> > -    uint8_t  source;
> > -    uint32_t gsi;
> > -    uint16_t flags;
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtIntsrcovr AcpiMadtIntsrcovr;
> > -
> > -struct AcpiMadtLocalNmi {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint8_t  processor_id;           /* ACPI processor id */
> > -    uint16_t flags;                  /* MPS INTI flags */
> > -    uint8_t  lint;                   /* Local APIC LINT# */
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
> > -
> > -struct AcpiMadtProcessorX2Apic {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint16_t reserved;
> > -    uint32_t x2apic_id;              /* Processor's local x2APIC ID */
> > -    uint32_t flags;
> > -    uint32_t uid;                    /* Processor object _UID */
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtProcessorX2Apic AcpiMadtProcessorX2Apic;
> > -
> > -struct AcpiMadtLocalX2ApicNmi {
> > -    ACPI_SUB_HEADER_DEF
> > -    uint16_t flags;                  /* MPS INTI flags */
> > -    uint32_t uid;                    /* Processor object _UID */
> > -    uint8_t  lint;                   /* Local APIC LINT# */
> > -    uint8_t  reserved[3];            /* Local APIC LINT# */
> > -} QEMU_PACKED;
> > -typedef struct AcpiMadtLocalX2ApicNmi AcpiMadtLocalX2ApicNmi;
> > -
> >  struct AcpiMadtGenericCpuInterface {
> >      ACPI_SUB_HEADER_DEF
> >      uint16_t reserved;
> > diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> > index 7983a13a93..aa7b5c357e 100644
> > --- a/hw/i386/acpi-common.c
> > +++ b/hw/i386/acpi-common.c
> > @@ -38,7 +38,9 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> >                         bool force_enabled)
> >  {
> >      uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> > -    uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ? 1 : 0;
> > +    /* Flags – Local APIC Flags */  
> nit: move that comment at the place of the build_append_int_noprefix

that place(s) already has comment in expected format, here is just reminder
about what is initialized here.

> > +    uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
> > +                     1 /* Enabled */ : 0;
> >  
> >      /* ACPI spec says that LAPIC entry for non present
> >       * CPU may be omitted from MADT or it must be marked
> > @@ -47,24 +49,47 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> >       * should be put in MADT but kept disabled.
> >       */
> >      if (apic_id < 255) {
> > -        AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
> > -
> > -        apic->type = ACPI_APIC_PROCESSOR;
> > -        apic->length = sizeof(*apic);
> > -        apic->processor_id = uid;
> > -        apic->local_apic_id = apic_id;
> > -        apic->flags = cpu_to_le32(flags);
> > +        /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
> > +        build_append_int_noprefix(entry, 0, 1);       /* Type */
> > +        build_append_int_noprefix(entry, 8, 1);       /* Length */
> > +        build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */  
> nit s/ID/UID

it's ID in 1.0b

> > +        build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */
> > +        build_append_int_noprefix(entry, flags, 4); /* Flags */
> >      } else {
> > -        AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic);
> > -
> > -        apic->type = ACPI_APIC_LOCAL_X2APIC;
> > -        apic->length = sizeof(*apic);
> > -        apic->uid = cpu_to_le32(uid);
> > -        apic->x2apic_id = cpu_to_le32(apic_id);
> > -        apic->flags = cpu_to_le32(flags);
> > +        /* Rev 4.0, 5.2.12.12 Processor Local x2APIC Structure */
> > +        build_append_int_noprefix(entry, 9, 1);       /* Type */
> > +        build_append_int_noprefix(entry, 16, 1);      /* Length */
> > +        build_append_int_noprefix(entry, 0, 2);       /* Reserved */
> > +        build_append_int_noprefix(entry, apic_id, 4); /* X2APIC ID */
> > +        build_append_int_noprefix(entry, flags, 4);   /* Flags */
> > +        build_append_int_noprefix(entry, uid, 4);     /* ACPI Processor UID */
> >      }
> >  }
> >  
> > +static void build_ioapic(GArray *entry, uint8_t id, uint32_t addr, uint32_t irq)
> > +{
> > +    /* Rev 1.0b, 5.2.8.2 IO APIC */
> > +    build_append_int_noprefix(entry, 1, 1);    /* Type */
> > +    build_append_int_noprefix(entry, 12, 1);   /* Length */
> > +    build_append_int_noprefix(entry, id, 1);   /* IO APIC ID */
> > +    build_append_int_noprefix(entry, 0, 1);    /* Reserved */
> > +    build_append_int_noprefix(entry, addr, 4); /* IO APIC Address */
> > +    build_append_int_noprefix(entry, irq, 4);  /* System Vector Base */
> > +}
> > +
> > +static void
> > +build_xrupt_override(GArray *entry, uint8_t src, uint32_t gsi, uint16_t flags)
> > +{
> > +    /* Rev 1.0b, 5.2.8.3.1 Interrupt Source Overrides */
> > +    build_append_int_noprefix(entry, 2, 1);  /* Type */
> > +    build_append_int_noprefix(entry, 10, 1); /* Length */
> > +    build_append_int_noprefix(entry, 0, 1);  /* Bus */
> > +    build_append_int_noprefix(entry, src, 1);  /* Source */
> > +    /* Global System Interrupt Vector */
> > +    build_append_int_noprefix(entry, gsi, 4);
> > +    build_append_int_noprefix(entry, flags, 2);  /* Flags */
> > +}
> > +
> >  /*
> >   * ACPI spec, Revision 1.0b
> >   * 5.2.8 Multiple APIC Description Table
> > @@ -73,14 +98,11 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> >                       X86MachineState *x86ms, AcpiDeviceIf *adev,
> >                       const char *oem_id, const char *oem_table_id)
> >  {
> > +    int i;
> > +    bool x2apic_mode = false;
> >      MachineClass *mc = MACHINE_GET_CLASS(x86ms);
> >      const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> >      AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
> > -    bool x2apic_mode = false;
> > -
> > -    AcpiMadtIoApic *io_apic;
> > -    AcpiMadtIntsrcovr *intsrcovr;
> > -    int i;
> >      AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
> >                          .oem_table_id = oem_table_id };
> >  
> > @@ -96,30 +118,14 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> >          }
> >      }
> >  
> > -    io_apic = acpi_data_push(table_data, sizeof *io_apic);
> > -    io_apic->type = ACPI_APIC_IO;
> > -    io_apic->length = sizeof(*io_apic);
> > -    io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
> > -    io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
> > -    io_apic->interrupt = cpu_to_le32(0);
> > -
> > +    build_ioapic(table_data, ACPI_BUILD_IOAPIC_ID, IO_APIC_DEFAULT_ADDRESS, 0);
> >      if (x86ms->ioapic2) {
> > -        AcpiMadtIoApic *io_apic2;
> > -        io_apic2 = acpi_data_push(table_data, sizeof *io_apic);
> > -        io_apic2->type = ACPI_APIC_IO;
> > -        io_apic2->length = sizeof(*io_apic);
> > -        io_apic2->io_apic_id = ACPI_BUILD_IOAPIC_ID + 1;
> > -        io_apic2->address = cpu_to_le32(IO_APIC_SECONDARY_ADDRESS);
> > -        io_apic2->interrupt = cpu_to_le32(IO_APIC_SECONDARY_IRQBASE);
> > +        build_ioapic(table_data, ACPI_BUILD_IOAPIC_ID + 1,
> > +                     IO_APIC_SECONDARY_ADDRESS, IO_APIC_SECONDARY_IRQBASE);
> >      }
> >  
> >      if (x86ms->apic_xrupt_override) {
> > -        intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
> > -        intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
> > -        intsrcovr->length = sizeof(*intsrcovr);
> > -        intsrcovr->source = 0;
> > -        intsrcovr->gsi    = cpu_to_le32(2);
> > -        intsrcovr->flags  = cpu_to_le16(0); /* conforms to bus specifications */
> > +        build_xrupt_override(table_data, 0, 2, 0);  

> lost /* conforms to bus specifications */ comment for the flag during
> the battle/ Don't know if it is useful though


> >      }
> >  
> >      for (i = 1; i < 16; i++) {
> > @@ -127,32 +133,29 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> >              /* No need for a INT source override structure. */
> >              continue;
> >          }
> > -        intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
> > -        intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
> > -        intsrcovr->length = sizeof(*intsrcovr);
> > -        intsrcovr->source = i;
> > -        intsrcovr->gsi    = cpu_to_le32(i);
> > -        intsrcovr->flags  = cpu_to_le16(0xd); /* active high, level triggered */
> > +        build_xrupt_override(table_data, i, i, 0xd);  
> Lost for the flag /* active high, level triggered */
> >      }
> >  
> >      if (x2apic_mode) {
> > -        AcpiMadtLocalX2ApicNmi *local_nmi;
> > -
> > -        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
> > -        local_nmi->type   = ACPI_APIC_LOCAL_X2APIC_NMI;
> > -        local_nmi->length = sizeof(*local_nmi);
> > -        local_nmi->uid    = 0xFFFFFFFF; /* all processors */
> > -        local_nmi->flags  = cpu_to_le16(0);
> > -        local_nmi->lint   = 1; /* ACPI_LINT1 */
> > +        /* Rev 4.0, 5.2.12.13 Local x2APIC NMI Structure*/
> > +        build_append_int_noprefix(table_data, 0xA, 1); /* Type */
> > +        build_append_int_noprefix(table_data, 12, 1);  /* Length */
> > +        build_append_int_noprefix(table_data, 0, 2);   /* Flags */
> > +        /* ACPI Processor UID */
> > +        build_append_int_noprefix(table_data, 0xFFFFFFFF /* all processors */,
> > +                                  4);
> > +        /* Local x2APIC INTI# */  
> Local x2APIC LINT#
> > +        build_append_int_noprefix(table_data, 1 /* ACPI_LINT1 */, 1);
> > +        build_append_int_noprefix(table_data, 0, 3);  
> /* reserved */
> >      } else {
> > -        AcpiMadtLocalNmi *local_nmi;
> > -
> > -        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
> > -        local_nmi->type         = ACPI_APIC_LOCAL_NMI;
> > -        local_nmi->length       = sizeof(*local_nmi);
> > -        local_nmi->processor_id = 0xff; /* all processors */
> > -        local_nmi->flags        = cpu_to_le16(0);
> > -        local_nmi->lint         = 1; /* ACPI_LINT1 */
> > +        /* Rev 1.0b, 5.2.8.3.3 Local APIC NMI */
> > +        build_append_int_noprefix(table_data, 4, 1);  /* Type */
> > +        build_append_int_noprefix(table_data, 6, 1);  /* Length */
> > +        /* ACPI Processor ID */
> > +        build_append_int_noprefix(table_data, 0xff /* all processors */, 1);  
> 0xFF

> > +        build_append_int_noprefix(table_data, 0, 2);  /* Flags */
> > +        /* Local APIC INTI# */  
> Local APIC LINT#

it's "Local APIC INTI#" in 1.0b

> > +        build_append_int_noprefix(table_data, 1 /* ACPI_LINT1 */, 1);
> >      }
> >  
> >      acpi_table_end(linker, &table);
> >   

the rest is fixed up

> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
>