[PATCH 0/5] amd_iommu fixes staged for 11.1

[PATCH 0/5] amd_iommu fixes staged for 11.1

[Alejandro Jimenez]

Hi Michael,

I have staged these recently reviewed AMD vIOMMU fixes for 11.1:

https://github.com/aljimenezb/qemu/commits/amdvi-next

Base: 30e8a06b64aa ("Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging")
Tip:  018c1dfb3e8c ("amd_iommu: Fix endianness handling for command buffer entries")

Patch 1 is an independent fix previously posted at:
https://lore.kernel.org/qemu-devel/20260616174620.438468-1-alejandro.j.jimenez@oracle.com/

Patches 2-5 address a Coverity finding and big-endian portability
issues. They were reviewed in:
https://lore.kernel.org/qemu-devel/20260624195925.1254462-1-alejandro.j.jimenez@oracle.com/

I did basic smoke testing on an AMD Zen4 host using the AMD vIOMMU device.
Completed several guest boot and reboot cycles with xtsup both enabled and
disabled. No regressions were observed in guest startup or reboot behavior.

There are two other outstanding series pending review, but I am unlikely to
complete review/testing by myself for those before the upcoming soft-freeze
deadline.

Thank you,
Alejandro

Alejandro Jimenez (4):
  amd_iommu: Return int from page walk status helpers
  amd_iommu: Decode XT interrupt control register without bitfields
  amd_iommu: Decode IRTEs without bitfields
  amd_iommu: Fix endianness handling for command buffer entries

David Hoppenbrouwers (1):
  amd_iommu: Fix opcode reported in invalid command handling

 hw/i386/amd_iommu.c | 168 +++++++++++++++++++++++++++++++-------------
 hw/i386/amd_iommu.h |  63 -----------------
 2 files changed, 120 insertions(+), 111 deletions(-)


base-commit: 30e8a06b64aa58a3990ba39cb5d09531e7d265e0
-- 
2.47.3

[PATCH 1/5] amd_iommu: Fix opcode reported in invalid command handling

[Alejandro Jimenez]

From: David Hoppenbrouwers <qemu@demindiro.com>

According to the AMD I/O Virtualization Technology (IOMMU) Specification
(Rev 3.10), Section 2.4 Commands, the Generic Command Buffer Entry Format
encodes the opcode in bits [63:60] of the command buffer.

When handling illegal opcodes, the traces for unhandled commands and event
log info extract the opcode from an incorrect offset in the command buffer.
Fix this issue to avoid potential confusion with mismatched opcodes in
traces and unlikely errors in guest event processing.

Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: David Hoppenbrouwers <qemu@demindiro.com>
Reviewed-by: Sairaj Kodilkar <sarunkod@amd.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.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 79216fb305..05b7c638f4 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1509,9 +1509,9 @@ static void amdvi_cmdbuf_exec(AMDVIState *s)
         amdvi_inval_all(s, cmd);
         break;
     default:
-        trace_amdvi_unhandled_command(extract64(cmd[1], 60, 4));
+        trace_amdvi_unhandled_command(extract64(cmd[0], 60, 4));
         /* log illegal command */
-        amdvi_log_illegalcom_error(s, extract64(cmd[1], 60, 4),
+        amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
                                    s->cmdbuf + s->cmdbuf_head);
     }
 }
-- 
2.47.3

[PATCH 2/5] amd_iommu: Return int from page walk status helpers

[Alejandro Jimenez]

fetch_pte() returns a status code 0 on success, and (small) negative values
on failure. The PTE value itself is returned via an output parameter.
amdvi_get_top_pt_level_and_perms() follows the same return convention.

Both functions currently return uint64_t, which means any negative error
values are returned as unsigned and then converted back to int by the
callers. This does not cause any issues in the current implementation, but
Coverity flags the type mismatch and potential overflow.

Make both helpers return int, so the type matches what the return variable
is (0 on success, small negative value on failure), and also the type used
by all callers to store their return values.

No functional changes are intended.

Fixes: a1c97c395729 ("amd_iommu: Sync shadow page tables on page invalidation")
Fixes: 786550e2d38a ("amd_iommu: Follow root pointer before page walk and use 1-based levels")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 05b7c638f4..a0835a20d7 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -659,7 +659,7 @@ static uint64_t large_pte_page_size(uint64_t pte)
  *   - IOVA exceeds the address width supported by DTE[Mode]
  * In all such cases a page walk must be aborted.
  */
-static uint64_t amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
+static int amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
                                                  uint8_t *top_level,
                                                  IOMMUAccessFlags *dte_perms)
 {
@@ -702,7 +702,7 @@ static uint64_t amdvi_get_top_pt_level_and_perms(hwaddr address, uint64_t dte,
  *      page table walk. This means that the DTE has valid data, but one of the
  *      lower level entries in the Page Table could not be read.
  */
-static uint64_t fetch_pte(AMDVIAddressSpace *as, hwaddr address, uint64_t dte,
+static int fetch_pte(AMDVIAddressSpace *as, hwaddr address, uint64_t dte,
                           uint64_t *pte, hwaddr *page_size)
 {
     uint64_t pte_addr;
-- 
2.47.3

[PATCH 3/5] amd_iommu: Decode XT interrupt control register without bitfields

[Alejandro Jimenez]

The XT IOMMU General Interrupt Control Register is a guest-visible MMIO
register. Decoding it with bitfields depends on host bitfield layout and is
not portable to big-endian hosts.

Fix this by removing union mmio_xt_intr and explicitly extracting fields
with FIELD_EX64() from the full register value returned by amdvi_readq(),
which has already been converted to host endianness.

Using a designated initializer for X86IOMMUIrq also ensures fields not
provided by the XT register (e.g. msi_addr_last_bits) are initialized before
x86_iommu_irq_to_msi_message() uses them.

CID: 1660056
Fixes: cf0210df65aa ("amd_iommu: Generate XT interrupts when xt support is enabled")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/amd_iommu.c | 28 ++++++++++++++++++----------
 hw/i386/amd_iommu.h | 14 --------------
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index a0835a20d7..291cb368a9 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -34,6 +34,7 @@
 #include "hw/core/qdev-properties.h"
 #include "kvm/kvm_i386.h"
 #include "qemu/iova-tree.h"
+#include "hw/core/registerfields.h"
 
 struct AMDVIAddressSpace {
     PCIBus *bus;                /* PCIBus (for bus number)              */
@@ -88,6 +89,13 @@ typedef struct AMDVIIOTLBKey {
     uint16_t devid;
 } AMDVIIOTLBKey;
 
+/* XT IOMMU General Interrupt Control Register layout */
+FIELD(AMDVI_XT_GEN_INTR, DEST_MODE, 2, 1)
+FIELD(AMDVI_XT_GEN_INTR, DEST_LO, 8, 24)
+FIELD(AMDVI_XT_GEN_INTR, VECTOR, 32, 8)
+FIELD(AMDVI_XT_GEN_INTR, DELIVERY_MODE, 40, 1)
+FIELD(AMDVI_XT_GEN_INTR, DEST_HI, 56, 8)
+
 uint64_t amdvi_extended_feature_register(AMDVIState *s)
 {
     uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
@@ -194,17 +202,17 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
 
 static void amdvi_build_xt_msi_msg(AMDVIState *s, MSIMessage *msg)
 {
-    union mmio_xt_intr xt_reg;
-    struct X86IOMMUIrq irq;
-
-    xt_reg.val = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
+    uint64_t xt_reg = amdvi_readq(s, AMDVI_MMIO_XT_GEN_INTR);
 
-    irq.vector = xt_reg.vector;
-    irq.delivery_mode = xt_reg.delivery_mode;
-    irq.dest_mode = xt_reg.destination_mode;
-    irq.dest = (xt_reg.destination_hi << 24) | xt_reg.destination_lo;
-    irq.trigger_mode = 0;
-    irq.redir_hint = 0;
+    X86IOMMUIrq irq = {
+        .vector = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, VECTOR),
+        .delivery_mode = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DELIVERY_MODE),
+        .dest_mode = FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_MODE),
+        .dest = (FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_HI) << 24) |
+                 FIELD_EX64(xt_reg, AMDVI_XT_GEN_INTR, DEST_LO),
+        .trigger_mode = 0,
+        .redir_hint = 0,
+    };
 
     x86_iommu_irq_to_msi_message(&irq, msg);
 }
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index 3cab04a6d4..ca4440a4c1 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -340,20 +340,6 @@ struct irte_ga {
   union irte_ga_hi hi;
 };
 
-union mmio_xt_intr {
-    uint64_t val;
-    struct {
-        uint64_t rsvd_1:2,
-                 destination_mode:1,
-                 rsvd_2:5,
-                 destination_lo:24,
-                 vector:8,
-                 delivery_mode:1,
-                 rsvd_3:15,
-                 destination_hi:8;
-    };
-};
-
 #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
 OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
 
-- 
2.47.3

[PATCH 4/5] amd_iommu: Decode IRTEs without bitfields

[Alejandro Jimenez]

Interrupt remapping table entries are data stored in guest memory in
little-endian format. Decoding them with bitfields depends on host bitfield
layout and the value returned from dma_memory_read() is not portable to
big-endian hosts.

Replace the legacy and GA IRTE bitfield definitions with explicit FIELD()
definitions. Convert the guest memory values returned from dma_memory_read()
with le32_to_cpu() or le64_to_cpu(), then extract relevant fields using
FIELD_EX32() or FIELD_EX64() as appropriate to match the IRTE format.

Fixes: b44159fe0078 ("x86_iommu/amd: Add interrupt remap support when VAPIC is not enabled")
Fixes: 135f866e609c ("x86_iommu/amd: Add interrupt remap support when VAPIC is enabled")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/amd_iommu.c | 93 +++++++++++++++++++++++++++++++++------------
 hw/i386/amd_iommu.h | 49 ------------------------
 2 files changed, 69 insertions(+), 73 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 291cb368a9..c7bf21b762 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -89,6 +89,11 @@ typedef struct AMDVIIOTLBKey {
     uint16_t devid;
 } AMDVIIOTLBKey;
 
+typedef struct AMDVIIrteGA {
+    uint64_t ga_lo;
+    uint64_t ga_hi;
+} AMDVIIrteGA;
+
 /* XT IOMMU General Interrupt Control Register layout */
 FIELD(AMDVI_XT_GEN_INTR, DEST_MODE, 2, 1)
 FIELD(AMDVI_XT_GEN_INTR, DEST_LO, 8, 24)
@@ -96,6 +101,37 @@ FIELD(AMDVI_XT_GEN_INTR, VECTOR, 32, 8)
 FIELD(AMDVI_XT_GEN_INTR, DELIVERY_MODE, 40, 1)
 FIELD(AMDVI_XT_GEN_INTR, DEST_HI, 56, 8)
 
+/* Interrupt Remapping Table Fields Formats */
+
+/* Basic 32-bit IRTE layout (GAEn=0) */
+FIELD(AMDVI_IRTE, VALID, 0, 1)
+FIELD(AMDVI_IRTE, SUP_IOPF, 1, 1)
+FIELD(AMDVI_IRTE, INT_TYPE, 2, 3)
+FIELD(AMDVI_IRTE, RQ_EOI, 5, 1)
+FIELD(AMDVI_IRTE, DM, 6, 1)
+FIELD(AMDVI_IRTE, GUEST_MODE, 7, 1)
+FIELD(AMDVI_IRTE, DESTINATION, 8, 8)
+FIELD(AMDVI_IRTE, VECTOR, 16, 8)
+
+/* 128-bit IRTE layout (GAEn=1) */
+FIELD(AMDVI_IRTE_GA_LO, VALID, 0, 1)
+FIELD(AMDVI_IRTE_GA_LO, SUP_IOPF, 1, 1)
+FIELD(AMDVI_IRTE_GA_LO, INT_TYPE, 2, 3)
+FIELD(AMDVI_IRTE_GA_LO, RQ_EOI, 5, 1)
+FIELD(AMDVI_IRTE_GA_LO, DM, 6, 1)
+FIELD(AMDVI_IRTE_GA_LO, GUEST_MODE, 7, 1)
+/*
+ * In the 128-bit IRTE format, XT mode uses IRTE_GA_LOW.Destination[23:0]
+ * together with IRTE_GA_HI.DestinationHi[7:0] to construct a 32-bit x2APIC
+ * destination.
+ * Without XTEn (i.e. when x2APIC support is not enabled), only
+ * IRTE_GA_LOW.Destination[7:0] is used.
+ */
+FIELD(AMDVI_IRTE_GA_LO, DESTINATION, 8, 24)
+
+FIELD(AMDVI_IRTE_GA_HI, VECTOR, 0, 8)
+FIELD(AMDVI_IRTE_GA_HI, DESTINATION_HI, 56, 8)
+
 uint64_t amdvi_extended_feature_register(AMDVIState *s)
 {
     uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
@@ -1983,7 +2019,7 @@ static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
 }
 
 static int amdvi_get_irte(AMDVIState *s, MSIMessage *origin, uint64_t *dte,
-                          union irte *irte, uint16_t devid)
+                          uint32_t *irte, uint16_t devid)
 {
     uint64_t irte_root, offset;
 
@@ -1998,7 +2034,8 @@ static int amdvi_get_irte(AMDVIState *s, MSIMessage *origin, uint64_t *dte,
         return -AMDVI_IR_GET_IRTE;
     }
 
-    trace_amdvi_ir_irte_val(irte->val);
+    *irte = le32_to_cpu(*irte);
+    trace_amdvi_ir_irte_val(*irte);
 
     return 0;
 }
@@ -2010,8 +2047,9 @@ static int amdvi_int_remap_legacy(AMDVIState *iommu,
                                   X86IOMMUIrq *irq,
                                   uint16_t sid)
 {
+    uint8_t int_type;
+    uint32_t irte;
     int ret;
-    union irte irte;
 
     /* get interrupt remapping table */
     ret = amdvi_get_irte(iommu, origin, dte, &irte, sid);
@@ -2019,32 +2057,33 @@ static int amdvi_int_remap_legacy(AMDVIState *iommu,
         return ret;
     }
 
-    if (!irte.fields.valid) {
+    if (!FIELD_EX32(irte, AMDVI_IRTE, VALID)) {
         trace_amdvi_ir_target_abort("RemapEn is disabled");
         return -AMDVI_IR_TARGET_ABORT;
     }
 
-    if (irte.fields.guest_mode) {
+    if (FIELD_EX32(irte, AMDVI_IRTE, GUEST_MODE)) {
         error_report_once("guest mode is not zero");
         return -AMDVI_IR_ERR;
     }
 
-    if (irte.fields.int_type > AMDVI_IOAPIC_INT_TYPE_ARBITRATED) {
+    int_type = FIELD_EX32(irte, AMDVI_IRTE, INT_TYPE);
+    if (int_type > AMDVI_IOAPIC_INT_TYPE_ARBITRATED) {
         error_report_once("reserved int_type");
         return -AMDVI_IR_ERR;
     }
 
-    irq->delivery_mode = irte.fields.int_type;
-    irq->vector = irte.fields.vector;
-    irq->dest_mode = irte.fields.dm;
-    irq->redir_hint = irte.fields.rq_eoi;
-    irq->dest = irte.fields.destination;
+    irq->delivery_mode = int_type;
+    irq->vector = FIELD_EX32(irte, AMDVI_IRTE, VECTOR);
+    irq->dest_mode = FIELD_EX32(irte, AMDVI_IRTE, DM);
+    irq->redir_hint = FIELD_EX32(irte, AMDVI_IRTE, RQ_EOI);
+    irq->dest = FIELD_EX32(irte, AMDVI_IRTE, DESTINATION);
 
     return 0;
 }
 
 static int amdvi_get_irte_ga(AMDVIState *s, MSIMessage *origin, uint64_t *dte,
-                             struct irte_ga *irte, uint16_t devid)
+                             AMDVIIrteGA *irte, uint16_t devid)
 {
     uint64_t irte_root, offset;
 
@@ -2058,7 +2097,9 @@ static int amdvi_get_irte_ga(AMDVIState *s, MSIMessage *origin, uint64_t *dte,
         return -AMDVI_IR_GET_IRTE;
     }
 
-    trace_amdvi_ir_irte_ga_val(irte->hi.val, irte->lo.val);
+    irte->ga_lo = le64_to_cpu(irte->ga_lo);
+    irte->ga_hi = le64_to_cpu(irte->ga_hi);
+    trace_amdvi_ir_irte_ga_val(irte->ga_hi, irte->ga_lo);
     return 0;
 }
 
@@ -2069,8 +2110,9 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
                               X86IOMMUIrq *irq,
                               uint16_t sid)
 {
+    AMDVIIrteGA irte;
+    uint8_t int_type;
     int ret;
-    struct irte_ga irte;
 
     /* get interrupt remapping table */
     ret = amdvi_get_irte_ga(iommu, origin, dte, &irte, sid);
@@ -2078,30 +2120,33 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
         return ret;
     }
 
-    if (!irte.lo.fields_remap.valid) {
+    if (!FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, VALID)) {
         trace_amdvi_ir_target_abort("RemapEn is disabled");
         return -AMDVI_IR_TARGET_ABORT;
     }
 
-    if (irte.lo.fields_remap.guest_mode) {
+    if (FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, GUEST_MODE)) {
         error_report_once("guest mode is not zero");
         return -AMDVI_IR_ERR;
     }
 
-    if (irte.lo.fields_remap.int_type > AMDVI_IOAPIC_INT_TYPE_ARBITRATED) {
+    int_type = FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, INT_TYPE);
+    if (int_type > AMDVI_IOAPIC_INT_TYPE_ARBITRATED) {
         error_report_once("reserved int_type is set");
         return -AMDVI_IR_ERR;
     }
 
-    irq->delivery_mode = irte.lo.fields_remap.int_type;
-    irq->vector = irte.hi.fields.vector;
-    irq->dest_mode = irte.lo.fields_remap.dm;
-    irq->redir_hint = irte.lo.fields_remap.rq_eoi;
+    irq->delivery_mode = int_type;
+    irq->vector = FIELD_EX64(irte.ga_hi, AMDVI_IRTE_GA_HI, VECTOR);
+    irq->dest_mode = FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, DM);
+    irq->redir_hint = FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, RQ_EOI);
     if (iommu->xten) {
-        irq->dest = irte.lo.fields_remap.destination |
-                    (irte.hi.fields.destination_hi << 24);
+        irq->dest = FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, DESTINATION) |
+                    (FIELD_EX64(irte.ga_hi, AMDVI_IRTE_GA_HI, DESTINATION_HI)
+                    << 24);
     } else {
-        irq->dest = irte.lo.fields_remap.destination & 0xff;
+        irq->dest = FIELD_EX64(irte.ga_lo, AMDVI_IRTE_GA_LO, DESTINATION) &
+                    0xff;
     }
 
     return 0;
diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
index ca4440a4c1..687691ec1c 100644
--- a/hw/i386/amd_iommu.h
+++ b/hw/i386/amd_iommu.h
@@ -291,55 +291,6 @@
 #define AMDVI_DEV_LINT0_PASS_MASK       (1ULL << 62)
 #define AMDVI_DEV_LINT1_PASS_MASK       (1ULL << 63)
 
-/* Interrupt remapping table fields (Guest VAPIC not enabled) */
-union irte {
-    uint32_t val;
-    struct {
-        uint32_t valid:1,
-                 no_fault:1,
-                 int_type:3,
-                 rq_eoi:1,
-                 dm:1,
-                 guest_mode:1,
-                 destination:8,
-                 vector:8,
-                 rsvd:8;
-    } fields;
-};
-
-/* Interrupt remapping table fields (Guest VAPIC is enabled) */
-union irte_ga_lo {
-  uint64_t val;
-
-  /* For int remapping */
-  struct {
-      uint64_t  valid:1,
-                no_fault:1,
-                /* ------ */
-                int_type:3,
-                rq_eoi:1,
-                dm:1,
-                /* ------ */
-                guest_mode:1,
-                destination:24,
-                rsvd_1:32;
-  } fields_remap;
-};
-
-union irte_ga_hi {
-  uint64_t val;
-  struct {
-      uint64_t  vector:8,
-                rsvd_2:48,
-                destination_hi:8;
-  } fields;
-};
-
-struct irte_ga {
-  union irte_ga_lo lo;
-  union irte_ga_hi hi;
-};
-
 #define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
 OBJECT_DECLARE_SIMPLE_TYPE(AMDVIState, AMD_IOMMU_DEVICE)
 
-- 
2.47.3

[PATCH 5/5] amd_iommu: Fix endianness handling for command buffer entries

[Alejandro Jimenez]

AMD IOMMU command buffer entries are stored in guest memory in little-endian
format. Convert command buffer with le64_to_cpu() after dma_memory_read(),
so that command handlers can all operate using host native endianness.

Remove the cpu_to_le*() conversions from command handlers, since the values
are used internally by device emulation and do not need translation.
Conversion is only necessary when reading or writing to guest memory e.g.
writing completion-wait data and event log entries.

The flow for command buffer handling is:

- Retrieve command buffer (cmd[]) from guest memory (via dma_memory_read())
- Convert command buffer to host endianness (via le64_to_cpu())
- All handlers decode fields from cmd[] in host-endian format
- All emulation code uses decoded values in host-endian format
- Use cpu_to_le*() when writing back data to guest memory

Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/i386/amd_iommu.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index c7bf21b762..90252c52af 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -278,6 +278,7 @@ static uint32_t get_next_eventlog_entry(AMDVIState *s)
 
 static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
 {
+    uint64_t le_evt[2];
     uint32_t evtlog_tail_next;
 
     /* event logging not enabled */
@@ -298,8 +299,14 @@ static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
         return;
     }
 
+    /*
+     * Convert event buffer to little-endian before writing it to guest memory.
+     */
+    le_evt[0] = cpu_to_le64(evt[0]);
+    le_evt[1] = cpu_to_le64(evt[1]);
+
     if (dma_memory_write(&address_space_memory, s->evtlog + s->evtlog_tail,
-                         evt, AMDVI_EVENT_LEN, MEMTXATTRS_UNSPECIFIED)) {
+                         le_evt, AMDVI_EVENT_LEN, MEMTXATTRS_UNSPECIFIED)) {
         trace_amdvi_evntlog_fail(s->evtlog, s->evtlog_tail);
     }
 
@@ -545,15 +552,18 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
 static void amdvi_completion_wait(AMDVIState *s, uint64_t *cmd)
 {
     /* pad the last 3 bits */
-    hwaddr addr = cpu_to_le64(extract64(cmd[0], 3, 49)) << 3;
-    uint64_t data = cpu_to_le64(cmd[1]);
+    hwaddr addr = extract64(cmd[0], 3, 49) << 3;
+    uint64_t data = cmd[1];
+
+    /* Format the data to be written to guest memory as little-endian */
+    uint64_t le_data = cpu_to_le64(data);
 
     if (extract64(cmd[0], 52, 8)) {
         amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
                                    s->cmdbuf + s->cmdbuf_head);
     }
     if (extract64(cmd[0], 0, 1)) {
-        if (dma_memory_write(&address_space_memory, addr, &data,
+        if (dma_memory_write(&address_space_memory, addr, &le_data,
                              AMDVI_COMPLETION_DATA_SIZE,
                              MEMTXATTRS_UNSPECIFIED)) {
             trace_amdvi_completion_wait_fail(addr);
@@ -1281,7 +1291,7 @@ static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
 /* log error without aborting since linux seems to be using reserved bits */
 static void amdvi_inval_devtab_entry(AMDVIState *s, uint64_t *cmd)
 {
-    uint16_t devid = cpu_to_le16((uint16_t)extract64(cmd[0], 0, 16));
+    uint16_t devid = extract64(cmd[0], 0, 16);
 
     trace_amdvi_devtab_inval(PCI_BUS_NUM(devid), PCI_SLOT(devid),
                              PCI_FUNC(devid));
@@ -1448,9 +1458,9 @@ static void amdvi_sync_domain(AMDVIState *s, uint16_t domid, uint64_t addr,
 /* we don't have devid - we can't remove pages by address */
 static void amdvi_inval_pages(AMDVIState *s, uint64_t *cmd)
 {
-    uint16_t domid = cpu_to_le16((uint16_t)extract64(cmd[0], 32, 16));
-    uint64_t addr = cpu_to_le64(extract64(cmd[1], 12, 52)) << 12;
-    uint16_t flags = cpu_to_le16((uint16_t)extract64(cmd[1], 0, 3));
+    uint16_t domid = extract64(cmd[0], 32, 16);
+    uint64_t addr = extract64(cmd[1], 12, 52) << 12;
+    uint16_t flags = extract64(cmd[1], 0, 3);
 
     if (extract64(cmd[0], 20, 12) || extract64(cmd[0], 48, 12) ||
         extract64(cmd[1], 3, 9)) {
@@ -1497,7 +1507,7 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
 static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
 {
 
-    uint16_t devid = cpu_to_le16(extract64(cmd[0], 0, 16));
+    uint16_t devid = 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),
@@ -1509,7 +1519,7 @@ static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
         g_hash_table_foreach_remove(s->iotlb, amdvi_iotlb_remove_by_devid,
                                     &devid);
     } else {
-        amdvi_iotlb_remove_page(s, cpu_to_le64(extract64(cmd[1], 12, 52)) << 12,
+        amdvi_iotlb_remove_page(s, extract64(cmd[1], 12, 52) << 12,
                                 devid);
     }
     trace_amdvi_iotlb_inval();
@@ -1527,6 +1537,15 @@ static void amdvi_cmdbuf_exec(AMDVIState *s)
         return;
     }
 
+    /*
+     * Commands in guest memory are little-endian. Convert once after reading
+     * so that command handlers can decode values in host native endianness.
+     * Convert back to little-endian only when writing data to guest memory via
+     * dma_memory_write().
+     */
+    cmd[0] = le64_to_cpu(cmd[0]);
+    cmd[1] = le64_to_cpu(cmd[1]);
+
     switch (extract64(cmd[0], 60, 4)) {
     case AMDVI_CMD_COMPLETION_WAIT:
         amdvi_completion_wait(s, cmd);
-- 
2.47.3

Re: [PATCH 4/5] amd_iommu: Decode IRTEs without bitfields

[Philippe Mathieu-Daudé]

On 1/7/26 00:08, Alejandro Jimenez wrote:
> Interrupt remapping table entries are data stored in guest memory in
> little-endian format. Decoding them with bitfields depends on host bitfield
> layout and the value returned from dma_memory_read() is not portable to
> big-endian hosts.
> 
> Replace the legacy and GA IRTE bitfield definitions with explicit FIELD()
> definitions. Convert the guest memory values returned from dma_memory_read()
> with le32_to_cpu() or le64_to_cpu(), then extract relevant fields using
> FIELD_EX32() or FIELD_EX64() as appropriate to match the IRTE format.
> 
> Fixes: b44159fe0078 ("x86_iommu/amd: Add interrupt remap support when VAPIC is not enabled")
> Fixes: 135f866e609c ("x86_iommu/amd: Add interrupt remap support when VAPIC is enabled")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/i386/amd_iommu.c | 93 +++++++++++++++++++++++++++++++++------------
>   hw/i386/amd_iommu.h | 49 ------------------------
>   2 files changed, 69 insertions(+), 73 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Re: [PATCH 5/5] amd_iommu: Fix endianness handling for command buffer entries

[Philippe Mathieu-Daudé]

On 1/7/26 00:08, Alejandro Jimenez wrote:
> AMD IOMMU command buffer entries are stored in guest memory in little-endian
> format. Convert command buffer with le64_to_cpu() after dma_memory_read(),
> so that command handlers can all operate using host native endianness.
> 
> Remove the cpu_to_le*() conversions from command handlers, since the values
> are used internally by device emulation and do not need translation.
> Conversion is only necessary when reading or writing to guest memory e.g.
> writing completion-wait data and event log entries.
> 
> The flow for command buffer handling is:
> 
> - Retrieve command buffer (cmd[]) from guest memory (via dma_memory_read())
> - Convert command buffer to host endianness (via le64_to_cpu())
> - All handlers decode fields from cmd[] in host-endian format
> - All emulation code uses decoded values in host-endian format
> - Use cpu_to_le*() when writing back data to guest memory
> 
> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/i386/amd_iommu.c | 39 +++++++++++++++++++++++++++++----------
>   1 file changed, 29 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Re: [PATCH 2/5] amd_iommu: Return int from page walk status helpers

[Philippe Mathieu-Daudé]

On 1/7/26 00:08, Alejandro Jimenez wrote:
> fetch_pte() returns a status code 0 on success, and (small) negative values
> on failure. The PTE value itself is returned via an output parameter.
> amdvi_get_top_pt_level_and_perms() follows the same return convention.
> 
> Both functions currently return uint64_t, which means any negative error
> values are returned as unsigned and then converted back to int by the
> callers. This does not cause any issues in the current implementation, but
> Coverity flags the type mismatch and potential overflow.
> 
> Make both helpers return int, so the type matches what the return variable
> is (0 on success, small negative value on failure), and also the type used
> by all callers to store their return values.
> 
> No functional changes are intended.
> 
> Fixes: a1c97c395729 ("amd_iommu: Sync shadow page tables on page invalidation")
> Fixes: 786550e2d38a ("amd_iommu: Follow root pointer before page walk and use 1-based levels")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/i386/amd_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Re: [PATCH 0/5] amd_iommu fixes staged for 11.1

[Michael S. Tsirkin]

On Tue, Jun 30, 2026 at 10:08:01PM +0000, Alejandro Jimenez wrote:
> Hi Michael,
> 
> I have staged these recently reviewed AMD vIOMMU fixes for 11.1:
> 
> https://github.com/aljimenezb/qemu/commits/amdvi-next
> Base: 30e8a06b64aa ("Merge tag 'net-pull-request' of https://github.com/jasowang/qemu into staging")
> Tip:  018c1dfb3e8c ("amd_iommu: Fix endianness handling for command buffer entries")
> 
> Patch 1 is an independent fix previously posted at:
> https://lore.kernel.org/qemu-devel/20260616174620.438468-1-alejandro.j.jimenez@oracle.com/
> 
> Patches 2-5 address a Coverity finding and big-endian portability
> issues. They were reviewed in:
> https://lore.kernel.org/qemu-devel/20260624195925.1254462-1-alejandro.j.jimenez@oracle.com/

Great, I'm applying these. Thanks!


> 
> I did basic smoke testing on an AMD Zen4 host using the AMD vIOMMU device.
> Completed several guest boot and reboot cycles with xtsup both enabled and
> disabled. No regressions were observed in guest startup or reboot behavior.
> 
> There are two other outstanding series pending review, but I am unlikely to
> complete review/testing by myself for those before the upcoming soft-freeze
> deadline.
> 
> Thank you,
> Alejandro


To be frank, these are bugfixes and such changes are okay after soft freeze. Thanks!

> Alejandro Jimenez (4):
>   amd_iommu: Return int from page walk status helpers
>   amd_iommu: Decode XT interrupt control register without bitfields
>   amd_iommu: Decode IRTEs without bitfields
>   amd_iommu: Fix endianness handling for command buffer entries
> 
> David Hoppenbrouwers (1):
>   amd_iommu: Fix opcode reported in invalid command handling
> 
>  hw/i386/amd_iommu.c | 168 +++++++++++++++++++++++++++++++-------------
>  hw/i386/amd_iommu.h |  63 -----------------
>  2 files changed, 120 insertions(+), 111 deletions(-)
> 
> 
> base-commit: 30e8a06b64aa58a3990ba39cb5d09531e7d265e0
> -- 
> 2.47.3