[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 25/35] acpi: arm/virt: madt: use build_append_int_noprefix
|
From: |
Igor Mammedov |
|
Subject: |
Re: [PATCH v2 25/35] acpi: arm/virt: madt: use build_append_int_noprefix() API to compose MADT table |
|
Date: |
Mon, 6 Sep 2021 14:40:18 +0200 |
On Fri, 3 Sep 2021 15:45:00 +0200
Eric Auger <eauger@redhat.com> wrote:
> 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.
Change above is correcting stale comment, to document actual state of the code
there is no version upgrade here.
> > * 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?
yep, I'm trying to minimize changes not relevant to refactoring,
so here I basically mark TODO item and document the current state of the code
/some other patches also have similar comments for out of scope thing I noticed/
there is no change to the table contents.
> > + */
> > + /*
> > + * 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.
it was yet another Errata, already fixed in current WIP.
comment above now looks like this:
+ /*
+ * FIXME: Structure is from Revision 6.0 where 'GIC Structure'
+ * has additional fields on top of implemented 5.1 Errata A,
+ * to make it consistent with v6.0 we need to bump everything
+ * to v6.0
+ */
+ /*
+ * ACPI spec, Revision 6.0 Errata A
+ * (original 6.0 definition has invalid Length)
+ * 5.2.12.18 GIC ITS Structure
+ */
> 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);
> > }
> >
>