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 17:37:40 +0200
Eric Auger <eauger@redhat.com> wrote:

> Hi,
> On 9/22/21 5:30 PM, Igor Mammedov wrote:
> > 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  
> OK sorry for the noise. Maybe we should add this link somewhere
> https://uefi.org/acpi/specs
> 
> I may have looked at some other references when reviewing.

problems with the links is that they are usually go stale
(good or rather bad example of that is so called Microsoft 'specs' html docs,
ex: early revisions of SRAT)

That's why we put in put in comment spec name and revision which
is usually sufficient to reliably find a spec in question,
regardless of where it's hosted at the moment.


> 
> Thanks
> 
> Eric
> >   
> >>> +        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
> >>  
> >   
>