[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 18/35] acpi: build_dmar_q35: use acpi_table_begin()/acpi_t
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v3 18/35] acpi: build_dmar_q35: use acpi_table_begin()/acpi_table_end() instead of build_header() |
Date: |
Wed, 22 Sep 2021 12:06:17 +0200 |
On Wed, 22 Sep 2021 11:19:05 +0200
Eric Auger <eauger@redhat.com> wrote:
> On 9/7/21 4:47 PM, Igor Mammedov wrote:
> > it replaces error-prone pointer arithmetic for build_header() API,
> > with 2 calls to start and finish table creation,
> > which hides offsets magic from API user.
> >
> > While at it switch to build_append_int_noprefix() to build
> > table entries tables.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> > - rebase on top 26863366b29 (hw/i386/acpi-build: Add DMAR support to
> > bypass iommu)
> > - s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/
> >
> > CC: wangxingang5@huawei.com
> > CC: marcel.apfelbaum@gmail.com
> > ---
> > include/hw/acpi/acpi-defs.h | 68 --------------------------
> > hw/i386/acpi-build.c | 97 ++++++++++++++++++++-----------------
> > 2 files changed, 53 insertions(+), 112 deletions(-)
> >
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index d293304f9c..c4f0a202e8 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -358,74 +358,6 @@ struct AcpiGenericTimerTable {
> > } QEMU_PACKED;
> > typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
> >
> > -/* DMAR - DMA Remapping table r2.2 */
> > -struct AcpiTableDmar {
> > - ACPI_TABLE_HEADER_DEF
> > - uint8_t host_address_width; /* Maximum DMA physical addressability */
> > - uint8_t flags;
> > - uint8_t reserved[10];
> > -} QEMU_PACKED;
> > -typedef struct AcpiTableDmar AcpiTableDmar;
> > -
> > -/* Masks for Flags field above */
> > -#define ACPI_DMAR_INTR_REMAP 1
> > -#define ACPI_DMAR_X2APIC_OPT_OUT (1 << 1)
> > -
> > -/* Values for sub-structure type for DMAR */
> > -enum {
> > - ACPI_DMAR_TYPE_HARDWARE_UNIT = 0, /* DRHD */
> > - ACPI_DMAR_TYPE_RESERVED_MEMORY = 1, /* RMRR */
> > - ACPI_DMAR_TYPE_ATSR = 2, /* ATSR */
> > - ACPI_DMAR_TYPE_HARDWARE_AFFINITY = 3, /* RHSR */
> > - ACPI_DMAR_TYPE_ANDD = 4, /* ANDD */
> > - ACPI_DMAR_TYPE_RESERVED = 5 /* Reserved for furture use */
> > -};
> > -
> > -/*
> > - * Sub-structures for DMAR
> > - */
> > -
> > -/* Device scope structure for DRHD. */
> > -struct AcpiDmarDeviceScope {
> > - uint8_t entry_type;
> > - uint8_t length;
> > - uint16_t reserved;
> > - uint8_t enumeration_id;
> > - uint8_t bus;
> > - struct {
> > - uint8_t device;
> > - uint8_t function;
> > - } path[];
> > -} QEMU_PACKED;
> > -typedef struct AcpiDmarDeviceScope AcpiDmarDeviceScope;
> > -
> > -/* Type 0: Hardware Unit Definition */
> > -struct AcpiDmarHardwareUnit {
> > - uint16_t type;
> > - uint16_t length;
> > - uint8_t flags;
> > - uint8_t reserved;
> > - uint16_t pci_segment; /* The PCI Segment associated with this unit */
> > - uint64_t address; /* Base address of remapping hardware register-set
> > */
> > - AcpiDmarDeviceScope scope[];
> > -} QEMU_PACKED;
> > -typedef struct AcpiDmarHardwareUnit AcpiDmarHardwareUnit;
> > -
> > -/* Type 2: Root Port ATS Capability Reporting Structure */
> > -struct AcpiDmarRootPortATS {
> > - uint16_t type;
> > - uint16_t length;
> > - uint8_t flags;
> > - uint8_t reserved;
> > - uint16_t pci_segment;
> > - AcpiDmarDeviceScope scope[];
> > -} QEMU_PACKED;
> > -typedef struct AcpiDmarRootPortATS AcpiDmarRootPortATS;
> > -
> > -/* Masks for Flags field above */
> > -#define ACPI_DMAR_INCLUDE_PCI_ALL 1
> > -#define ACPI_DMAR_ATSR_ALL_PORTS 1
> > -
> > /*
> > * Input Output Remapping Table (IORT)
> > * Conforms to "IO Remapping Table System Software on ARM Platforms",
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 51e0ba07b6..2875c4f393 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2064,8 +2064,9 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> > MachineState *machine)
> > static void
> > insert_scope(PCIBus *bus, PCIDevice *dev, void *opaque)
> > {
> > + const size_t device_scope_size = 6 /* device scope structure */ +
> > + 2 /* 1 path entry */;
> > GArray *scope_blob = opaque;
> > - AcpiDmarDeviceScope *scope = NULL;
> >
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> > /* Dmar Scope Type: 0x02 for PCI Bridge */
> > @@ -2076,8 +2077,7 @@ insert_scope(PCIBus *bus, PCIDevice *dev, void
> > *opaque)
> > }
> >
> > /* length */
> > - build_append_int_noprefix(scope_blob,
> > - sizeof(*scope) + sizeof(scope->path[0]), 1);
> > + build_append_int_noprefix(scope_blob, device_scope_size, 1);
> > /* reserved */
> > build_append_int_noprefix(scope_blob, 0, 2);
> > /* enumeration_id */
> > @@ -2109,26 +2109,31 @@ dmar_host_bridges(Object *obj, void *opaque)
> > }
> >
> > /*
> > - * VT-d spec 8.1 DMA Remapping Reporting Structure
> > - * (version Oct. 2014 or later)
> > + * Intel ® Virtualization Technology for Directed I/O
> > + * Architecture Specification. Revision 3.3
> > + * 8.1 DMA Remapping Reporting Structure
> > */
> > static void
> > build_dmar_q35(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> > const char *oem_table_id)
> > {
> > - int dmar_start = table_data->len;
> > -
> > - AcpiTableDmar *dmar;
> > - AcpiDmarHardwareUnit *drhd;
> > - AcpiDmarRootPortATS *atsr;
> > uint8_t dmar_flags = 0;
> > + uint8_t rsvd10[10] = {};
> > + /* Root complex IOAPIC uses one path only */
> > + const size_t ioapic_scope_size = 6 /* device scope structure */ +
> > + 2 /* 1 path entry */;
> > X86IOMMUState *iommu = x86_iommu_get_default();
> > - AcpiDmarDeviceScope *scope = NULL;
> > - /* Root complex IOAPIC use one path[0] only */
> > - size_t ioapic_scope_size = sizeof(*scope) + sizeof(scope->path[0]);
> > IntelIOMMUState *intel_iommu = INTEL_IOMMU_DEVICE(iommu);
> > GArray *scope_blob = g_array_new(false, true, 1);
> >
> > + AcpiTable table = { .sig = "DMAR", .rev = 1, .oem_id = oem_id,
> > + .oem_table_id = oem_table_id };
> > +
> > + assert(iommu);
> > + if (x86_iommu_ir_supported(iommu)) {
> > + dmar_flags |= 0x1; /* Flags: 0x1: INT_REMAP */
> > + }
> > +
> > /*
> > * A PCI bus walk, for each PCI host bridge.
> > * Insert scope for each PCI bridge and endpoint device which
> > @@ -2137,48 +2142,52 @@ build_dmar_q35(GArray *table_data, BIOSLinker
> > *linker, const char *oem_id,
> > object_child_foreach_recursive(object_get_root(),
> > dmar_host_bridges, scope_blob);
> >
> > - assert(iommu);
> > - if (x86_iommu_ir_supported(iommu)) {
> > - dmar_flags |= 0x1; /* Flags: 0x1: INT_REMAP */
> > - }
> why this move?
it's not really necessary/related, probably it was moved to be closer
to other variables initialization.
> > -
> > - dmar = acpi_data_push(table_data, sizeof(*dmar));
> > - dmar->host_address_width = intel_iommu->aw_bits - 1;
> > - dmar->flags = dmar_flags;
> > -
> > - /* DMAR Remapping Hardware Unit Definition structure */
> > - drhd = acpi_data_push(table_data, sizeof(*drhd) + ioapic_scope_size);
> > - drhd->type = cpu_to_le16(ACPI_DMAR_TYPE_HARDWARE_UNIT);
> > - drhd->length =
> > - cpu_to_le16(sizeof(*drhd) + ioapic_scope_size + scope_blob->len);
> > - drhd->flags = 0; /* Don't include all pci device */
> > - drhd->pci_segment = cpu_to_le16(0);
> > - drhd->address = cpu_to_le64(Q35_HOST_BRIDGE_IOMMU_ADDR);
> > + acpi_table_begin(&table, table_data);
> > + /* Host Address Width */
> > + build_append_int_noprefix(table_data, intel_iommu->aw_bits - 1, 1);
> > + build_append_int_noprefix(table_data, dmar_flags, 1); /* Flags */
> > + g_array_append_vals(table_data, rsvd10, sizeof(rsvd10)); /* Reserved */
> > +
> > + /* 8.3 DMAR Remapping Hardware Unit Definition structure */
> > + build_append_int_noprefix(table_data, 0, 2); /* Type */
> > + /* Length */
> > + build_append_int_noprefix(table_data,
> > + 16 + ioapic_scope_size + scope_blob->len, 2);
> > + /* Flags */
> > + build_append_int_noprefix(table_data, 0 /* Don't include all pci
> > device */ ,
> > + 1);
> > + build_append_int_noprefix(table_data, 0 , 1); /* Reserved */
> > + build_append_int_noprefix(table_data, 0 , 2); /* Segment Number */
> > + /* Register Base Address */
> > + build_append_int_noprefix(table_data, Q35_HOST_BRIDGE_IOMMU_ADDR , 8);
> >
> > /* Scope definition for the root-complex IOAPIC. See VT-d spec
> > * 8.3.1 (version Oct. 2014 or later). */
> > - scope = &drhd->scope[0];
> > - scope->entry_type = 0x03; /* Type: 0x03 for IOAPIC */
> > - scope->length = ioapic_scope_size;
> > - scope->enumeration_id = ACPI_BUILD_IOAPIC_ID;
> > - scope->bus = Q35_PSEUDO_BUS_PLATFORM;
> > - scope->path[0].device = PCI_SLOT(Q35_PSEUDO_DEVFN_IOAPIC);
> > - scope->path[0].function = PCI_FUNC(Q35_PSEUDO_DEVFN_IOAPIC);
> > + build_append_int_noprefix(table_data, 0x03 /* IOAPIC */, 1); /* Type */
> > + build_append_int_noprefix(table_data, ioapic_scope_size, 1); /* Length
> > */
> > + build_append_int_noprefix(table_data, 0, 2); /* Reserved */
> > + /* Enumeration ID */
> > + build_append_int_noprefix(table_data, ACPI_BUILD_IOAPIC_ID, 1);
> > + /* Start Bus Number */
> > + build_append_int_noprefix(table_data, Q35_PSEUDO_BUS_PLATFORM, 1);
> > + /* Path, {Device, Function} pair */
> > + build_append_int_noprefix(table_data,
> > PCI_SLOT(Q35_PSEUDO_DEVFN_IOAPIC), 1);
> > + build_append_int_noprefix(table_data,
> > PCI_FUNC(Q35_PSEUDO_DEVFN_IOAPIC), 1);
> >
> > /* Add scope found above */
> > g_array_append_vals(table_data, scope_blob->data, scope_blob->len);
> > g_array_free(scope_blob, true);
> >
> > if (iommu->dt_supported) {
> > - atsr = acpi_data_push(table_data, sizeof(*atsr));
> > - atsr->type = cpu_to_le16(ACPI_DMAR_TYPE_ATSR);
> > - atsr->length = cpu_to_le16(sizeof(*atsr));
> > - atsr->flags = ACPI_DMAR_ATSR_ALL_PORTS;
> > - atsr->pci_segment = cpu_to_le16(0);
> > + /* 8.5 Root Port ATS Capability Reporting Structure */
> > + build_append_int_noprefix(table_data, 2, 2); /* Type */
> > + build_append_int_noprefix(table_data, 8, 2); /* Length */
> 8 + no device scope
yep, but given no device scope implemented I didn't see a need to mention it
at all.
> > + build_append_int_noprefix(table_data, 1 /* ALL_PORTS */, 1); /*
> > Flags */
> > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 2); /* Segment Number */
> > }
> >
> > - build_header(linker, table_data, (void *)(table_data->data +
> > dmar_start),
> > - "DMAR", table_data->len - dmar_start, 1, oem_id,
> > oem_table_id);
> > + acpi_table_end(linker, &table);
> > }
> >
> > /*
> >
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
> Eric
>
- Re: [PATCH v3 12/35] acpi: vmgenid_build_acpi: use acpi_table_begin()/acpi_table_end() instead of build_header(), (continued)
- [PATCH v3 13/35] acpi: x86: build_dsdt: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 14/35] acpi: build_hpet: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 16/35] acpi: arm/x86: build_srat: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 15/35] acpi: build_tpm_tcpa: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 18/35] acpi: build_dmar_q35: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 17/35] acpi: use build_append_int_noprefix() API to compose SRAT table, Igor Mammedov, 2021/09/07
- [PATCH v3 19/35] acpi: build_waet: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 21/35] acpi: madt: arm/x86: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07
- [PATCH v3 20/35] acpi: build_amd_iommu: use acpi_table_begin()/acpi_table_end() instead of build_header(), Igor Mammedov, 2021/09/07