Re: [PATCH v2 25/35] acpi: arm/virt: madt: use build_append_int_noprefix() API to compose MADT table

[Eric Auger]

Hi Igor,
On 7/8/21 5:46 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: drjones@redhat.com
> CC: peter.maydell@linaro.org
> CC: shannon.zhaosl@gmail.com
> CC: qemu-arm@nongnu.org
> ---
>  include/hw/acpi/acpi-defs.h |  84 --------------------
>  hw/arm/virt-acpi-build.c    | 149 +++++++++++++++++++++---------------
>  2 files changed, 89 insertions(+), 144 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 3f174ba208..bcada37601 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -163,90 +163,6 @@ struct AcpiFacsDescriptorRev1 {
>  } QEMU_PACKED;
>  typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
>  
> -/* Values for Type in APIC sub-headers */
> -
> -#define ACPI_APIC_GENERIC_CPU_INTERFACE 11
> -#define ACPI_APIC_GENERIC_DISTRIBUTOR   12
> -#define ACPI_APIC_GENERIC_MSI_FRAME     13
> -#define ACPI_APIC_GENERIC_REDISTRIBUTOR 14
> -#define ACPI_APIC_GENERIC_TRANSLATOR    15
> -#define ACPI_APIC_RESERVED              16   /* 16 and greater are reserved 
> */
> -
> -/*
> - * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
> - */
> -#define ACPI_SUB_HEADER_DEF   /* Common ACPI sub-structure header */\
> -    uint8_t  type;                               \
> -    uint8_t  length;
> -
> -/* Sub-structures for MADT */
> -
> -struct AcpiMadtGenericCpuInterface {
> -    ACPI_SUB_HEADER_DEF
> -    uint16_t reserved;
> -    uint32_t cpu_interface_number;
> -    uint32_t uid;
> -    uint32_t flags;
> -    uint32_t parking_version;
> -    uint32_t performance_interrupt;
> -    uint64_t parked_address;
> -    uint64_t base_address;
> -    uint64_t gicv_base_address;
> -    uint64_t gich_base_address;
> -    uint32_t vgic_interrupt;
> -    uint64_t gicr_base_address;
> -    uint64_t arm_mpidr;
> -} QEMU_PACKED;
> -
> -typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
> -
> -/* GICC CPU Interface Flags */
> -#define ACPI_MADT_GICC_ENABLED 1
> -
> -struct AcpiMadtGenericDistributor {
> -    ACPI_SUB_HEADER_DEF
> -    uint16_t reserved;
> -    uint32_t gic_id;
> -    uint64_t base_address;
> -    uint32_t global_irq_base;
> -    /* ACPI 5.1 Errata 1228 Present GIC version in MADT table */
> -    uint8_t version;
> -    uint8_t reserved2[3];
> -} QEMU_PACKED;
> -
> -typedef struct AcpiMadtGenericDistributor AcpiMadtGenericDistributor;
> -
> -struct AcpiMadtGenericMsiFrame {
> -    ACPI_SUB_HEADER_DEF
> -    uint16_t reserved;
> -    uint32_t gic_msi_frame_id;
> -    uint64_t base_address;
> -    uint32_t flags;
> -    uint16_t spi_count;
> -    uint16_t spi_base;
> -} QEMU_PACKED;
> -
> -typedef struct AcpiMadtGenericMsiFrame AcpiMadtGenericMsiFrame;
> -
> -struct AcpiMadtGenericRedistributor {
> -    ACPI_SUB_HEADER_DEF
> -    uint16_t reserved;
> -    uint64_t base_address;
> -    uint32_t range_length;
> -} QEMU_PACKED;
> -
> -typedef struct AcpiMadtGenericRedistributor AcpiMadtGenericRedistributor;
> -
> -struct AcpiMadtGenericTranslator {
> -    ACPI_SUB_HEADER_DEF
> -    uint16_t reserved;
> -    uint32_t translation_id;
> -    uint64_t base_address;
> -    uint32_t reserved2;
> -} QEMU_PACKED;
> -
> -typedef struct AcpiMadtGenericTranslator AcpiMadtGenericTranslator;
> -
>  /*
>   * Generic Timer Description Table (GTDT)
>   */
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index cd8b73dde9..dd8011a670 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -476,94 +476,123 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>  }
>  
>  /*
> - * ACPI spec, Revision 5.0
> + * ACPI spec, Revision 5.1 Errata A
It is a bit difficult to follow both the conversion and the upgrade of
the version. May be worth splitting both concerns if sensible.
>   * 5.2.12 Multiple APIC Description Table (MADT)
>   */
> +static void build_append_gicr(GArray *table_data, uint64_t base, uint32_t 
> size)
> +{
> +    build_append_int_noprefix(table_data, 0xE, 1);  /* Type */
> +    build_append_int_noprefix(table_data, 16, 1);   /* Length */
> +    build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
> +    /* Discovery Range Base Addres */
> +    build_append_int_noprefix(table_data, base, 8);
> +    build_append_int_noprefix(table_data, size, 4); /* Discovery Range 
> Length */
> +}
> +
>  static void
>  build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
> +    int i;
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>      const MemMapEntry *memmap = vms->memmap;
> -    const int *irqmap = vms->irqmap;
> -    AcpiMadtGenericDistributor *gicd;
> -    AcpiMadtGenericMsiFrame *gic_msi;
> -    int i;
>      AcpiTable table = { .sig = "APIC", .rev = 3, .oem_id = vms->oem_id,
>                          .oem_table_id = vms->oem_table_id };
>  
>      acpi_init_table(&table, table_data);
>      /* Local Interrupt Controller Address */
>      build_append_int_noprefix(table_data, 0, 4);
> -    build_append_int_noprefix(table_data, 0, 4); /* Flags */
> -
> -    gicd = acpi_data_push(table_data, sizeof *gicd);
> -    gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
> -    gicd->length = sizeof(*gicd);
> -    gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
> -    gicd->version = vms->gic_version;
> +    build_append_int_noprefix(table_data, 0, 4);   /* Flags */
> +
> +    /* 5.2.12.15 GIC Distributor Structure */
> +    build_append_int_noprefix(table_data, 0xC, 1); /* Type */
> +    build_append_int_noprefix(table_data, 24, 1);  /* Length */
> +    build_append_int_noprefix(table_data, 0, 2);   /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 4);   /* GIC ID */
> +    /* Physical Base Address */
> +    build_append_int_noprefix(table_data, memmap[VIRT_GIC_DIST].base, 8);
> +    build_append_int_noprefix(table_data, 0, 4);   /* System Vector Base */
> +    /* GIC version */
> +    build_append_int_noprefix(table_data, vms->gic_version, 1);
> +    build_append_int_noprefix(table_data, 0, 3);   /* Reserved */
>  
>      for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
> -        AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
> -                                                           sizeof(*gicc));
>          ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
> +        uint64_t physical_base_address = 0, gich = 0, gicv = 0;
> +        uint32_t vgic_interrupt = vms->virt ? PPI(ARCH_GIC_MAINT_IRQ) : 0;
> +        uint32_t pmu_interrupt = arm_feature(&armcpu->env, ARM_FEATURE_PMU) ?
> +                                             PPI(VIRTUAL_PMU_IRQ) : 0;
>  
> -        gicc->type = ACPI_APIC_GENERIC_CPU_INTERFACE;
> -        gicc->length = sizeof(*gicc);
>          if (vms->gic_version == 2) {
> -            gicc->base_address = cpu_to_le64(memmap[VIRT_GIC_CPU].base);
> -            gicc->gich_base_address = cpu_to_le64(memmap[VIRT_GIC_HYP].base);
> -            gicc->gicv_base_address = 
> cpu_to_le64(memmap[VIRT_GIC_VCPU].base);
> +            physical_base_address = memmap[VIRT_GIC_CPU].base;
> +            gicv = memmap[VIRT_GIC_VCPU].base;
> +            gich = memmap[VIRT_GIC_HYP].base;
>          }
> -        gicc->cpu_interface_number = cpu_to_le32(i);
> -        gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
> -        gicc->uid = cpu_to_le32(i);
> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>  
> -        if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> -            gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> -        }
> -        if (vms->virt) {
> -            gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
> -        }
> +        /* 5.2.12.14 GIC Structure */
> +        build_append_int_noprefix(table_data, 0xB, 1);  /* Type */
> +        build_append_int_noprefix(table_data, 76, 1);   /* Length */
> +        build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
> +        build_append_int_noprefix(table_data, i, 4);    /* GIC ID */
> +        build_append_int_noprefix(table_data, i, 4);    /* ACPI Processor 
> UID */
> +        /* Flags */
> +        build_append_int_noprefix(table_data, 1, 4);    /* Enabled */
> +        /* Parking Protocol Version */
> +        build_append_int_noprefix(table_data, 0, 4);
> +        /* Performance Interrupt GSIV */
> +        build_append_int_noprefix(table_data, pmu_interrupt, 4);
> +        build_append_int_noprefix(table_data, 0, 8); /* Parked Address */
> +        /* Physical Base Address */
> +        build_append_int_noprefix(table_data, physical_base_address, 8);
> +        build_append_int_noprefix(table_data, gicv, 8); /* GICV */
> +        build_append_int_noprefix(table_data, gich, 8); /* GICH */
> +        /* VGIC Maintenance interrupt */
> +        build_append_int_noprefix(table_data, vgic_interrupt, 4);
> +        build_append_int_noprefix(table_data, 0, 8);    /* GICR Base 
> Address*/
> +        /* MPIDR */
> +        build_append_int_noprefix(table_data, armcpu->mp_affinity, 8);
>      }
>  
>      if (vms->gic_version == 3) {
> -        AcpiMadtGenericTranslator *gic_its;
> -        int nb_redist_regions = virt_gicv3_redist_region_count(vms);
> -        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
> -                                                         sizeof *gicr);
> -
> -        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> -        gicr->length = sizeof(*gicr);
> -        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
> -        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
> -
> -        if (nb_redist_regions == 2) {
> -            gicr = acpi_data_push(table_data, sizeof(*gicr));
> -            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> -            gicr->length = sizeof(*gicr);
> -            gicr->base_address =
> -                cpu_to_le64(memmap[VIRT_HIGH_GIC_REDIST2].base);
> -            gicr->range_length =
> -                cpu_to_le32(memmap[VIRT_HIGH_GIC_REDIST2].size);
> +        build_append_gicr(table_data, memmap[VIRT_GIC_REDIST].base,
> +                                      memmap[VIRT_GIC_REDIST].size);
> +        if (virt_gicv3_redist_region_count(vms) == 2) {
> +            build_append_gicr(table_data, memmap[VIRT_HIGH_GIC_REDIST2].base,
> +                                          
> memmap[VIRT_HIGH_GIC_REDIST2].size);
>          }
>  
>          if (its_class_name() && !vmc->no_its) {
> -            gic_its = acpi_data_push(table_data, sizeof *gic_its);
> -            gic_its->type = ACPI_APIC_GENERIC_TRANSLATOR;
> -            gic_its->length = sizeof(*gic_its);
> -            gic_its->translation_id = 0;
> -            gic_its->base_address = cpu_to_le64(memmap[VIRT_GIC_ITS].base);
> +            /*
> +             * FIXME: Structure is from Revision 6.0 where 'GIC Structure'
> +             * has additional fields on top of implemented 5.1 Errata A
> +             * make it consistent with v6.0 if ITS is present or bump
> +             * everything to v6.0 unconditionally
Do you mean some parts are 5.1 errata A and some others are 6.0 already?
> +             */
> +            /*
> +             * ACPI spec, Revision  6.0
> +             * 5.2.12.18 GIC ITS Structure
> +             */
> +            build_append_int_noprefix(table_data, 0xF, 1);  /* Type */
> +            build_append_int_noprefix(table_data, 16, 1);   /* Length */
this is 20 and not 16. Currently it fails with ITS.

With that fix
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> +            build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
> +            build_append_int_noprefix(table_data, 0, 4);    /* GIC ITS ID */
> +            /* Physical Base Address */
> +            build_append_int_noprefix(table_data, memmap[VIRT_GIC_ITS].base, 
> 8);
> +            build_append_int_noprefix(table_data, 0, 4);    /* Reserved */
>          }
>      } else {
> -        gic_msi = acpi_data_push(table_data, sizeof *gic_msi);
> -        gic_msi->type = ACPI_APIC_GENERIC_MSI_FRAME;
> -        gic_msi->length = sizeof(*gic_msi);
> -        gic_msi->gic_msi_frame_id = 0;
> -        gic_msi->base_address = cpu_to_le64(memmap[VIRT_GIC_V2M].base);
> -        gic_msi->flags = cpu_to_le32(1);
> -        gic_msi->spi_count = cpu_to_le16(NUM_GICV2M_SPIS);
> -        gic_msi->spi_base = cpu_to_le16(irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE);
> +        const uint16_t spi_base = vms->irqmap[VIRT_GIC_V2M] + ARM_SPI_BASE;
> +
> +        /* 5.2.12.16 GIC MSI Frame Structure */
> +        build_append_int_noprefix(table_data, 0xD, 1);  /* Type */
> +        build_append_int_noprefix(table_data, 24, 1);   /* Length */
> +        build_append_int_noprefix(table_data, 0, 2);    /* Reserved */
> +        build_append_int_noprefix(table_data, 0, 4);    /* GIC MSI Frame ID 
> */
> +        /* Physical Base Address */
> +        build_append_int_noprefix(table_data, memmap[VIRT_GIC_V2M].base, 8);
> +        build_append_int_noprefix(table_data, 1, 4);    /* Flags */
> +        /* SPI Count */
> +        build_append_int_noprefix(table_data, NUM_GICV2M_SPIS, 2);
> +        build_append_int_noprefix(table_data, spi_base, 2); /* SPI Base */
>      }
>      acpi_table_composed(linker, &table);
>  }
>