Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Philippe Mathieu-Daudé]

On 24/4/26 22:18, Junjie Cao wrote:
> Raise .impl.min_access_size from 4 to 8 in vtd_mem_ops so the memory
> subsystem always widens guest accesses to 8 bytes before calling the
> handler.  This eliminates all 25 assert(size == 4) sites that crashed
> QEMU on an 8-byte access to a 32-bit-only register.
> 
> With size always 8, the if/else branches for 64-bit register pairs
> collapse.  A zero-extended 4-byte write to the low half is safe:
> wmask protects read-only upper bits, and trigger functions re-read
> the register file and guard on their action bits.
> 
> The entry bounds check is relaxed to `addr >= DMAR_REG_SIZE` since
> the widened size no longer reflects the guest access width; the
> framework guarantees addr stays within the MemoryRegion.  Default
> branches fall back to vtd_get/set_long() when addr + 8 would exceed
> the register file.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   hw/i386/intel_iommu.c | 121 ++++++++----------------------------------
>   1 file changed, 23 insertions(+), 98 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f395fa248c..4b25907778 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3697,7 +3697,7 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>   
>       trace_vtd_reg_read(addr, size);
>   
> -    if (addr + size > DMAR_REG_SIZE) {
> +    if (addr >= DMAR_REG_SIZE) {
>           error_report_once("%s: MMIO over range: addr=0x%" PRIx64
>                             " size=0x%x", __func__, addr, size);
>           return (uint64_t)-1;
> @@ -3707,13 +3707,9 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>       /* Root Table Address Register, 64-bit */
>       case DMAR_RTADDR_REG:
>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
> -        if (size == 4) {
> -            val = val & ((1ULL << 32) - 1);
> -        }
>           break;
>   
>       case DMAR_RTADDR_REG_HI:
> -        assert(size == 4);
>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
>           break;
>   
> @@ -3722,26 +3718,21 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>           val = s->iq |
>                 (vtd_get_quad(s, DMAR_IQA_REG) &
>                 (VTD_IQA_QS | VTD_IQA_DW_MASK));
> -        if (size == 4) {
> -            val = val & ((1ULL << 32) - 1);
> -        }
>           break;
>   
>       case DMAR_IQA_REG_HI:
> -        assert(size == 4);
>           val = s->iq >> 32;
>           break;
>   
>       case DMAR_PEUADDR_REG:
> -        assert(size == 4);

Does this device support unaligned accesses? (I doubt). Otherwise
aren't all these assert(size == 4) now g_assert_not_reached()?


> @@ -4184,7 +4109,7 @@ static const MemoryRegionOps vtd_mem_ops = {
>       .write = vtd_mem_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 8,
>           .max_access_size = 8,
>       },
>       .valid = {

[PATCH v2 0/2] intel_iommu: fix guest-triggerable assert in MMIO handlers

[Junjie Cao]

An 8-byte guest access to any 32-bit-only VT-d register hits
assert(size == 4) and aborts QEMU.  Found by fuzzing with
generic-fuzz; 24 distinct crash inputs all share the same root cause.

v1: https://lore.kernel.org/all/20260420170523.17908-1-junjie.cao@intel.com/
v2: Per Philippe's suggestion, widen .impl.min_access_size to 8
instead of replacing asserts with guest-error checks.  This lets the
memory subsystem always pass size == 8 to the handler, eliminating
all 25 asserts and every size-based branch.

Junjie Cao (2):
  intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort
  tests/qtest: add 8-byte MMIO access sweep for intel-iommu

 hw/i386/intel_iommu.c          | 121 +++++++--------------------------
 tests/qtest/intel-iommu-test.c |  30 ++++++++
 2 files changed, 53 insertions(+), 98 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Junjie Cao]

Raise .impl.min_access_size from 4 to 8 in vtd_mem_ops so the memory
subsystem always widens guest accesses to 8 bytes before calling the
handler.  This eliminates all 25 assert(size == 4) sites that crashed
QEMU on an 8-byte access to a 32-bit-only register.

With size always 8, the if/else branches for 64-bit register pairs
collapse.  A zero-extended 4-byte write to the low half is safe:
wmask protects read-only upper bits, and trigger functions re-read
the register file and guard on their action bits.

The entry bounds check is relaxed to `addr >= DMAR_REG_SIZE` since
the widened size no longer reflects the guest access width; the
framework guarantees addr stays within the MemoryRegion.  Default
branches fall back to vtd_get/set_long() when addr + 8 would exceed
the register file.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 hw/i386/intel_iommu.c | 121 ++++++++----------------------------------
 1 file changed, 23 insertions(+), 98 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f395fa248c..4b25907778 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3697,7 +3697,7 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
 
     trace_vtd_reg_read(addr, size);
 
-    if (addr + size > DMAR_REG_SIZE) {
+    if (addr >= DMAR_REG_SIZE) {
         error_report_once("%s: MMIO over range: addr=0x%" PRIx64
                           " size=0x%x", __func__, addr, size);
         return (uint64_t)-1;
@@ -3707,13 +3707,9 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
     /* Root Table Address Register, 64-bit */
     case DMAR_RTADDR_REG:
         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
-        if (size == 4) {
-            val = val & ((1ULL << 32) - 1);
-        }
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
         break;
 
@@ -3722,26 +3718,21 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
         val = s->iq |
               (vtd_get_quad(s, DMAR_IQA_REG) &
               (VTD_IQA_QS | VTD_IQA_DW_MASK));
-        if (size == 4) {
-            val = val & ((1ULL << 32) - 1);
-        }
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         val = s->iq >> 32;
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
         break;
 
     default:
-        if (size == 4) {
-            val = vtd_get_long(s, addr);
-        } else {
+        if (addr + 8 <= DMAR_REG_SIZE) {
             val = vtd_get_quad(s, addr);
+        } else {
+            val = vtd_get_long(s, addr);
         }
     }
 
@@ -3755,7 +3746,7 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
 
     trace_vtd_reg_write(addr, size, val);
 
-    if (addr + size > DMAR_REG_SIZE) {
+    if (addr >= DMAR_REG_SIZE) {
         error_report_once("%s: MMIO over range: addr=0x%" PRIx64
                           " size=0x%x", __func__, addr, size);
         return;
@@ -3770,238 +3761,172 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
 
     /* Context Command Register, 64-bit */
     case DMAR_CCMD_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-            vtd_handle_ccmd_write(s);
-        }
+        vtd_set_quad(s, addr, val);
+        vtd_handle_ccmd_write(s);
         break;
 
     case DMAR_CCMD_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ccmd_write(s);
         break;
 
     /* IOTLB Invalidation Register, 64-bit */
     case DMAR_IOTLB_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-            vtd_handle_iotlb_write(s);
-        }
+        vtd_set_quad(s, addr, val);
+        vtd_handle_iotlb_write(s);
         break;
 
     case DMAR_IOTLB_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_iotlb_write(s);
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidate Address Register, 64-bit */
     case DMAR_IVA_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         break;
 
     case DMAR_IVA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Fault Status Register, 32-bit */
     case DMAR_FSTS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_fsts_write(s);
         break;
 
     /* Fault Event Control Register, 32-bit */
     case DMAR_FECTL_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_fectl_write(s);
         break;
 
     /* Fault Event Data Register, 32-bit */
     case DMAR_FEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Fault Event Address Register, 32-bit */
     case DMAR_FEADDR_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            /*
-             * While the register is 32-bit only, some guests (Xen...) write to
-             * it with 64-bit.
-             */
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         break;
 
     /* Fault Event Upper Address Register, 32-bit */
     case DMAR_FEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Protected Memory Enable Register, 32-bit */
     case DMAR_PMEN_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Root Table Address Register, 64-bit */
     case DMAR_RTADDR_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Queue Tail Register, 64-bit */
     case DMAR_IQT_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         vtd_handle_iqt_write(s);
         break;
 
     case DMAR_IQT_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* 19:63 of IQT_REG is RsvdZ, do nothing here */
         break;
 
     /* Invalidation Queue Address Register, 64-bit */
     case DMAR_IQA_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         vtd_update_iq_dw(s);
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Completion Status Register, 32-bit */
     case DMAR_ICS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ics_write(s);
         break;
 
     /* Invalidation Event Control Register, 32-bit */
     case DMAR_IECTL_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_iectl_write(s);
         break;
 
     /* Invalidation Event Data Register, 32-bit */
     case DMAR_IEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Address Register, 32-bit */
     case DMAR_IEADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Upper Address Register, 32-bit */
     case DMAR_IEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Fault Recording Registers, 128-bit */
     case DMAR_FRCD_REG_0_0:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         break;
 
     case DMAR_FRCD_REG_0_1:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     case DMAR_FRCD_REG_0_2:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-            /* May clear bit 127 (Fault), update PPF */
-            vtd_update_fsts_ppf(s);
-        }
+        vtd_set_quad(s, addr, val);
+        /* May clear bit 127 (Fault), update PPF */
+        vtd_update_fsts_ppf(s);
         break;
 
     case DMAR_FRCD_REG_0_3:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* May clear bit 127 (Fault), update PPF */
         vtd_update_fsts_ppf(s);
         break;
 
     case DMAR_IRTA_REG:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
-            vtd_set_quad(s, addr, val);
-        }
+        vtd_set_quad(s, addr, val);
         break;
 
     case DMAR_IRTA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     case DMAR_PRS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_prs_write(s);
         break;
 
     case DMAR_PECTL_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_pectl_write(s);
         break;
 
     default:
-        if (size == 4) {
-            vtd_set_long(s, addr, val);
-        } else {
+        if (addr + 8 <= DMAR_REG_SIZE) {
             vtd_set_quad(s, addr, val);
+        } else {
+            vtd_set_long(s, addr, val);
         }
     }
 }
@@ -4184,7 +4109,7 @@ static const MemoryRegionOps vtd_mem_ops = {
     .write = vtd_mem_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 4,
+        .min_access_size = 8,
         .max_access_size = 8,
     },
     .valid = {
-- 
2.43.0

[PATCH v2 2/2] tests/qtest: add 8-byte MMIO access sweep for intel-iommu

[Junjie Cao]

Sweep every 4-byte-aligned offset in the VT-d MMIO register space
with 8-byte reads and writes to verify that no register handler
aborts on an oversized access.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 tests/qtest/intel-iommu-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/qtest/intel-iommu-test.c b/tests/qtest/intel-iommu-test.c
index e5cc6acaf0..b1763ed294 100644
--- a/tests/qtest/intel-iommu-test.c
+++ b/tests/qtest/intel-iommu-test.c
@@ -17,11 +17,39 @@
 #define ECAP_STAGE_1_FIXED1   (VTD_ECAP_QI |  VTD_ECAP_IR | VTD_ECAP_IRO | \
                               VTD_ECAP_MHMV | VTD_ECAP_SMTS | VTD_ECAP_FSTS)
 
+static inline uint32_t vtd_reg_readl(QTestState *s, uint64_t offset)
+{
+    return qtest_readl(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
+}
+
 static inline uint64_t vtd_reg_readq(QTestState *s, uint64_t offset)
 {
     return qtest_readq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
 }
 
+static inline void vtd_reg_writeq(QTestState *s, uint64_t offset,
+                                  uint64_t value)
+{
+    qtest_writeq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset, value);
+}
+
+static void test_intel_iommu_8byte_access(void)
+{
+    QTestState *s;
+    uint64_t off;
+
+    s = qtest_init("-M q35 -device intel-iommu");
+
+    for (off = 0; off < DMAR_REG_SIZE; off += 4) {
+        vtd_reg_readq(s, off);
+        vtd_reg_writeq(s, off, 0);
+    }
+
+    g_assert_cmpuint(vtd_reg_readl(s, DMAR_VER_REG), !=, 0);
+
+    qtest_quit(s);
+}
+
 static void test_intel_iommu_stage_1(void)
 {
     uint8_t init_csr[DMAR_REG_SIZE];     /* register values */
@@ -58,6 +86,8 @@ static void test_intel_iommu_stage_1(void)
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/q35/intel-iommu/8byte-access",
+                   test_intel_iommu_8byte_access);
     qtest_add_func("/q35/intel-iommu/stage-1", test_intel_iommu_stage_1);
 
     return g_test_run();
-- 
2.43.0

Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Junjie Cao]

Hi Philippe,

Thanks for the review and the good question.

Looking at access_with_adjusted_size(), the _HI offsets (0x24, 0x2c, ...)
and standalone 32-bit registers like DMAR_PEUADDR_REG (0xec) are
4-byte-aligned but not 8-byte-aligned.  An 8-byte guest access to them
is indeed rejected by memory_region_access_valid (0xec & 7 != 0).  But a
4-byte access passes (0xec & 3 == 0), and the framework then widens size
to 8 while keeping the original addr -- so the handler is still reached
at these offsets.

For example, a guest 4-byte write to DMAR_PEUADDR_REG (0xec):

  1. memory_region_access_valid: size=4, 0xec & 3 == 0 -> pass
  2. access_with_adjusted_size: access_size = MAX(MIN(4,8),8) = 8
  3. write_accessor calls vtd_mem_write(opaque, 0xec, val_zext, 8)
  4. switch(addr) -> case DMAR_PEUADDR_REG -> vtd_set_long(s, addr, val)

So I believe these branches are still needed.  Please let me know if
I've missed something, or if you'd like any changes for v3.

Many thanks,
Junjie

RE: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cao, Junjie <junjie.cao@intel.com>
>Subject: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix
>MMIO abort
>
>Raise .impl.min_access_size from 4 to 8 in vtd_mem_ops so the memory
>subsystem always widens guest accesses to 8 bytes before calling the
>handler.  This eliminates all 25 assert(size == 4) sites that crashed
>QEMU on an 8-byte access to a 32-bit-only register.
>
>With size always 8, the if/else branches for 64-bit register pairs
>collapse.  A zero-extended 4-byte write to the low half is safe:
>wmask protects read-only upper bits, and trigger functions re-read
>the register file and guard on their action bits.
>
>The entry bounds check is relaxed to `addr >= DMAR_REG_SIZE` since
>the widened size no longer reflects the guest access width; the
>framework guarantees addr stays within the MemoryRegion.  Default
>branches fall back to vtd_get/set_long() when addr + 8 would exceed
>the register file.
>
>Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Signed-off-by: Junjie Cao <junjie.cao@intel.com>
>---
> hw/i386/intel_iommu.c | 121 ++++++++----------------------------------
> 1 file changed, 23 insertions(+), 98 deletions(-)
>
>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>index f395fa248c..4b25907778 100644
>--- a/hw/i386/intel_iommu.c
>+++ b/hw/i386/intel_iommu.c
>@@ -3697,7 +3697,7 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr
>addr, unsigned size)
>
>     trace_vtd_reg_read(addr, size);
>
>-    if (addr + size > DMAR_REG_SIZE) {
>+    if (addr >= DMAR_REG_SIZE) {
>         error_report_once("%s: MMIO over range: addr=0x%" PRIx64
>                           " size=0x%x", __func__, addr, size);
>         return (uint64_t)-1;
>@@ -3707,13 +3707,9 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr
>addr, unsigned size)
>     /* Root Table Address Register, 64-bit */
>     case DMAR_RTADDR_REG:
>         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
>-        if (size == 4) {
>-            val = val & ((1ULL << 32) - 1);
>-        }
>         break;
>
>     case DMAR_RTADDR_REG_HI:
>-        assert(size == 4);
>         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
>         break;
>
>@@ -3722,26 +3718,21 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr
>addr, unsigned size)
>         val = s->iq |
>               (vtd_get_quad(s, DMAR_IQA_REG) &
>               (VTD_IQA_QS | VTD_IQA_DW_MASK));
>-        if (size == 4) {
>-            val = val & ((1ULL << 32) - 1);
>-        }
>         break;
>
>     case DMAR_IQA_REG_HI:
>-        assert(size == 4);
>         val = s->iq >> 32;
>         break;
>
>     case DMAR_PEUADDR_REG:
>-        assert(size == 4);
>         val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
>         break;
>
>     default:
>-        if (size == 4) {
>-            val = vtd_get_long(s, addr);
>-        } else {
>+        if (addr + 8 <= DMAR_REG_SIZE) {
>             val = vtd_get_quad(s, addr);
>+        } else {
>+            val = vtd_get_long(s, addr);
>         }
>     }
>
>@@ -3755,7 +3746,7 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>
>     trace_vtd_reg_write(addr, size, val);
>
>-    if (addr + size > DMAR_REG_SIZE) {
>+    if (addr >= DMAR_REG_SIZE) {
>         error_report_once("%s: MMIO over range: addr=0x%" PRIx64
>                           " size=0x%x", __func__, addr, size);
>         return;
>@@ -3770,238 +3761,172 @@ static void vtd_mem_write(void *opaque, hwaddr
>addr,
>
>     /* Context Command Register, 64-bit */
>     case DMAR_CCMD_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-            vtd_handle_ccmd_write(s);
>-        }
>+        vtd_set_quad(s, addr, val);
>+        vtd_handle_ccmd_write(s);

Will this cause wrong emulation when writing DMAR_CCMD_REG with 4 bytes size?

>         break;
>
>     case DMAR_CCMD_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_ccmd_write(s);
>         break;
>
>     /* IOTLB Invalidation Register, 64-bit */
>     case DMAR_IOTLB_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-            vtd_handle_iotlb_write(s);
>-        }
>+        vtd_set_quad(s, addr, val);
>+        vtd_handle_iotlb_write(s);
>         break;
>
>     case DMAR_IOTLB_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_iotlb_write(s);
>         break;
>
>     case DMAR_PEUADDR_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Invalidate Address Register, 64-bit */
>     case DMAR_IVA_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         break;
>
>     case DMAR_IVA_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Fault Status Register, 32-bit */
>     case DMAR_FSTS_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_fsts_write(s);
>         break;
>
>     /* Fault Event Control Register, 32-bit */
>     case DMAR_FECTL_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_fectl_write(s);
>         break;
>
>     /* Fault Event Data Register, 32-bit */
>     case DMAR_FEDATA_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Fault Event Address Register, 32-bit */
>     case DMAR_FEADDR_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            /*
>-             * While the register is 32-bit only, some guests (Xen...) write to
>-             * it with 64-bit.
>-             */
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         break;
>
>     /* Fault Event Upper Address Register, 32-bit */
>     case DMAR_FEUADDR_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Protected Memory Enable Register, 32-bit */
>     case DMAR_PMEN_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Root Table Address Register, 64-bit */
>     case DMAR_RTADDR_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);

Same here, maybe also others, I didn't check all registers.

Thanks
Zhenzhong

>         break;
>
>     case DMAR_RTADDR_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Invalidation Queue Tail Register, 64-bit */
>     case DMAR_IQT_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         vtd_handle_iqt_write(s);
>         break;
>
>     case DMAR_IQT_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         /* 19:63 of IQT_REG is RsvdZ, do nothing here */
>         break;
>
>     /* Invalidation Queue Address Register, 64-bit */
>     case DMAR_IQA_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         vtd_update_iq_dw(s);
>         break;
>
>     case DMAR_IQA_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Invalidation Completion Status Register, 32-bit */
>     case DMAR_ICS_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_ics_write(s);
>         break;
>
>     /* Invalidation Event Control Register, 32-bit */
>     case DMAR_IECTL_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_iectl_write(s);
>         break;
>
>     /* Invalidation Event Data Register, 32-bit */
>     case DMAR_IEDATA_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Invalidation Event Address Register, 32-bit */
>     case DMAR_IEADDR_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Invalidation Event Upper Address Register, 32-bit */
>     case DMAR_IEUADDR_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     /* Fault Recording Registers, 128-bit */
>     case DMAR_FRCD_REG_0_0:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         break;
>
>     case DMAR_FRCD_REG_0_1:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     case DMAR_FRCD_REG_0_2:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-            /* May clear bit 127 (Fault), update PPF */
>-            vtd_update_fsts_ppf(s);
>-        }
>+        vtd_set_quad(s, addr, val);
>+        /* May clear bit 127 (Fault), update PPF */
>+        vtd_update_fsts_ppf(s);
>         break;
>
>     case DMAR_FRCD_REG_0_3:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         /* May clear bit 127 (Fault), update PPF */
>         vtd_update_fsts_ppf(s);
>         break;
>
>     case DMAR_IRTA_REG:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>-            vtd_set_quad(s, addr, val);
>-        }
>+        vtd_set_quad(s, addr, val);
>         break;
>
>     case DMAR_IRTA_REG_HI:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         break;
>
>     case DMAR_PRS_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_prs_write(s);
>         break;
>
>     case DMAR_PECTL_REG:
>-        assert(size == 4);
>         vtd_set_long(s, addr, val);
>         vtd_handle_pectl_write(s);
>         break;
>
>     default:
>-        if (size == 4) {
>-            vtd_set_long(s, addr, val);
>-        } else {
>+        if (addr + 8 <= DMAR_REG_SIZE) {
>             vtd_set_quad(s, addr, val);
>+        } else {
>+            vtd_set_long(s, addr, val);
>         }
>     }
> }
>@@ -4184,7 +4109,7 @@ static const MemoryRegionOps vtd_mem_ops = {
>     .write = vtd_mem_write,
>     .endianness = DEVICE_LITTLE_ENDIAN,
>     .impl = {
>-        .min_access_size = 4,
>+        .min_access_size = 8,
>         .max_access_size = 8,
>     },
>     .valid = {
>--
>2.43.0

Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Junjie Cao]

Hi Zhenzhong,

Thanks for the careful review.

You are right that the code path has changed.  With min_access_size=8
the handler always receives size=8, so a guest 4-byte write to a base
offset calls vtd_set_quad() with a zero-extended value instead of
vtd_set_long().  Two effects:


1. Triggers called unconditionally

CCMD_REG, IOTLB_REG, and FRCD_REG_0_2 originally guarded the
trigger on size==8.  In v2 all three are still no-ops:
vtd_handle_ccmd_write() and vtd_handle_iotlb_write() gate on
ICC/IVT (bit 63), which the zero-extended write just cleared
through the wmask.  For FRCD_REG_0_2, wmask is zero and the w1c
bit 63 does not fire (val bit 63 = 0), so the register is
unchanged.


2. Upper writable bits cleared by zero-extended vtd_set_quad()

Where the upper 32-bit wmask is non-zero, zero-extended val clears
those bits:

  Register       Offset  Upper wmask   Bits cleared
  CCMD_REG       0x28    0xe0000003    ICC, CIRG, 33:32
  IOTLB_REG      0xf8    0xb003ffff    IVT, IIRG, DID
  RTADDR_REG     0x20    0xffffffff    address 63:32
  IQA_REG        0x90    0xffffffff    address 63:32
  IVA_REG        0xf0    0xffffffff    address 63:32
  IRTA_REG       0xb8    0xffffffff    address 63:32

  (IQT_REG, FRCD_0_0, and FRCD_0_2 have upper wmask = 0 -- safe.)

This is safe because a guest must program the full register before
the value is consumed: CCMD/IOTLB fields are set before the action
bit (ICC/IVT) in the high-half write; RTADDR, IQA, and IRTA are
latched only when a GCMD enable bit (SRTP/QIE/SIRTP) fires; IVA is
read when IOTLB invalidation triggers via IVT.

However, a read-back between the low-half and high-half writes
would observe zeroed upper bits -- a real behavioral difference.


Two possible paths

(a) Keep v2 as-is, add comments documenting the safety argument
    above.

(b) Stay with min_access_size=4.  Simply remove all 25 asserts.
    21 sit at non-8-aligned offsets -- unreachable by any 8-byte
    guest access (alignment check rejects them).  The remaining 4
    at 8-aligned offsets (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8,
    PECTL 0xe0) fall through to vtd_set_long() which implicitly
    truncates the value -- safe and no semantic change.  All
    existing size branches in 64-bit register pairs are preserved.

As I see it, both approaches are correct; (a) is simpler but
changes observable semantics in an edge case, (b) is conservative
with zero semantic change.  That said, I am happy to hear any other
suggestions.

Thanks,
Junjie

RE: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix MMIO abort

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cao, Junjie <junjie.cao@intel.com>
>Subject: Re: [PATCH v2 1/2] intel_iommu: widen impl.min_access_size to 8 to fix
>MMIO abort
>
>Hi Zhenzhong,
>
>Thanks for the careful review.
>
>You are right that the code path has changed.  With min_access_size=8
>the handler always receives size=8, so a guest 4-byte write to a base
>offset calls vtd_set_quad() with a zero-extended value instead of
>vtd_set_long().  Two effects:
>
>
>1. Triggers called unconditionally
>
>CCMD_REG, IOTLB_REG, and FRCD_REG_0_2 originally guarded the
>trigger on size==8.  In v2 all three are still no-ops:
>vtd_handle_ccmd_write() and vtd_handle_iotlb_write() gate on
>ICC/IVT (bit 63), which the zero-extended write just cleared
>through the wmask.  For FRCD_REG_0_2, wmask is zero and the w1c
>bit 63 does not fire (val bit 63 = 0), so the register is
>unchanged.
>
>
>2. Upper writable bits cleared by zero-extended vtd_set_quad()
>
>Where the upper 32-bit wmask is non-zero, zero-extended val clears
>those bits:
>
>  Register       Offset  Upper wmask   Bits cleared
>  CCMD_REG       0x28    0xe0000003    ICC, CIRG, 33:32
>  IOTLB_REG      0xf8    0xb003ffff    IVT, IIRG, DID
>  RTADDR_REG     0x20    0xffffffff    address 63:32
>  IQA_REG        0x90    0xffffffff    address 63:32
>  IVA_REG        0xf0    0xffffffff    address 63:32
>  IRTA_REG       0xb8    0xffffffff    address 63:32
>
>  (IQT_REG, FRCD_0_0, and FRCD_0_2 have upper wmask = 0 -- safe.)
>
>This is safe because a guest must program the full register before
>the value is consumed: CCMD/IOTLB fields are set before the action
>bit (ICC/IVT) in the high-half write; RTADDR, IQA, and IRTA are
>latched only when a GCMD enable bit (SRTP/QIE/SIRTP) fires; IVA is
>read when IOTLB invalidation triggers via IVT.
>
>However, a read-back between the low-half and high-half writes
>would observe zeroed upper bits -- a real behavioral difference.
>
>
>Two possible paths
>
>(a) Keep v2 as-is, add comments documenting the safety argument
>    above.

This is risky, there is no guarantee that trigger bit is in upper register.

>
>(b) Stay with min_access_size=4.  Simply remove all 25 asserts.
>    21 sit at non-8-aligned offsets -- unreachable by any 8-byte
>    guest access (alignment check rejects them).  The remaining 4
>    at 8-aligned offsets (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8,
>    PECTL 0xe0) fall through to vtd_set_long() which implicitly
>    truncates the value -- safe and no semantic change.  All
>    existing size branches in 64-bit register pairs are preserved.

This makes sense. I didn't find what happen about 8bytes access on
4bytes register in VTD spec, but I think at least we can print a
warning for the remaining 4 registers, instead of silently truncate.

Thanks
Zhenzhong

>
>As I see it, both approaches are correct; (a) is simpler but
>changes observable semantics in an edge case, (b) is conservative
>with zero semantic change.  That said, I am happy to hear any other
>suggestions.
>
>Thanks,
>Junjie

[PATCH v3 0/2] intel_iommu: fix guest-triggerable assert in MMIO handlers

[Junjie Cao]

An 8-byte guest access to a 32-bit-only VT-d register hits
assert(size == 4) and aborts QEMU.  Found by generic-fuzz.

v1: https://lore.kernel.org/all/20260420170523.17908-1-junjie.cao@intel.com/
v2: https://lore.kernel.org/all/20260424201842.176953-1-junjie.cao@intel.com/

Changes in v3:
  - Drop v2's min_access_size=8 approach: per Zhenzhong, it
    silently zero-extends 4-byte guest writes, wiping upper
    wmask bits of 64-bit registers and firing triggers gated
    on size==8.
  - Keep min_access_size=4.  Remove the 25 assert(size == 4)
    sites: 21 are unreachable (non-8-aligned), the 4 reachable
    (FECTL 0x38, IECTL 0xa0, IEADDR 0xa8, PECTL 0xe0) fall
    through to vtd_set_long() and log a guest error.

Junjie Cao (2):
  intel_iommu: fix guest-triggerable abort on oversized MMIO access
  tests/qtest: add 8-byte MMIO access sweep for intel-iommu

 hw/i386/intel_iommu.c          | 41 +++++++++++++---------------------
 tests/qtest/intel-iommu-test.c | 30 +++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 25 deletions(-)


base-commit: da6c4fe60fee30dd77267764d55b38af9cb89d4b
-- 
2.43.0

[PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Junjie Cao]

An 8-byte guest access to a 32-bit-only VT-d register hit
assert(size == 4) and aborted QEMU.  Remove all 25 asserts.

21 sit at non-8-byte-aligned offsets and are rejected by
memory_region_access_valid() before reaching the handler -- dead
code, simply deleted.

The remaining 4 guard 8-byte-aligned 32-bit registers reachable
by a well-formed 8-byte access (FECTL 0x38, IECTL 0xa0, IEADDR
0xa8, PECTL 0xe0).  Writes fall through to vtd_set_long(), which
takes uint32_t and implicitly truncates, and log a guest error.
Reads fall through to the default vtd_get_quad() -- equivalent
to two 4-byte reads and therefore harmless, no warn needed.
min_access_size stays 4, so all size-based branches on 64-bit
register pairs are preserved.

Found by generic-fuzz (24 distinct crash seeds, all fixed).

Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 hw/i386/intel_iommu.c | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index f395fa248c..0fb89332f9 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3713,7 +3713,6 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
         break;
 
@@ -3728,12 +3727,10 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         val = s->iq >> 32;
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
         break;
 
@@ -3779,7 +3776,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_CCMD_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ccmd_write(s);
         break;
@@ -3795,13 +3791,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IOTLB_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_iotlb_write(s);
         break;
 
     case DMAR_PEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3815,27 +3809,27 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IVA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Fault Status Register, 32-bit */
     case DMAR_FSTS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_fsts_write(s);
         break;
 
     /* Fault Event Control Register, 32-bit */
     case DMAR_FECTL_REG:
-        assert(size == 4);
+        if (size != 4) {
+            error_report_once("%s: invalid %u-byte access to 32-bit reg "
+                              "addr=0x%" PRIx64, __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_fectl_write(s);
         break;
 
     /* Fault Event Data Register, 32-bit */
     case DMAR_FEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3854,13 +3848,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
 
     /* Fault Event Upper Address Register, 32-bit */
     case DMAR_FEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Protected Memory Enable Register, 32-bit */
     case DMAR_PMEN_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3874,7 +3866,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_RTADDR_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3889,7 +3880,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IQT_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* 19:63 of IQT_REG is RsvdZ, do nothing here */
         break;
@@ -3905,39 +3895,41 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IQA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Completion Status Register, 32-bit */
     case DMAR_ICS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_ics_write(s);
         break;
 
     /* Invalidation Event Control Register, 32-bit */
     case DMAR_IECTL_REG:
-        assert(size == 4);
+        if (size != 4) {
+            error_report_once("%s: invalid %u-byte access to 32-bit reg "
+                              "addr=0x%" PRIx64, __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_iectl_write(s);
         break;
 
     /* Invalidation Event Data Register, 32-bit */
     case DMAR_IEDATA_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Address Register, 32-bit */
     case DMAR_IEADDR_REG:
-        assert(size == 4);
+        if (size != 4) {
+            error_report_once("%s: invalid %u-byte access to 32-bit reg "
+                              "addr=0x%" PRIx64, __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         break;
 
     /* Invalidation Event Upper Address Register, 32-bit */
     case DMAR_IEUADDR_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3951,7 +3943,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_FRCD_REG_0_1:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
@@ -3966,7 +3957,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_FRCD_REG_0_3:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         /* May clear bit 127 (Fault), update PPF */
         vtd_update_fsts_ppf(s);
@@ -3981,18 +3971,19 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
         break;
 
     case DMAR_IRTA_REG_HI:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         break;
 
     case DMAR_PRS_REG:
-        assert(size == 4);
         vtd_set_long(s, addr, val);
         vtd_handle_prs_write(s);
         break;
 
     case DMAR_PECTL_REG:
-        assert(size == 4);
+        if (size != 4) {
+            error_report_once("%s: invalid %u-byte access to 32-bit reg "
+                              "addr=0x%" PRIx64, __func__, size, addr);
+        }
         vtd_set_long(s, addr, val);
         vtd_handle_pectl_write(s);
         break;
-- 
2.43.0

[PATCH v3 2/2] tests/qtest: add 8-byte MMIO access sweep for intel-iommu

[Junjie Cao]

Sweep every 4-byte-aligned offset in the VT-d MMIO register space
with 8-byte reads and writes to verify that no register handler
aborts on an oversized access.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Junjie Cao <junjie.cao@intel.com>
---
 tests/qtest/intel-iommu-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/qtest/intel-iommu-test.c b/tests/qtest/intel-iommu-test.c
index e5cc6acaf0..b1763ed294 100644
--- a/tests/qtest/intel-iommu-test.c
+++ b/tests/qtest/intel-iommu-test.c
@@ -17,11 +17,39 @@
 #define ECAP_STAGE_1_FIXED1   (VTD_ECAP_QI |  VTD_ECAP_IR | VTD_ECAP_IRO | \
                               VTD_ECAP_MHMV | VTD_ECAP_SMTS | VTD_ECAP_FSTS)
 
+static inline uint32_t vtd_reg_readl(QTestState *s, uint64_t offset)
+{
+    return qtest_readl(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
+}
+
 static inline uint64_t vtd_reg_readq(QTestState *s, uint64_t offset)
 {
     return qtest_readq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset);
 }
 
+static inline void vtd_reg_writeq(QTestState *s, uint64_t offset,
+                                  uint64_t value)
+{
+    qtest_writeq(s, Q35_HOST_BRIDGE_IOMMU_ADDR + offset, value);
+}
+
+static void test_intel_iommu_8byte_access(void)
+{
+    QTestState *s;
+    uint64_t off;
+
+    s = qtest_init("-M q35 -device intel-iommu");
+
+    for (off = 0; off < DMAR_REG_SIZE; off += 4) {
+        vtd_reg_readq(s, off);
+        vtd_reg_writeq(s, off, 0);
+    }
+
+    g_assert_cmpuint(vtd_reg_readl(s, DMAR_VER_REG), !=, 0);
+
+    qtest_quit(s);
+}
+
 static void test_intel_iommu_stage_1(void)
 {
     uint8_t init_csr[DMAR_REG_SIZE];     /* register values */
@@ -58,6 +86,8 @@ static void test_intel_iommu_stage_1(void)
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/q35/intel-iommu/8byte-access",
+                   test_intel_iommu_8byte_access);
     qtest_add_func("/q35/intel-iommu/stage-1", test_intel_iommu_stage_1);
 
     return g_test_run();
-- 
2.43.0

Re: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Yi Liu]

On 5/6/26 11:19, Junjie Cao wrote:
> An 8-byte guest access to a 32-bit-only VT-d register hit
> assert(size == 4) and aborted QEMU.  Remove all 25 asserts.
> 
> 21 sit at non-8-byte-aligned offsets and are rejected by
> memory_region_access_valid() before reaching the handler -- dead
> code, simply deleted.
> 
> The remaining 4 guard 8-byte-aligned 32-bit registers reachable
> by a well-formed 8-byte access (FECTL 0x38, IECTL 0xa0, IEADDR
> 0xa8, PECTL 0xe0). 

could you share how you identify the 4 registers among all the
registers. The offset of such registers are multiple of 8? is it? Just
curious why only 4 registers.

> Writes fall through to vtd_set_long(), which
> takes uint32_t and implicitly truncates, and log a guest error.
> Reads fall through to the default vtd_get_quad() -- equivalent
> to two 4-byte reads and therefore harmless, no warn needed.
> min_access_size stays 4, so all size-based branches on 64-bit
> register pairs are preserved.
> 
> Found by generic-fuzz (24 distinct crash seeds, all fixed).
> 
> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>   hw/i386/intel_iommu.c | 41 ++++++++++++++++-------------------------
>   1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index f395fa248c..0fb89332f9 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3713,7 +3713,6 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>           break;
>   
>       case DMAR_RTADDR_REG_HI:
> -        assert(size == 4);
>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
>           break;
>   
> @@ -3728,12 +3727,10 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>           break;
>   
>       case DMAR_IQA_REG_HI:
> -        assert(size == 4);
>           val = s->iq >> 32;
>           break;
>   
>       case DMAR_PEUADDR_REG:
> -        assert(size == 4);
>           val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
>           break;
>   
> @@ -3779,7 +3776,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_CCMD_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           vtd_handle_ccmd_write(s);
>           break;
> @@ -3795,13 +3791,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_IOTLB_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           vtd_handle_iotlb_write(s);
>           break;
>   
>       case DMAR_PEUADDR_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3815,27 +3809,27 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_IVA_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
>       /* Fault Status Register, 32-bit */
>       case DMAR_FSTS_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           vtd_handle_fsts_write(s);
>           break;
>   
>       /* Fault Event Control Register, 32-bit */
>       case DMAR_FECTL_REG:
> -        assert(size == 4);
> +        if (size != 4) {
> +            error_report_once("%s: invalid %u-byte access to 32-bit reg "
> +                              "addr=0x%" PRIx64, __func__, size, addr);
> +        }

Since such writes are harmless, I don't think it is really helpful to
have this error log. Instead, we should have a comment here to avoid
somebody break it in future.

>           vtd_set_long(s, addr, val);
>           vtd_handle_fectl_write(s);
>           break;
>   
>       /* Fault Event Data Register, 32-bit */
>       case DMAR_FEDATA_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3854,13 +3848,11 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>   
>       /* Fault Event Upper Address Register, 32-bit */
>       case DMAR_FEUADDR_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
>       /* Protected Memory Enable Register, 32-bit */
>       case DMAR_PMEN_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3874,7 +3866,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_RTADDR_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3889,7 +3880,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_IQT_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           /* 19:63 of IQT_REG is RsvdZ, do nothing here */
>           break;
> @@ -3905,39 +3895,41 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_IQA_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
>       /* Invalidation Completion Status Register, 32-bit */
>       case DMAR_ICS_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           vtd_handle_ics_write(s);
>           break;
>   
>       /* Invalidation Event Control Register, 32-bit */
>       case DMAR_IECTL_REG:
> -        assert(size == 4);
> +        if (size != 4) {
> +            error_report_once("%s: invalid %u-byte access to 32-bit reg "
> +                              "addr=0x%" PRIx64, __func__, size, addr);
> +        }
>           vtd_set_long(s, addr, val);
>           vtd_handle_iectl_write(s);
>           break;
>   
>       /* Invalidation Event Data Register, 32-bit */
>       case DMAR_IEDATA_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
>       /* Invalidation Event Address Register, 32-bit */
>       case DMAR_IEADDR_REG:
> -        assert(size == 4);
> +        if (size != 4) {
> +            error_report_once("%s: invalid %u-byte access to 32-bit reg "
> +                              "addr=0x%" PRIx64, __func__, size, addr);
> +        }
>           vtd_set_long(s, addr, val);
>           break;
>   
>       /* Invalidation Event Upper Address Register, 32-bit */
>       case DMAR_IEUADDR_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3951,7 +3943,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_FRCD_REG_0_1:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
> @@ -3966,7 +3957,6 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_FRCD_REG_0_3:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           /* May clear bit 127 (Fault), update PPF */
>           vtd_update_fsts_ppf(s);
> @@ -3981,18 +3971,19 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
>           break;
>   
>       case DMAR_IRTA_REG_HI:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           break;
>   
>       case DMAR_PRS_REG:
> -        assert(size == 4);
>           vtd_set_long(s, addr, val);
>           vtd_handle_prs_write(s);
>           break;
>   
>       case DMAR_PECTL_REG:
> -        assert(size == 4);
> +        if (size != 4) {
> +            error_report_once("%s: invalid %u-byte access to 32-bit reg "
> +                              "addr=0x%" PRIx64, __func__, size, addr);
> +        }
>           vtd_set_long(s, addr, val);
>           vtd_handle_pectl_write(s);
>           break;

RE: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized
>MMIO access
>
>On 5/6/26 11:19, Junjie Cao wrote:
>> An 8-byte guest access to a 32-bit-only VT-d register hit
>> assert(size == 4) and aborted QEMU.  Remove all 25 asserts.
>>
>> 21 sit at non-8-byte-aligned offsets and are rejected by
>> memory_region_access_valid() before reaching the handler -- dead
>> code, simply deleted.
>>
>> The remaining 4 guard 8-byte-aligned 32-bit registers reachable
>> by a well-formed 8-byte access (FECTL 0x38, IECTL 0xa0, IEADDR
>> 0xa8, PECTL 0xe0).
>
>could you share how you identify the 4 registers among all the
>registers. The offset of such registers are multiple of 8? is it? Just
>curious why only 4 registers.
>
>> Writes fall through to vtd_set_long(), which
>> takes uint32_t and implicitly truncates, and log a guest error.
>> Reads fall through to the default vtd_get_quad() -- equivalent
>> to two 4-byte reads and therefore harmless, no warn needed.
>> min_access_size stays 4, so all size-based branches on 64-bit
>> register pairs are preserved.
>>
>> Found by generic-fuzz (24 distinct crash seeds, all fixed).
>>
>> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
>> ---
>>   hw/i386/intel_iommu.c | 41 ++++++++++++++++-------------------------
>>   1 file changed, 16 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index f395fa248c..0fb89332f9 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3713,7 +3713,6 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr
>addr, unsigned size)
>>           break;
>>
>>       case DMAR_RTADDR_REG_HI:
>> -        assert(size == 4);
>>           val = vtd_get_quad_raw(s, DMAR_RTADDR_REG) >> 32;
>>           break;
>>
>> @@ -3728,12 +3727,10 @@ static uint64_t vtd_mem_read(void *opaque,
>hwaddr addr, unsigned size)
>>           break;
>>
>>       case DMAR_IQA_REG_HI:
>> -        assert(size == 4);
>>           val = s->iq >> 32;
>>           break;
>>
>>       case DMAR_PEUADDR_REG:
>> -        assert(size == 4);
>>           val = vtd_get_long_raw(s, DMAR_PEUADDR_REG);
>>           break;
>>
>> @@ -3779,7 +3776,6 @@ static void vtd_mem_write(void *opaque, hwaddr
>addr,
>>           break;
>>
>>       case DMAR_CCMD_REG_HI:
>> -        assert(size == 4);
>>           vtd_set_long(s, addr, val);
>>           vtd_handle_ccmd_write(s);
>>           break;
>> @@ -3795,13 +3791,11 @@ static void vtd_mem_write(void *opaque, hwaddr
>addr,
>>           break;
>>
>>       case DMAR_IOTLB_REG_HI:
>> -        assert(size == 4);
>>           vtd_set_long(s, addr, val);
>>           vtd_handle_iotlb_write(s);
>>           break;
>>
>>       case DMAR_PEUADDR_REG:
>> -        assert(size == 4);
>>           vtd_set_long(s, addr, val);
>>           break;
>>
>> @@ -3815,27 +3809,27 @@ static void vtd_mem_write(void *opaque, hwaddr
>addr,
>>           break;
>>
>>       case DMAR_IVA_REG_HI:
>> -        assert(size == 4);
>>           vtd_set_long(s, addr, val);
>>           break;
>>
>>       /* Fault Status Register, 32-bit */
>>       case DMAR_FSTS_REG:
>> -        assert(size == 4);
>>           vtd_set_long(s, addr, val);
>>           vtd_handle_fsts_write(s);
>>           break;
>>
>>       /* Fault Event Control Register, 32-bit */
>>       case DMAR_FECTL_REG:
>> -        assert(size == 4);
>> +        if (size != 4) {
>> +            error_report_once("%s: invalid %u-byte access to 32-bit reg "
>> +                              "addr=0x%" PRIx64, __func__, size, addr);
>> +        }
>
>Since such writes are harmless, I don't think it is really helpful to
>have this error log. Instead, we should have a comment here to avoid
>somebody break it in future.

Maybe to take it as guest error and log it:

qemu_log_mask(LOG_GUEST_ERROR,...),

then people could catch it and fix guest code?

Thanks
Zhenzhong

Re: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Yi Liu]

On 5/14/26 21:42, Junjie Cao wrote:
> Hi Yi and Zhenzhong,
> 
> Thanks both.
> 
> On Yi's question -- I enumerated the 25 v2 assert sites and checked
> offset & 7.  21 (3 reads + 18 writes) sit at non-8-aligned offsets
> and are unreachable (memory_region_access_valid() rejects 8-byte
> access there) -- just deleted.  The remaining 4, all writes at
> 8-aligned offsets, are reachable: FECTL 0x38, IECTL 0xa0,
> IEADDR 0xa8, PECTL 0xe0.
> 
> (FEADDR 0x40 is 8-aligned too but already takes 8-byte writes via
> vtd_set_quad() for Xen; GCMD 0x18 and VER 0x0 never asserted on
> size and their default/long paths already truncate.)
> 
> On the log -- Yi, fair point that the truncation is harmless from
> QEMU's side.  The one thing I'd gently float: the VT-d spec is
> silent on oversized accesses to 32-bit registers, so the guest is
> in undefined territory, and Zhenzhong's LOG_GUEST_ERROR suggestion
> is free unless -d guest_errors is passed.  If that reasoning works
> for you, I'd combine both -- keep the log (with Zhenzhong's API)
> and add the comment you asked for, so future maintainers don't
> delete the block as "harmless":
> 
>      /*
>       * 32-bit register at an 8-byte-aligned offset: a well-formed
>       * 8-byte guest access reaches this handler.  vtd_set_long()
>       * takes uint32_t and truncates the high half -- undefined
>       * per the VT-d spec but harmless here.  Flag it so
>       * -d guest_errors surfaces the guest-side bug.
>       */
>      if (size != 4) {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: invalid %u-byte access to 32-bit reg "
>                        "addr=0x%" PRIx64 "\n", __func__, size, addr);
>      }
>      vtd_set_long(s, addr, val);
>      vtd_handle_*_write(s);
> 
> Happy either way -- if you'd still rather drop the log, I'll do
> that in v4.

LGTM. Please go ahead. :)

Regards,
Yi Liu

Re: [PATCH v3 1/2] intel_iommu: fix guest-triggerable abort on oversized MMIO access

[Junjie Cao]

Hi Yi and Zhenzhong,

Thanks both.

On Yi's question -- I enumerated the 25 v2 assert sites and checked
offset & 7.  21 (3 reads + 18 writes) sit at non-8-aligned offsets
and are unreachable (memory_region_access_valid() rejects 8-byte
access there) -- just deleted.  The remaining 4, all writes at
8-aligned offsets, are reachable: FECTL 0x38, IECTL 0xa0,
IEADDR 0xa8, PECTL 0xe0.

(FEADDR 0x40 is 8-aligned too but already takes 8-byte writes via
vtd_set_quad() for Xen; GCMD 0x18 and VER 0x0 never asserted on
size and their default/long paths already truncate.)

On the log -- Yi, fair point that the truncation is harmless from
QEMU's side.  The one thing I'd gently float: the VT-d spec is
silent on oversized accesses to 32-bit registers, so the guest is
in undefined territory, and Zhenzhong's LOG_GUEST_ERROR suggestion
is free unless -d guest_errors is passed.  If that reasoning works
for you, I'd combine both -- keep the log (with Zhenzhong's API)
and add the comment you asked for, so future maintainers don't
delete the block as "harmless":

    /*
     * 32-bit register at an 8-byte-aligned offset: a well-formed
     * 8-byte guest access reaches this handler.  vtd_set_long()
     * takes uint32_t and truncates the high half -- undefined
     * per the VT-d spec but harmless here.  Flag it so
     * -d guest_errors surfaces the guest-side bug.
     */
    if (size != 4) {
        qemu_log_mask(LOG_GUEST_ERROR,
                      "%s: invalid %u-byte access to 32-bit reg "
                      "addr=0x%" PRIx64 "\n", __func__, size, addr);
    }
    vtd_set_long(s, addr, val);
    vtd_handle_*_write(s);

Happy either way -- if you'd still rather drop the log, I'll do
that in v4.

Thanks,
Junjie