From: "Michael S. Tsirkin" <mst@redhat.com>
To: Sairaj Kodilkar <sarunkod@amd.com>
Cc: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
Ani Sinha <anisinha@redhat.com>,
Eduardo Habkost <eduardo@habkost.net>,
Igor Mammedov <imammedo@redhat.com>,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
qemu-devel@nongnu.org, vasant.hegde@amd.com,
suravee.suthikulpanit@amd.com
Subject: Re: [PATCH 7/8] acpi_build: Cleanup AMD IOMMU IVRS building
Date: Mon, 3 Aug 2026 17:56:24 -0400 [thread overview]
Message-ID: <20260803175508-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20260511123937.32743-8-sarunkod@amd.com>
On Mon, May 11, 2026 at 06:09:36PM +0530, Sairaj Kodilkar wrote:
> Use structs and macros to improve the readability and maintainability of
> the the code.
>
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
> hw/i386/acpi-build.c | 149 +++++++++++++++++--------------------------
> 1 file changed, 59 insertions(+), 90 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 82208e06e155..e18c9be801a2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1663,11 +1663,13 @@ static void
> insert_ivhd(PCIBus *bus, PCIDevice *dev, void *opaque)
> {
> GArray *table_data = opaque;
> - uint32_t entry;
> + AmdIvhdDeviceEntry entry = {};
>
> /* "Select" IVHD entry, type 0x2 */
> - entry = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn) << 8 | 0x2;
> - build_append_int_noprefix(table_data, entry, 4);
> + entry.type = AMD_IVHD_DEVICE_ENTRY_TYPE_SELECT;
> + entry.devid = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
> +
> + g_array_append_vals(table_data, &entry, sizeof(entry));
>
> if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> @@ -1691,11 +1693,14 @@ insert_ivhd(PCIBus *bus, PCIDevice *dev, void *opaque)
> */
> if (sec == sub) { /* leaf bus */
> /* "Start of Range" IVHD entry, type 0x3 */
> - entry = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0)) << 8 | 0x3;
> - build_append_int_noprefix(table_data, entry, 4);
> + entry.type = AMD_IVHD_DEVICE_ENTRY_TYPE_START_RANGE;
> + entry.devid = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
> + g_array_append_vals(table_data, &entry, sizeof(entry));
> +
> /* "End of Range" IVHD entry, type 0x4 */
> - entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> - build_append_int_noprefix(table_data, entry, 4);
> + entry.type = AMD_IVHD_DEVICE_ENTRY_TYPE_END_RANGE;
> + entry.devid = PCI_BUILD_BDF(sub, PCI_DEVFN_MAX - 1);
> + g_array_append_vals(table_data, &entry, sizeof(entry));
> } else {
> pci_for_each_device(sec_bus, sec, insert_ivhd, table_data);
> }
> @@ -1708,24 +1713,26 @@ insert_ivhd(PCIBus *bus, PCIDevice *dev, void *opaque)
> * express bridges, just as in pci_device_iommu_address_space().
> * DeviceIDa vs DeviceIDb as per the AMD IOMMU spec.
> */
> - uint16_t dev_id_a, dev_id_b;
> + AmdIvhdDeviceEntryExt entry_ext = {};
>
> - dev_id_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
> + entry_ext.type = AMD_IVHD_DEVICE_ENTRY_TYPE_ALIAS_START_RANGE;
> + entry_ext.devid_a = PCI_BUILD_BDF(sec, PCI_DEVFN(0, 0));
>
> if (pci_is_express(dev) &&
> pcie_cap_get_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE) {
> - dev_id_b = dev_id_a;
> + entry_ext.devid_b = entry_ext.devid_a;
> } else {
> - dev_id_b = PCI_BUILD_BDF(pci_bus_num(bus), dev->devfn);
> + entry_ext.devid_b = PCI_BUILD_BDF(pci_bus_num(bus),
> + dev->devfn);
> }
>
> /* "Alias Start of Range" IVHD entry, type 0x43, 8 bytes */
> - build_append_int_noprefix(table_data, dev_id_a << 8 | 0x43, 4);
> - build_append_int_noprefix(table_data, dev_id_b << 8 | 0x0, 4);
> + g_array_append_vals(table_data, &entry_ext, sizeof(entry_ext));
>
> /* "End of Range" IVHD entry, type 0x4 */
> - entry = PCI_BUILD_BDF(sub, PCI_DEVFN(31, 7)) << 8 | 0x4;
> - build_append_int_noprefix(table_data, entry, 4);
> + entry.type = AMD_IVHD_DEVICE_ENTRY_TYPE_END_RANGE;
> + entry.devid = PCI_BUILD_BDF(sub, PCI_DEVFN_MAX - 1);
> + g_array_append_vals(table_data, &entry, sizeof(entry));
> }
> }
> }
> @@ -1786,20 +1793,20 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> GArray *ivhd_blob = g_array_new(false, true, 1);
> AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
> .oem_table_id = oem_table_id };
> - uint64_t feature_report;
> int iommu_bus = pci_bus_num(pci_get_bus(iommu_dev));
> uint16_t iommu_devid = PCI_BUILD_BDF(iommu_bus, iommu_dev->devfn);
> + AmdIvrsVendorHdr ivrs_hdr = {};
> + AmdIvhdHdr10 ivhd10 = {};
> + AmdIvhdHdr11 ivhd11 = {};
>
> acpi_table_begin(&table, table_data);
> /* IVinfo - IO virtualization information common to all
> * IOMMU units in a system
> */
> - build_append_int_noprefix(table_data,
> - (1UL << 0) | /* EFRSup */
> - AMDVI_PA_SIZE_52,
> - 4);
> - /* reserved */
> - build_append_int_noprefix(table_data, 0, 8);
> + ivrs_hdr.ivinfo = AMD_IVINFO_EFR_SUP | AMDVI_GVA_SIZE_48 |
> + AMDVI_PA_SIZE_52 | AMDVI_VA_SIZE_64;
> +
> + g_array_append_vals(table_data, &ivrs_hdr, sizeof(ivrs_hdr));
This has broken endian-ness.
Please do not use packed structs.
ACPI has infrastructure to build up structs such as
build_append_int_noprefix.
Just use that.
>
> /*
> * A PCI bus walk, for each PCI host bridge, is necessary to create a
> @@ -1817,7 +1824,8 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> * These are 4-byte device entries currently reporting the range of
> * Refer to Spec - Table 95:IVHD Device Entry Type Codes(4-byte)
> */
> - build_append_int_noprefix(ivhd_blob, 0x0000001, 4);
> + AmdIvhdDeviceEntry entry = { .type = AMD_IVHD_DEVICE_ENTRY_TYPE_ALL };
> + g_array_append_vals(ivhd_blob, &entry, sizeof(entry));
> }
>
> /*
> @@ -1829,76 +1837,37 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> * See Linux kernel commit 'c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059'
> */
> if (x86_iommu_ir_supported(x86_iommu_get_default())) {
> - build_append_int_noprefix(ivhd_blob,
> - (0x1ull << 56) | /* type IOAPIC */
> - (IOAPIC_SB_DEVID << 40) | /* IOAPIC devid */
> - 0x48, /* special device */
> - 8);
> - }
> -
> - /* IVHD definition - type 10h */
> - build_append_int_noprefix(table_data, 0x10, 1);
> - /* virtualization flags */
> - build_append_int_noprefix(table_data,
> - (1UL << 0) | /* HtTunEn */
> - (1UL << 4) | /* iotblSup */
> - (1UL << 6) | /* PrefSup */
> - (1UL << 7), /* PPRSup */
> - 1);
> -
> - /* IVHD length */
> - build_append_int_noprefix(table_data, ivhd_blob->len + 24, 2);
> - /* DeviceID */
> - build_append_int_noprefix(table_data, iommu_devid, 2);
> - /* Capability offset */
> - build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> - /* IOMMU base address */
> - build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
> - /* PCI Segment Group */
> - build_append_int_noprefix(table_data, 0, 2);
> - /* IOMMU info */
> - build_append_int_noprefix(table_data, 0, 2);
> - /* IOMMU Feature Reporting */
> - feature_report = get_amd_ivhd_feature_report(s);
> - build_append_int_noprefix(table_data, feature_report, 4);
> -
> + AmdIvhdDeviceEntryExt entry_ext = {
> + .type = AMD_IVHD_DEVICE_ENTRY_TYPE_SPECIAL_DEVICE,
> + .devid_b = IOAPIC_SB_DEVID,
> + .variety = IVHD_VARIETY_IOAPIC
> + };
> +
> + g_array_append_vals(ivhd_blob, &entry_ext, sizeof(entry_ext));
> + }
> +
> + ivhd10.type = 0x10;
> + ivhd10.flags = AMD_IVHD_FLAG_HT_TUN_EN | AMD_IVHD_FLAG_IOTLB_SUP |
> + AMD_IVHD_FLAG_PREF_SUP | AMD_IVHD_FLAG_PPR_SUP;
> + ivhd10.length = ivhd_blob->len + sizeof(ivhd10);
> + ivhd10.devid = iommu_devid;
> + ivhd10.capab_offset = s->pci->capab_offset;
> + ivhd10.base_addr = s->mr_mmio.addr;
> + ivhd10.iommu_feature_report = get_amd_ivhd_feature_report(s);
> + g_array_append_vals(table_data, &ivhd10, sizeof(ivhd10));
> /* IVHD entries as found above */
> g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
>
> - /* IVHD definition - type 11h */
> - build_append_int_noprefix(table_data, 0x11, 1);
> - /* virtualization flags */
> - build_append_int_noprefix(table_data,
> - (1UL << 0) | /* HtTunEn */
> - (1UL << 4), /* iotblSup */
> - 1);
> -
> - /* IVHD length */
> - build_append_int_noprefix(table_data, ivhd_blob->len + 40, 2);
> -
> - /* DeviceID */
> - build_append_int_noprefix(table_data, iommu_devid, 2);
> - /* Capability offset */
> - build_append_int_noprefix(table_data, s->pci->capab_offset, 2);
> - /* IOMMU base address */
> - build_append_int_noprefix(table_data, s->mr_mmio.addr, 8);
> - /* PCI Segment Group */
> - build_append_int_noprefix(table_data, 0, 2);
> - /* IOMMU info */
> - build_append_int_noprefix(table_data, 0, 2);
> - /* IOMMU Attributes */
> - if (!s->iommu.dma_translation) {
> - build_append_int_noprefix(table_data, (1UL << 0) /* HATDis */, 4);
> - } else {
> - build_append_int_noprefix(table_data, 0, 4);
> - }
> - /* EFR Register Image */
> - build_append_int_noprefix(table_data,
> - amdvi_extended_feature_register(s),
> - 8);
> - /* EFR Register Image 2 */
> - build_append_int_noprefix(table_data, 0, 8);
> -
> + ivhd11.type = 0x11;
> + ivhd11.flags = AMD_IVHD_FLAG_HT_TUN_EN | AMD_IVHD_FLAG_IOTLB_SUP;
> + ivhd11.length = ivhd_blob->len + sizeof(ivhd11);
> + ivhd11.devid = iommu_devid;
> + ivhd11.capab_offset = s->pci->capab_offset;
> + ivhd11.base_addr = s->mr_mmio.addr;
> + ivhd11.iommu_attributes = !s->iommu.dma_translation <<
> + AMD_IVHD_ATTRIBUTES_HATDIS_SHIFT;
> + ivhd11.efr = amdvi_extended_feature_register(s);
> + g_array_append_vals(table_data, &ivhd11, sizeof(ivhd11));
> /* IVHD entries as found above */
> g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
>
> --
> 2.34.1
next prev parent reply other threads:[~2026-08-03 21:57 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-11 12:39 [PATCH 0/8] acpi_build: Refactor and cleanup AMD IVRS build Sairaj Kodilkar
2026-05-11 12:39 ` [PATCH 1/8] tests/acpi: x86: Allow IVRS acpi table changes Sairaj Kodilkar
2026-05-11 12:39 ` [PATCH 2/8] amd_iommu: update PA, GVA and VA size macros Sairaj Kodilkar
2026-05-19 8:39 ` Vasant Hegde
2026-07-30 21:41 ` Alejandro Jimenez
2026-08-03 22:15 ` Michael S. Tsirkin
2026-05-11 12:39 ` [PATCH 3/8] amd_iommu: Return empty efr for stub call Sairaj Kodilkar
2026-07-30 21:45 ` Alejandro Jimenez
2026-05-11 12:39 ` [PATCH 4/8] acpi_build: Use IOMMU pci device to build IOMMU device ID Sairaj Kodilkar
2026-07-30 22:35 ` Alejandro Jimenez
2026-07-31 9:50 ` Michael S. Tsirkin
2026-07-31 12:32 ` Sairaj Kodilkar
2026-07-31 21:47 ` Alejandro Jimenez
2026-08-01 0:01 ` Michael S. Tsirkin
2026-08-04 5:01 ` Sairaj Kodilkar
2026-05-11 12:39 ` [PATCH 5/8] acpi_build: Introduce necessary macros and structs for AMD IOMMU IVRS Sairaj Kodilkar
2026-08-03 21:35 ` Alejandro Jimenez
2026-08-03 22:08 ` Michael S. Tsirkin
2026-05-11 12:39 ` [PATCH 6/8] acpi_build: Build IVRS feature report using extended feature register Sairaj Kodilkar
2026-05-19 8:43 ` Vasant Hegde
2026-05-11 12:39 ` [PATCH 7/8] acpi_build: Cleanup AMD IOMMU IVRS building Sairaj Kodilkar
2026-08-03 21:56 ` Michael S. Tsirkin [this message]
2026-08-03 22:10 ` Michael S. Tsirkin
2026-08-04 5:23 ` Sairaj Kodilkar
2026-05-11 12:39 ` [PATCH 8/8] tests/acpi: x86: update golden masters for IVRS Sairaj Kodilkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260803175508-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=alejandro.j.jimenez@oracle.com \
--cc=anisinha@redhat.com \
--cc=eduardo@habkost.net \
--cc=imammedo@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=sarunkod@amd.com \
--cc=suravee.suthikulpanit@amd.com \
--cc=vasant.hegde@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.