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

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

[Alejandro Jimenez]

The main reason for sending this new revision so soon is that v2 included a
duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
removing the old patch. Apologies for the mistake.

Additional changes in v3:
- Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
- Added 'Fixes:' tag to [PATCH 5/7].
- Added Vasant's R-b to patches 4,5,7.

Thank you,
Alejandro

v2:
https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/

v1:
https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/


Alejandro Jimenez (7):
  amd_iommu: Fix Miscellaneous 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 v3 1/7] amd_iommu: Fix Miscellaneous 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 v3 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 v3 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 v3 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>
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)
-#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 v3 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
Fixes: b44159fe0078 ("x86_iommu/amd: Add interrupt remap support when VAPIC is not enabled")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.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 v3 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 v3 7/7] amd_iommu: Remove duplicated definitions

[Alejandro Jimenez]

No functional change.

Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Vasant Hegde <vasant.hegde@amd.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 v3 0/7] amd_iommu: Fixes to align with AMDVi specification

[Ethan MILON]

Hi,

Is this series the right place to include the following minor fix?

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 0775c..18d30e1 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
uint64_t val)
 {
     uint64_t romask = ldq_le_p(&s->romask[addr]);
     uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
-    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
+    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
     stq_le_p(&s->mmior[addr],
             ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
 }

This corrects the type of oldval to match the return type of ldq_le_p().

Thanks,
Ethan

On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> The main reason for sending this new revision so soon is that v2 included a
> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
> removing the old patch. Apologies for the mistake.
> 
> Additional changes in v3:
> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
> - Added 'Fixes:' tag to [PATCH 5/7].
> - Added Vasant's R-b to patches 4,5,7.
> 
> Thank you,
> Alejandro
> 
> v2:
> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/
> 
> v1:
> https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
> 
> 
> Alejandro Jimenez (7):
>   amd_iommu: Fix Miscellaneous 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
> 
> 

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

[Alejandro Jimenez]

Hi Ethan,

On 6/12/25 4:36 AM, Ethan MILON wrote:
> Hi,
> 
> Is this series the right place to include the following minor fix?
> 

I would defer this change for two reasons:

1) This series has been reviewed and tested already. I was hoping it 
would be included on the Jun 1st pull but I sent v3 too late for that. I 
think it is ready so I would like to leave it as is unless there are any 
objections ...


> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 0775c..18d30e1 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
> uint64_t val)
>   {
>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>       stq_le_p(&s->mmior[addr],
>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>   }
> 
> This corrects the type of oldval to match the return type of ldq_le_p().
> 

2) This fix is needed, but it is likely better as part of additional 
changes that are needed to cleanup/fix the XTSup support. i.e. there are 
unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and those 
depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of oldval that 
you found is causing XTEn and IntCapXTEn bits on the control registers 
to be ignored, but ultimately things are not broken enough (yet). In 
other words, I think there is a lot more work to do in here, and it is 
something I am looking into.

I suspect Vasant might have spotted this problem already, so he might 
even have some fixes queued up...

That being said, if you want to send a patch with your S-b I'll add it 
to this series.

Alejandro

> Thanks,
> Ethan
> 
> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>>
>>
>> The main reason for sending this new revision so soon is that v2 included a
>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>> removing the old patch. Apologies for the mistake.
>>
>> Additional changes in v3:
>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/Miscellaneous/).
>> - Added 'Fixes:' tag to [PATCH 5/7].
>> - Added Vasant's R-b to patches 4,5,7.
>>
>> Thank you,
>> Alejandro
>>
>> v2:
>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1-alejandro.j.jimenez@oracle.com/
>>
>> v1:
>> https://lore.kernel.org/all/20250311152446.45086-1-alejandro.j.jimenez@oracle.com/
>>
>>
>> Alejandro Jimenez (7):
>>    amd_iommu: Fix Miscellaneous 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
>>
>>

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

[Philippe Mathieu-Daudé]

Hi Alejandro,

On 12/6/25 22:59, Alejandro Jimenez wrote:
> Hi Ethan,
> 
> On 6/12/25 4:36 AM, Ethan MILON wrote:
>> Hi,
>>
>> Is this series the right place to include the following minor fix?
>>
> 
> I would defer this change for two reasons:
> 
> 1) This series has been reviewed and tested already. I was hoping it 
> would be included on the Jun 1st pull but I sent v3 too late for that. I 
> think it is ready so I would like to leave it as is unless there are any 
> objections ...
> 
> 
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 0775c..18d30e1 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
>> uint64_t val)
>>   {
>>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
>> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>>       stq_le_p(&s->mmior[addr],
>>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>>   }
>>
>> This corrects the type of oldval to match the return type of ldq_le_p().
>>
> 
> 2) This fix is needed, but it is likely better as part of additional 
> changes that are needed to cleanup/fix the XTSup support. i.e. there are 
> unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and those 
> depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of oldval that 
> you found is causing XTEn and IntCapXTEn bits on the control registers 
> to be ignored, but ultimately things are not broken enough (yet).

I agree with Ethan it is better to avoid hidden truncation, because it
just makes debugging experience harder.

If this is the expected behavior, better add a comment, or use
extract64() which makes the truncation explicit.

Regards,

Phil.

> In 
> other words, I think there is a lot more work to do in here, and it is 
> something I am looking into.
> 
> I suspect Vasant might have spotted this problem already, so he might 
> even have some fixes queued up...
> 
> That being said, if you want to send a patch with your S-b I'll add it 
> to this series.
> 
> Alejandro
> 
>> Thanks,
>> Ethan
>>
>> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>>> Caution: External email. Do not open attachments or click links, 
>>> unless this email comes from a known sender and you know the content 
>>> is safe.
>>>
>>>
>>> The main reason for sending this new revision so soon is that v2 
>>> included a
>>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>>> removing the old patch. Apologies for the mistake.
>>>
>>> Additional changes in v3:
>>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/ 
>>> Miscellaneous/).
>>> - Added 'Fixes:' tag to [PATCH 5/7].
>>> - Added Vasant's R-b to patches 4,5,7.
>>>
>>> Thank you,
>>> Alejandro
>>>
>>> v2:
>>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1- 
>>> alejandro.j.jimenez@oracle.com/
>>>
>>> v1:
>>> https://lore.kernel.org/all/20250311152446.45086-1- 
>>> alejandro.j.jimenez@oracle.com/
>>>
>>>
>>> Alejandro Jimenez (7):
>>>    amd_iommu: Fix Miscellaneous 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
>>>
>>>
> 
> 

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

[Alejandro Jimenez]

Hi Phil,

On 6/16/25 2:59 AM, Philippe Mathieu-Daudé wrote:
> Hi Alejandro,
> 
> On 12/6/25 22:59, Alejandro Jimenez wrote:
>> Hi Ethan,
>>
>> On 6/12/25 4:36 AM, Ethan MILON wrote:
>>> Hi,
>>>
>>> Is this series the right place to include the following minor fix?
>>>
>>
>> I would defer this change for two reasons:
>>
>> 1) This series has been reviewed and tested already. I was hoping it 
>> would be included on the Jun 1st pull but I sent v3 too late for that. 
>> I think it is ready so I would like to leave it as is unless there are 
>> any objections ...
>>
>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index 0775c..18d30e1 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -140,7 +140,7 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr,
>>> uint64_t val)
>>>   {
>>>       uint64_t romask = ldq_le_p(&s->romask[addr]);
>>>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>>> -    uint32_t oldval = ldq_le_p(&s->mmior[addr]);
>>> +    uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>>>       stq_le_p(&s->mmior[addr],
>>>               ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>>>   }
>>>
>>> This corrects the type of oldval to match the return type of ldq_le_p().
>>>
>>
>> 2) This fix is needed, but it is likely better as part of additional 
>> changes that are needed to cleanup/fix the XTSup support. i.e. there 
>> are unhandled writes to the 0x170, 0x178, and 0x180 MMIO offsets, and 
>> those depend on MMIO 0x18[IntCapXTEn]=1. I think the truncation of 
>> oldval that you found is causing XTEn and IntCapXTEn bits on the 
>> control registers to be ignored, but ultimately things are not broken 
>> enough (yet).
> 
> I agree with Ethan it is better to avoid hidden truncation, because it
> just makes debugging experience harder.
> 

I agree that Ethan found a bug that must be fixed. I am answering his 
initial question of whether this series is the right place to fix it by 
pointing out that this bug uncovers that there is more to do than just 
fixing this specific error, and it could be included in a series to 
address those larger problems that I mentioned above.

On the other hand, it is probably better just to fix this specific bug 
now since it is simple enough, which is why I asked Ethan to send a 
commit and I will add it (he should get credit)

> If this is the expected behavior, better add a comment, or use
> extract64() which makes the truncation explicit.
> 

It is not the expected behavior, the truncation is a bug. It doesn't yet 
cause any issues because amdvi_writeq() is currently only called to 
handle MMIO writes for a few offsets/register, mostly to 
AMDVI_MMIO_CONTROL, and the romask for the offset is 0. This means that 
the bug doesn't really change the value that is ultimately written to 
the emulated MMIO register, but it could cause problems in the future.

Thank you,
Alejandro


> Regards,
> 
> Phil.
> 
>> In other words, I think there is a lot more work to do in here, and it 
>> is something I am looking into.
>>
>> I suspect Vasant might have spotted this problem already, so he might 
>> even have some fixes queued up...
>>
>> That being said, if you want to send a patch with your S-b I'll add it 
>> to this series.
>>
>> Alejandro
>>
>>> Thanks,
>>> Ethan
>>>
>>> On 5/29/25 9:30 PM, Alejandro Jimenez wrote:
>>>> Caution: External email. Do not open attachments or click links, 
>>>> unless this email comes from a known sender and you know the content 
>>>> is safe.
>>>>
>>>>
>>>> The main reason for sending this new revision so soon is that v2 
>>>> included a
>>>> duplicated [PATCH 5/7]. I fixed a typo in the commit subject and missed
>>>> removing the old patch. Apologies for the mistake.
>>>>
>>>> Additional changes in v3:
>>>> - Fixed typo on [PATCH 1/7] subject line (s/Miscellanous/ 
>>>> Miscellaneous/).
>>>> - Added 'Fixes:' tag to [PATCH 5/7].
>>>> - Added Vasant's R-b to patches 4,5,7.
>>>>
>>>> Thank you,
>>>> Alejandro
>>>>
>>>> v2:
>>>> https://lore.kernel.org/qemu-devel/20250528221725.3554040-1- 
>>>> alejandro.j.jimenez@oracle.com/
>>>>
>>>> v1:
>>>> https://lore.kernel.org/all/20250311152446.45086-1- 
>>>> alejandro.j.jimenez@oracle.com/
>>>>
>>>>
>>>> Alejandro Jimenez (7):
>>>>    amd_iommu: Fix Miscellaneous 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
>>>>
>>>>
>>
>>
>