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

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

On 5/11/26 8:39 AM, 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

acpi-build.h currently doesn't have any device specific definitions, and it
is included by several files that don't need AMD IOMMU information. I
initially thought about creating a new header just for these new
definitions, but I think the simplest approach is to put them in
amd_iommu.h (which is already included in acpi-build.c).


> @@ -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;
> +

nit: I think the preferred way to specify the packed attribute is to use
QEMU_PACKED like the FwCfgTPMConfig definition uses in acpi-build.c. I know
it is ultimately the same, but I think we should use project-specific
definitions when available/appropriate. Please change all the structs to
use QEMU_PACKED.

Thank you,
Alejandro

> +/* 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
> 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 */
>