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

[Eric Auger]

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.

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
>>
>