[PATCH v2 0/7] amd_iommu: Fixes to align with AMDVi specification

[PATCH v2 0/7] amd_iommu: Fixes to align with AMDVi specification

[Alejandro Jimenez]

Correct mistakes in bitmasks, offsets, decoding of fields, and behavior that
do not match the latest AMD I/O Virtualization Technology (IOMMU)
Specification. These bugs do not trigger problems today in the limited mode
of operation supported by the AMD vIOMMU (passthrough), but upcoming
functionality and tests will require them (and additional fixes).

These are all minor and hopefully not controversial fixes, so I am sending
them separately rather than including them on the DMA remap support
series[0].

It is unclear how relevant these changes will be to stable releases
considering the state of the AMD vIOMMU, but the fixes on this series should
be simple enough to apply, so I Cc'd stable for consideration.

Changes since v1[1]:
- Added R-b's from Vasant on PATCH 1-3, 5.
- P3: Match the spec, although vIOMMU case does not use DTE[3] (Vasant)
- P4: Fix more definitions using GENMASK64 for consistency. (Sairaj, Vasant)
- Dropped PATCH 6 from v1 and included it in DMA remap series[0] (Vasant: I
didn't want to assume approval so I dropped your R-b for this patch when
moving it to the other series).
- Fix issue with mask that retrieves the IRT pointer from DTE.
- Remove duplicated code. Although trivial, I didn't want to sneak this into
other unrelated commits.

Tested booting guest with AMD vIOMMU and guest kernel in passthrough mode.

Thank you,
Alejandro

[0] https://lore.kernel.org/qemu-devel/20250502021605.1795985-20-alejandro.j.jimenez@oracle.com/
[1] https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/

Alejandro Jimenez (7):
  amd_iommu: Fix Miscellanous Information Register 0 offsets
  amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
  amd_iommu: Update bitmasks representing DTE reserved fields
  amd_iommu: Fix masks for various IOMMU MMIO Registers
  amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
  amd_iommu: Fix the calculation for Device Table size
  amd_iommu: Remove duplicated definitions

 hw/i386/amd_iommu.c | 15 ++++++------
 hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
 2 files changed, 37 insertions(+), 37 deletions(-)


base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
-- 
2.43.5

[PATCH v2 1/7] amd_iommu: Fix Miscellanous Information Register 0 offsets

[Alejandro Jimenez]

The definitions encoding the maximum Virtual, Physical, and Guest Virtual
Address sizes supported by the IOMMU are using incorrect offsets i.e. the
VASize and GVASize offsets are switched.

Cc: qemu-stable@nongnu.org
Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 5672bdef89071..75a01eff468f5 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -196,9 +196,9 @@
 #define AMDVI_PAGE_SHIFT_4K 12
 #define AMDVI_PAGE_MASK_4K  (~((1ULL << AMDVI_PAGE_SHIFT_4K) - 1))
 
-#define AMDVI_MAX_VA_ADDR          (48UL << 5)
-#define AMDVI_MAX_PH_ADDR          (40UL << 8)
-#define AMDVI_MAX_GVA_ADDR         (48UL << 15)
+#define AMDVI_MAX_GVA_ADDR      (48UL << 5)
+#define AMDVI_MAX_PH_ADDR       (40UL << 8)
+#define AMDVI_MAX_VA_ADDR       (48UL << 15)
 
 /* Completion Wait data size */
 #define AMDVI_COMPLETION_DATA_SIZE    8
-- 
2.43.5

[PATCH v2 2/7] amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command

[Alejandro Jimenez]

The DeviceID bits are extracted using an incorrect offset in the call to
amdvi_iotlb_remove_page(). This field is read (correctly) earlier, so use
the value already retrieved for devid.

Cc: qemu-stable@nongnu.org
Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 0775c8f3bba8f..55202eb9ef7d6 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -508,7 +508,7 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
 static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
 {
 
-    uint16_t devid = extract64(cmd[0], 0, 16);
+    uint16_t devid = cpu_to_le16(extract64(cmd[0], 0, 16));
     if (extract64(cmd[1], 1, 1) || extract64(cmd[1], 3, 1) ||
         extract64(cmd[1], 6, 6)) {
         amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
@@ -521,7 +521,7 @@ static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
                                     &devid);
     } else {
         amdvi_iotlb_remove_page(s, cpu_to_le64(extract64(cmd[1], 12, 52)) << 12,
-                                cpu_to_le16(extract64(cmd[1], 0, 16)));
+                                devid);
     }
     trace_amdvi_iotlb_inval();
 }
-- 
2.43.5

[PATCH v2 3/7] amd_iommu: Update bitmasks representing DTE reserved fields

[Alejandro Jimenez]

The DTE validation method verifies that all bits in reserved DTE fields are
unset. Update them according to the latest definition available in AMD I/O
Virtualization Technology (IOMMU) Specification - Section 2.2.2.1 Device
Table Entry Format. Remove the magic numbers and use a macro helper to
generate bitmasks covering the specified ranges for better legibility.

Note that some reserved fields specify that events are generated when they
contain non-zero bits, or checks are skipped under certain configurations.
This change only updates the reserved masks, checks for special conditions
are not yet implemented.

Cc: qemu-stable@nongnu.org
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 7 ++++---
 hw/i386/amd_iommu.h | 9 ++++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 55202eb9ef7d6..cc47ba9249990 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -848,9 +848,10 @@ static inline uint64_t amdvi_get_perms(uint64_t entry)
 static bool amdvi_validate_dte(AMDVIState *s, uint16_t devid,
                                uint64_t *dte)
 {
-    if ((dte[0] & AMDVI_DTE_LOWER_QUAD_RESERVED)
-        || (dte[1] & AMDVI_DTE_MIDDLE_QUAD_RESERVED)
-        || (dte[2] & AMDVI_DTE_UPPER_QUAD_RESERVED) || dte[3]) {
+    if ((dte[0] & AMDVI_DTE_QUAD0_RESERVED) ||
+        (dte[1] & AMDVI_DTE_QUAD1_RESERVED) ||
+        (dte[2] & AMDVI_DTE_QUAD2_RESERVED) ||
+        (dte[3] & AMDVI_DTE_QUAD3_RESERVED)) {
         amdvi_log_illegaldevtab_error(s, devid,
                                       s->devtab +
                                       devid * AMDVI_DEVTAB_ENTRY_SIZE, 0);
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 75a01eff468f5..45a997af861e6 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -25,6 +25,8 @@
 #include "hw/i386/x86-iommu.h"
 #include "qom/object.h"
 
+#define GENMASK64(h, l)  (((~0ULL) >> (63 - (h) + (l))) << (l))
+
 /* Capability registers */
 #define AMDVI_CAPAB_BAR_LOW           0x04
 #define AMDVI_CAPAB_BAR_HIGH          0x08
@@ -162,9 +164,10 @@
 #define AMDVI_FEATURE_PC                  (1ULL << 9) /* Perf counters       */
 
 /* reserved DTE bits */
-#define AMDVI_DTE_LOWER_QUAD_RESERVED  0x80300000000000fc
-#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100
-#define AMDVI_DTE_UPPER_QUAD_RESERVED  0x08f0000000000000
+#define AMDVI_DTE_QUAD0_RESERVED        (GENMASK64(6, 2) | GENMASK64(63, 63))
+#define AMDVI_DTE_QUAD1_RESERVED        0
+#define AMDVI_DTE_QUAD2_RESERVED        GENMASK64(53, 52)
+#define AMDVI_DTE_QUAD3_RESERVED        (GENMASK64(14, 0) | GENMASK64(53, 48))
 
 /* AMDVI paging mode */
 #define AMDVI_GATS_MODE                 (2ULL <<  12)
-- 
2.43.5

[PATCH v2 4/7] amd_iommu: Fix masks for various IOMMU MMIO Registers

[Alejandro Jimenez]

Address various issues with definitions of the MMIO registers e.g. for the
Device Table Address Register, the size mask currently encompasses reserved
bits [11:9], so change it to only extract the bits [8:0] encoding size.

Convert masks to use GENMASK64 for consistency, and make unrelated
definitions independent.

Cc: qemu-stable@nongnu.org
Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.h | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 45a997af861e6..09352672bdcc2 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -68,34 +68,34 @@
 
 #define AMDVI_MMIO_SIZE               0x4000
 
-#define AMDVI_MMIO_DEVTAB_SIZE_MASK   ((1ULL << 12) - 1)
-#define AMDVI_MMIO_DEVTAB_BASE_MASK   (((1ULL << 52) - 1) & ~ \
-                                       AMDVI_MMIO_DEVTAB_SIZE_MASK)
+#define AMDVI_MMIO_DEVTAB_SIZE_MASK     GENMASK64(8, 0)
+#define AMDVI_MMIO_DEVTAB_BASE_MASK     GENMASK64(51, 12)
+
 #define AMDVI_MMIO_DEVTAB_ENTRY_SIZE  32
 #define AMDVI_MMIO_DEVTAB_SIZE_UNIT   4096
 
 /* some of this are similar but just for readability */
 #define AMDVI_MMIO_CMDBUF_SIZE_BYTE       (AMDVI_MMIO_COMMAND_BASE + 7)
 #define AMDVI_MMIO_CMDBUF_SIZE_MASK       0x0f
-#define AMDVI_MMIO_CMDBUF_BASE_MASK       AMDVI_MMIO_DEVTAB_BASE_MASK
-#define AMDVI_MMIO_CMDBUF_HEAD_MASK       (((1ULL << 19) - 1) & ~0x0f)
-#define AMDVI_MMIO_CMDBUF_TAIL_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
+#define AMDVI_MMIO_CMDBUF_BASE_MASK       GENMASK64(51, 12)
+#define AMDVI_MMIO_CMDBUF_HEAD_MASK       GENMASK64(18, 4)
+#define AMDVI_MMIO_CMDBUF_TAIL_MASK       GENMASK64(18, 4)
 
 #define AMDVI_MMIO_EVTLOG_SIZE_BYTE       (AMDVI_MMIO_EVENT_BASE + 7)
-#define AMDVI_MMIO_EVTLOG_SIZE_MASK       AMDVI_MMIO_CMDBUF_SIZE_MASK
-#define AMDVI_MMIO_EVTLOG_BASE_MASK       AMDVI_MMIO_CMDBUF_BASE_MASK
-#define AMDVI_MMIO_EVTLOG_HEAD_MASK       (((1ULL << 19) - 1) & ~0x0f)
-#define AMDVI_MMIO_EVTLOG_TAIL_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
+#define AMDVI_MMIO_EVTLOG_SIZE_MASK       0x0f
+#define AMDVI_MMIO_EVTLOG_BASE_MASK       GENMASK64(51, 12)
+#define AMDVI_MMIO_EVTLOG_HEAD_MASK       GENMASK64(18, 4)
+#define AMDVI_MMIO_EVTLOG_TAIL_MASK       GENMASK64(18, 4)
 
-#define AMDVI_MMIO_PPRLOG_SIZE_BYTE       (AMDVI_MMIO_EVENT_BASE + 7)
-#define AMDVI_MMIO_PPRLOG_HEAD_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
-#define AMDVI_MMIO_PPRLOG_TAIL_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
-#define AMDVI_MMIO_PPRLOG_BASE_MASK       AMDVI_MMIO_EVTLOG_BASE_MASK
-#define AMDVI_MMIO_PPRLOG_SIZE_MASK       AMDVI_MMIO_EVTLOG_SIZE_MASK
+#define AMDVI_MMIO_PPRLOG_SIZE_BYTE       (AMDVI_MMIO_PPR_BASE + 7)
+#define AMDVI_MMIO_PPRLOG_SIZE_MASK       0x0f
+#define AMDVI_MMIO_PPRLOG_BASE_MASK       GENMASK64(51, 12)
+#define AMDVI_MMIO_PPRLOG_HEAD_MASK       GENMASK64(18, 4)
+#define AMDVI_MMIO_PPRLOG_TAIL_MASK       GENMASK64(18, 4)
 
 #define AMDVI_MMIO_EXCL_ENABLED_MASK      (1ULL << 0)
 #define AMDVI_MMIO_EXCL_ALLOW_MASK        (1ULL << 1)
-#define AMDVI_MMIO_EXCL_LIMIT_MASK        AMDVI_MMIO_DEVTAB_BASE_MASK
+#define AMDVI_MMIO_EXCL_LIMIT_MASK        GENMASK64(51, 12)
 #define AMDVI_MMIO_EXCL_LIMIT_LOW         0xfff
 
 /* mmio control register flags */
@@ -132,14 +132,14 @@
 #define AMDVI_DEV_TRANSLATION_VALID       (1ULL << 1)
 #define AMDVI_DEV_MODE_MASK               0x7
 #define AMDVI_DEV_MODE_RSHIFT             9
-#define AMDVI_DEV_PT_ROOT_MASK            0xffffffffff000
+#define AMDVI_DEV_PT_ROOT_MASK            GENMASK64(51, 12)
 #define AMDVI_DEV_PT_ROOT_RSHIFT          12
 #define AMDVI_DEV_PERM_SHIFT              61
 #define AMDVI_DEV_PERM_READ               (1ULL << 61)
 #define AMDVI_DEV_PERM_WRITE              (1ULL << 62)
 
 /* Device table entry bits 64:127 */
-#define AMDVI_DEV_DOMID_ID_MASK          ((1ULL << 16) - 1)
+#define AMDVI_DEV_DOMID_ID_MASK             GENMASK64(15, 0)
 
 /* Event codes and flags, as stored in the info field */
 #define AMDVI_EVENT_ILLEGAL_DEVTAB_ENTRY  (0x1U << 12)
@@ -197,7 +197,7 @@
 #define AMDVI_PAGE_SIZE  (1ULL << AMDVI_PAGE_SHIFT)
 
 #define AMDVI_PAGE_SHIFT_4K 12
-#define AMDVI_PAGE_MASK_4K  (~((1ULL << AMDVI_PAGE_SHIFT_4K) - 1))
+#define AMDVI_PAGE_MASK_4K      GENMASK64(63, 12)
 
 #define AMDVI_MAX_GVA_ADDR      (48UL << 5)
 #define AMDVI_MAX_PH_ADDR       (40UL << 8)
-- 
2.43.5

[PATCH v2 5/7] amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE

[Alejandro Jimenez]

Fix an off-by-one error in the definition of AMDVI_IR_PHYS_ADDR_MASK. The
current definition masks off the most significant bit of the Interrupt Table
Root ptr i.e. it only generates a mask with bits [50:6] set. See the AMD I/O
Virtualization Technology (IOMMU) Specification for the Interrupt Table
Root Pointer[51:6] field in the Device Table Entry format.

Cc: qemu-stable@nongnu.org
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 09352672bdcc2..1836b7061d848 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -231,7 +231,7 @@
 #define AMDVI_IR_INTCTL_PASS            1
 #define AMDVI_IR_INTCTL_REMAP           2
 
-#define AMDVI_IR_PHYS_ADDR_MASK         (((1ULL << 45) - 1) << 6)
+#define AMDVI_IR_PHYS_ADDR_MASK         GENMASK64(51, 6)
 
 /* MSI data 10:0 bits (section 2.2.5.1 Fig 14) */
 #define AMDVI_IRTE_OFFSET               0x7ff
-- 
2.43.5

[PATCH v2 5/7] amd_iommu: Fix mask to retrive Interrupt Table Root Pointer from DTE

[Alejandro Jimenez]

Fix an off-by-one error in the definition of AMDVI_IR_PHYS_ADDR_MASK. The
current definition masks off the most significant bit of the Interrupt Table
Root ptr i.e. it only generates a mask with bits [50:6] set. See the AMD I/O
Virtualization Technology (IOMMU) Specification for the Interrupt Table
Root Pointer[51:6] field in the Device Table Entry format.

Cc: qemu-stable@nongnu.org
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 09352672bdcc2..1836b7061d848 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -231,7 +231,7 @@
 #define AMDVI_IR_INTCTL_PASS            1
 #define AMDVI_IR_INTCTL_REMAP           2
 
-#define AMDVI_IR_PHYS_ADDR_MASK         (((1ULL << 45) - 1) << 6)
+#define AMDVI_IR_PHYS_ADDR_MASK         GENMASK64(51, 6)
 
 /* MSI data 10:0 bits (section 2.2.5.1 Fig 14) */
 #define AMDVI_IRTE_OFFSET               0x7ff
-- 
2.43.5

[PATCH v2 6/7] amd_iommu: Fix the calculation for Device Table size

[Alejandro Jimenez]

Correctly calculate the Device Table size using the format encoded in the
Device Table Base Address Register (MMIO Offset 0000h).

Cc: qemu-stable@nongnu.org
Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index cc47ba9249990..6a68f0f6068a0 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -665,8 +665,8 @@ static inline void amdvi_handle_devtab_write(AMDVIState *s)
     uint64_t val = amdvi_readq(s, AMDVI_MMIO_DEVICE_TABLE);
     s->devtab = (val & AMDVI_MMIO_DEVTAB_BASE_MASK);
 
-    /* set device table length */
-    s->devtab_len = ((val & AMDVI_MMIO_DEVTAB_SIZE_MASK) + 1 *
+    /* set device table length (i.e. number of entries table can hold) */
+    s->devtab_len = (((val & AMDVI_MMIO_DEVTAB_SIZE_MASK) + 1) *
                     (AMDVI_MMIO_DEVTAB_SIZE_UNIT /
                      AMDVI_MMIO_DEVTAB_ENTRY_SIZE));
 }
-- 
2.43.5

[PATCH v2 7/7] amd_iommu: Remove duplicated definitions

[Alejandro Jimenez]

No functional change.

Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 hw/i386/amd_iommu.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 1836b7061d848..31e473924fa20 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -206,10 +206,6 @@
 /* Completion Wait data size */
 #define AMDVI_COMPLETION_DATA_SIZE    8
 
-#define AMDVI_COMMAND_SIZE   16
-/* Completion Wait data size */
-#define AMDVI_COMPLETION_DATA_SIZE    8
-
 #define AMDVI_COMMAND_SIZE   16
 
 #define AMDVI_INT_ADDR_FIRST    0xfee00000
-- 
2.43.5

Re: [PATCH v2 7/7] amd_iommu: Remove duplicated definitions

[Vasant Hegde]

On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
> No functional change.
> 
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

I had similar fix locally :-) Thanks for fixing it.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH v2 5/7] amd_iommu: Fix mask to retrive Interrupt Table Root Pointer from DTE

[Vasant Hegde]

On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
> Fix an off-by-one error in the definition of AMDVI_IR_PHYS_ADDR_MASK. The
> current definition masks off the most significant bit of the Interrupt Table
> Root ptr i.e. it only generates a mask with bits [50:6] set. See the AMD I/O
> Virtualization Technology (IOMMU) Specification for the Interrupt Table
> Root Pointer[51:6] field in the Device Table Entry format.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

I found this issue last week when I was going through the code!Thanks for fixing.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

Re: [PATCH v2 4/7] amd_iommu: Fix masks for various IOMMU MMIO Registers

[Vasant Hegde]

On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
> Address various issues with definitions of the MMIO registers e.g. for the
> Device Table Address Register, the size mask currently encompasses reserved
> bits [11:9], so change it to only extract the bits [8:0] encoding size.
> 
> Convert masks to use GENMASK64 for consistency, and make unrelated
> definitions independent.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

This patch makes it easy to read macros! Thanks.


Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>


> ---
>  hw/i386/amd_iommu.h | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index 45a997af861e6..09352672bdcc2 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -68,34 +68,34 @@
>  
>  #define AMDVI_MMIO_SIZE               0x4000
>  
> -#define AMDVI_MMIO_DEVTAB_SIZE_MASK   ((1ULL << 12) - 1)
> -#define AMDVI_MMIO_DEVTAB_BASE_MASK   (((1ULL << 52) - 1) & ~ \
> -                                       AMDVI_MMIO_DEVTAB_SIZE_MASK)
> +#define AMDVI_MMIO_DEVTAB_SIZE_MASK     GENMASK64(8, 0)
> +#define AMDVI_MMIO_DEVTAB_BASE_MASK     GENMASK64(51, 12)
> +
>  #define AMDVI_MMIO_DEVTAB_ENTRY_SIZE  32
>  #define AMDVI_MMIO_DEVTAB_SIZE_UNIT   4096
>  
>  /* some of this are similar but just for readability */
>  #define AMDVI_MMIO_CMDBUF_SIZE_BYTE       (AMDVI_MMIO_COMMAND_BASE + 7)
>  #define AMDVI_MMIO_CMDBUF_SIZE_MASK       0x0f
> -#define AMDVI_MMIO_CMDBUF_BASE_MASK       AMDVI_MMIO_DEVTAB_BASE_MASK
> -#define AMDVI_MMIO_CMDBUF_HEAD_MASK       (((1ULL << 19) - 1) & ~0x0f)
> -#define AMDVI_MMIO_CMDBUF_TAIL_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
> +#define AMDVI_MMIO_CMDBUF_BASE_MASK       GENMASK64(51, 12)
> +#define AMDVI_MMIO_CMDBUF_HEAD_MASK       GENMASK64(18, 4)
> +#define AMDVI_MMIO_CMDBUF_TAIL_MASK       GENMASK64(18, 4)
>  
>  #define AMDVI_MMIO_EVTLOG_SIZE_BYTE       (AMDVI_MMIO_EVENT_BASE + 7)
> -#define AMDVI_MMIO_EVTLOG_SIZE_MASK       AMDVI_MMIO_CMDBUF_SIZE_MASK
> -#define AMDVI_MMIO_EVTLOG_BASE_MASK       AMDVI_MMIO_CMDBUF_BASE_MASK
> -#define AMDVI_MMIO_EVTLOG_HEAD_MASK       (((1ULL << 19) - 1) & ~0x0f)
> -#define AMDVI_MMIO_EVTLOG_TAIL_MASK       AMDVI_MMIO_EVTLOG_HEAD_MASK
> +#define AMDVI_MMIO_EVTLOG_SIZE_MASK       0x0f
> +#define AMDVI_MMIO_EVTLOG_BASE_MASK       GENMASK64(51, 12)
> +#define AMDVI_MMIO_EVTLOG_HEAD_MASK       GENMASK64(18, 4)
> +#define AMDVI_MMIO_EVTLOG_TAIL_MASK       GENMASK64(18, 4)
>  
> -#define AMDVI_MMIO_PPRLOG_SIZE_BYTE       (AMDVI_MMIO_EVENT_BASE + 7)

Unrelated to this patch.. I think we should just read the AMDVI_MMIO_EVENT_BASE
register and extract the length. Same for rest of the buffer. We can do that as
improvement later.

-Vasant

Re: [PATCH v2 0/7] amd_iommu: Fixes to align with AMDVi specification

[Vasant Hegde]

Hi,

On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
> Correct mistakes in bitmasks, offsets, decoding of fields, and behavior that
> do not match the latest AMD I/O Virtualization Technology (IOMMU)
> Specification. These bugs do not trigger problems today in the limited mode
> of operation supported by the AMD vIOMMU (passthrough), but upcoming
> functionality and tests will require them (and additional fixes).
> 
> These are all minor and hopefully not controversial fixes, so I am sending
> them separately rather than including them on the DMA remap support
> series[0].

Thanks a lot Alejandro. These are very useful/important cleanup/fixes.
We have some more cleanup/fixes. We will base it on top of this series.

-Vasant



> 
> It is unclear how relevant these changes will be to stable releases
> considering the state of the AMD vIOMMU, but the fixes on this series should
> be simple enough to apply, so I Cc'd stable for consideration.
> 
> Changes since v1[1]:
> - Added R-b's from Vasant on PATCH 1-3, 5.
> - P3: Match the spec, although vIOMMU case does not use DTE[3] (Vasant)
> - P4: Fix more definitions using GENMASK64 for consistency. (Sairaj, Vasant)
> - Dropped PATCH 6 from v1 and included it in DMA remap series[0] (Vasant: I
> didn't want to assume approval so I dropped your R-b for this patch when
> moving it to the other series).
> - Fix issue with mask that retrieves the IRT pointer from DTE.
> - Remove duplicated code. Although trivial, I didn't want to sneak this into
> other unrelated commits.
> 
> Tested booting guest with AMD vIOMMU and guest kernel in passthrough mode.
> 
> Thank you,
> Alejandro
> 
> [0] https://lore.kernel.org/qemu-devel/20250502021605.1795985-20-alejandro.j.jimenez@oracle.com/
> [1] https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
> 
> Alejandro Jimenez (7):
>   amd_iommu: Fix Miscellanous Information Register 0 offsets
>   amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>   amd_iommu: Update bitmasks representing DTE reserved fields
>   amd_iommu: Fix masks for various IOMMU MMIO Registers
>   amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
>   amd_iommu: Fix the calculation for Device Table size
>   amd_iommu: Remove duplicated definitions
> 
>  hw/i386/amd_iommu.c | 15 ++++++------
>  hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
>  2 files changed, 37 insertions(+), 37 deletions(-)
> 
> 
> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e

Re: [PATCH v2 4/7] amd_iommu: Fix masks for various IOMMU MMIO Registers

[Alejandro Jimenez]

On 5/29/25 1:23 AM, Vasant Hegde wrote:
> 
> 
> On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
>> Address various issues with definitions of the MMIO registers e.g. for the
>> Device Table Address Register, the size mask currently encompasses reserved
>> bits [11:9], so change it to only extract the bits [8:0] encoding size.
>>
>> Convert masks to use GENMASK64 for consistency, and make unrelated
>> definitions independent.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> 
> This patch makes it easy to read macros! Thanks.
> 

Easier is the goal :). I was tempted to try to remove duplication since 
these registers all use similar offsets for their fields, but I figured 
it is better to keep them independent in case they diverge in future 
revisions.

> 
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> 
>> ---
>>   hw/i386/amd_iommu.h | 38 +++++++++++++++++++-------------------
>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 45a997af861e6..09352672bdcc2 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -68,34 +68,34 @@
>>   

>>   
>> -#define AMDVI_MMIO_PPRLOG_SIZE_BYTE       (AMDVI_MMIO_EVENT_BASE + 7)
> 
> Unrelated to this patch.. I think we should just read the AMDVI_MMIO_EVENT_BASE
> register and extract the length. Same for rest of the buffer. We can do that as
> improvement later.
> 

Agree. The current approach is technically a bug since it is also 
reading data from the next MMIO offset, and breaking the requirement 
from 3.4 IOMMU MMIO Registers that says:
"Accesses must be aligned to the size of the access..."

I looked into that area because I was initially confused by the 
*_SIZE_BYTE definitions (it didn't help that the one above is incorrect 
since it uses the wrong starting register).
I considered renaming them to use something like *_LEN_OFFSET instead 
since that would match the naming in the spec more closely, but decided 
to limit changes to the header. And with the correct fix as you mention 
those definitions won't be needed anyways.

Thank you,
Alejandro

> -Vasant
> 
> 

Re: [PATCH v2 0/7] amd_iommu: Fixes to align with AMDVi specification

[Alejandro Jimenez]

On 5/29/25 1:26 AM, Vasant Hegde wrote:
> Hi,
> 
> On 5/29/2025 3:47 AM, Alejandro Jimenez wrote:
>> Correct mistakes in bitmasks, offsets, decoding of fields, and behavior that
>> do not match the latest AMD I/O Virtualization Technology (IOMMU)
>> Specification. These bugs do not trigger problems today in the limited mode
>> of operation supported by the AMD vIOMMU (passthrough), but upcoming
>> functionality and tests will require them (and additional fixes).
>>
>> These are all minor and hopefully not controversial fixes, so I am sending
>> them separately rather than including them on the DMA remap support
>> series[0].
> 
> Thanks a lot Alejandro. These are very useful/important cleanup/fixes.
> We have some more cleanup/fixes. We will base it on top of this series.
> 

Great, thank you for the quick reviews! Looking forward to your changes. 
I'll be out on vacation next week, but will try to review as soon as I 
am back online.

Alejandro
> -Vasant
> 
> 
> 
>>
>> It is unclear how relevant these changes will be to stable releases
>> considering the state of the AMD vIOMMU, but the fixes on this series should
>> be simple enough to apply, so I Cc'd stable for consideration.
>>
>> Changes since v1[1]:
>> - Added R-b's from Vasant on PATCH 1-3, 5.
>> - P3: Match the spec, although vIOMMU case does not use DTE[3] (Vasant)
>> - P4: Fix more definitions using GENMASK64 for consistency. (Sairaj, Vasant)
>> - Dropped PATCH 6 from v1 and included it in DMA remap series[0] (Vasant: I
>> didn't want to assume approval so I dropped your R-b for this patch when
>> moving it to the other series).
>> - Fix issue with mask that retrieves the IRT pointer from DTE.
>> - Remove duplicated code. Although trivial, I didn't want to sneak this into
>> other unrelated commits.
>>
>> Tested booting guest with AMD vIOMMU and guest kernel in passthrough mode.
>>
>> Thank you,
>> Alejandro
>>
>> [0] https://lore.kernel.org/qemu-devel/20250502021605.1795985-20-alejandro.j.jimenez@oracle.com/
>> [1] https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
>>
>> Alejandro Jimenez (7):
>>    amd_iommu: Fix Miscellanous Information Register 0 offsets
>>    amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command
>>    amd_iommu: Update bitmasks representing DTE reserved fields
>>    amd_iommu: Fix masks for various IOMMU MMIO Registers
>>    amd_iommu: Fix mask to retrieve Interrupt Table Root Pointer from DTE
>>    amd_iommu: Fix the calculation for Device Table size
>>    amd_iommu: Remove duplicated definitions
>>
>>   hw/i386/amd_iommu.c | 15 ++++++------
>>   hw/i386/amd_iommu.h | 59 ++++++++++++++++++++++-----------------------
>>   2 files changed, 37 insertions(+), 37 deletions(-)
>>
>>
>> base-commit: 80db93b2b88f9b3ed8927ae7ac74ca30e643a83e
>