Re: [PATCH 5/8] acpi_build: Introduce necessary macros and structs for AMD IOMMU IVRS

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 11, 2026 at 06:09:34PM +0530, Sairaj Kodilkar wrote:
> Introduce necessary macros, along with packed structs which represents
> the AMD IVRS data structure, to hold the necessary information. This
> will improve readability of the current code.
> 
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  hw/i386/acpi-build.h | 92 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/i386/amd_iommu.h  |  6 +++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 8ba3c33e4831..9fd60a186db1 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -2,10 +2,102 @@
>  #ifndef HW_I386_ACPI_BUILD_H
>  #define HW_I386_ACPI_BUILD_H
>  #include "hw/acpi/acpi-defs.h"
> +#include "qemu/bitops.h"
>  
>  extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>  
>  void acpi_setup(void);
>  Object *acpi_get_i386_pci_host(void);
>  
> +#define AMD_IVINFO_EFR_SUP       BIT(0)
> +
> +#define AMD_IVHD_FLAG_HT_TUN_EN    BIT(0)
> +#define AMD_IVHD_FLAG_IOTLB_SUP    BIT(4)
> +#define AMD_IVHD_FLAG_PREF_SUP     BIT(6)
> +#define AMD_IVHD_FLAG_PPR_SUP      BIT(7)
> +
> +#define AMD_IVHD_FEATURE_REPORT_XT_SUP_SHIFT      (0)
> +#define AMD_IVHD_FEATURE_REPORT_GT_SUP_SHIFT      (2)
> +#define AMD_IVHD_FEATURE_REPORT_GLX_SUP_SHIFT     (3)
> +#define AMD_IVHD_FEATURE_REPORT_GA_SUP_SHIFT      (6)
> +#define AMD_IVHD_FEATURE_REPORT_GATS_SHIFT        (28)
> +#define AMD_IVHD_FEATURE_REPORT_HATS_SHIFT        (30)
> +
> +#define AMD_IVHD_ATTRIBUTES_HATDIS_SHIFT        (0)
> +
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_RESERVED             (0)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_ALL                  (1)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_SELECT               (2)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_START_RANGE          (3)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_END_RANGE            (4)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_ALIAS_START_RANGE    (0x43)
> +#define AMD_IVHD_DEVICE_ENTRY_TYPE_SPECIAL_DEVICE       (0x48)
> +
> +#define IVHD_VARIETY_IOAPIC (1)
> +#define IVHD_VARIETY_HPET   (2)
> +
> +/*
> + * Vendor(AMD) specific fields in the IVRS header
> + * Excludes fields in ACPI table header
> + */
> +typedef
> +struct AmdIvrsVendorHdr {
> +    uint32_t ivinfo;
> +    uint64_t reserved;
> +} __attribute__((packed)) AmdIvrsVendorHdr;
> +
> +/* IVHD type 10h */
> +typedef
> +struct AmdIvhdHdr10 {
> +    uint8_t type;
> +    uint8_t flags;
> +    uint16_t length;
> +    uint16_t devid;
> +    uint16_t capab_offset;
> +    uint64_t base_addr;
> +    uint16_t pci_seg;
> +    uint16_t iommu_info;
> +    uint32_t iommu_feature_report;
> +} __attribute__((packed)) AmdIvhdHdr10;
> +
> +/* IVHD type 11h */
> +typedef
> +struct AmdIvhdHdr11 {
> +    uint8_t type;
> +    uint8_t flags;
> +    uint16_t length;
> +    uint16_t devid;
> +    uint16_t capab_offset;
> +    uint64_t base_addr;
> +    uint16_t pci_seg;
> +    uint16_t iommu_info;
> +    uint32_t iommu_attributes;
> +    uint64_t efr;
> +    uint64_t efr2;
> +} __attribute__((packed)) AmdIvhdHdr11;
> +
> +typedef
> +struct AmdIvhdDeviceEntry {
> +    uint8_t type;
> +    uint16_t devid;
> +    uint8_t dte_setting;
> +} __attribute__((packed)) AmdIvhdDeviceEntry;
> +
> +typedef
> +struct AmdIvhdDeviceEntryExt {
> +    uint8_t type;
> +    uint16_t devid_a;
> +    uint8_t dte_setting;
> +    union {
> +        struct {
> +            uint8_t handle;
> +            uint16_t devid_b;
> +            uint8_t variety;
> +        } __attribute__((packed));
> +        struct {
> +            uint32_t ext_dte_setting;
> +        } __attribute__((packed));
> +    };
> +} __attribute__((packed)) AmdIvhdDeviceEntryExt;
> +
>  #endif


Pls do not do this is not how we build ACPI tables.
It is completely impossible to match fields and structs to
spec since it does not repeat it verbatim.



Instead of all this write functions matching spec exactly.
Examples:

	/* Table 104: IVHD Device Entry Type Codes (8-byte) */
	build_append_int_noprefix(....) /* Handle */
	build_append_int_noprefix(....) /* DevIDb */
	build_append_int_noprefix(....) /* Variety */


Same with all these numbers they are used exactly once.
Instead of a macro document in a comment:

build_append_int_noprefix(.... (0x0 << 30 /* 00b = 4 levels HATS */) |
			  ....)



You must also find and document for each table the *earliest*
spec that gives the features you are using and where it is in
that spec.
Why earliest? it gives people better idea which guests it can be
compatible with.


> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index fe8f4a6cdc74..d97ddbd612dc 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -175,9 +175,15 @@
>  #define AMDVI_DTE_QUAD3_RESERVED        (GENMASK64(14, 0) | GENMASK64(53, 48))
>  
>  /* AMDVI paging mode */
> +#define AMDVI_GATS_MODE_SHIFT           (12)
> +#define AMDVI_GATS_MODE_MASK            (3ULL <<  12)
>  #define AMDVI_GATS_MODE                 (2ULL <<  12)
> +#define AMDVI_HATS_MODE_SHIFT           (10)
> +#define AMDVI_HATS_MODE_MASK            (3ULL <<  10)
>  #define AMDVI_HATS_MODE                 (2ULL <<  10)
>  #define AMDVI_HATS_MODE_RESERVED        (3ULL <<  10)
> +#define AMDVI_GLX_SUP_SHIFT             (14)
> +#define AMDVI_GLX_SUP_MASK              (3ULL << 14)
>  
>  /* Page Table format */
>  
> -- 
> 2.34.1