Re: [PATCH v2] intel_iommu: Allow both Status Write and Interrupt Flag in QI wait

[Yi Liu]

Hi David,

On 2025/7/14 16:00, David Woodhouse wrote:

From: David Woodhouse <d...@amazon.co.uk>

FreeBSD does both, and this appears to be perfectly valid. The VT-d
spec even talks about the ordering (the status write should be done
first, unsurprisingly).

interesting. Have you tried setting both flags on baremetal and the hw
gives you both the status code and an interrupt?

We certainly shouldn't assert() and abort QEMU if the guest asks for
both.

Fixes: ed7b8fbcfb88 ("intel-iommu: add supports for queued invalidation 
interface")
Closes: https://gitlab.com/qemu-project/qemu/-/issues/3028
Signed-off-by: David Woodhouse <d...@amazon.co.uk>
---
v2:
  • Only generate the interrupt once.
  • Spaces around bitwise OR.

This stops QEMU crashing, but I still can't get FreeBSD to boot and use
CPUs with APIC ID > 255 using *either* Intel or AMD IOMMU with
interrupt remapping, or the native 15-bit APIC ID enlightenment.
cf. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=288122


  hw/i386/intel_iommu.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 69d72ad35c..851c4656c5 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2822,6 +2822,7 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
  {
      uint64_t mask[4] = {VTD_INV_DESC_WAIT_RSVD_LO, VTD_INV_DESC_WAIT_RSVD_HI,
                          VTD_INV_DESC_ALL_ONE, VTD_INV_DESC_ALL_ONE};
+    bool ret = true;

if (!vtd_inv_desc_reserved_check(s, inv_desc, mask, false,

                                       __func__, "wait")) {
@@ -2833,8 +2834,6 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
          uint32_t status_data = (uint32_t)(inv_desc->lo >>
                                 VTD_INV_DESC_WAIT_DATA_SHIFT);

- assert(!(inv_desc->lo & VTD_INV_DESC_WAIT_IF));

-
          /* FIXME: need to be masked with HAW? */
          dma_addr_t status_addr = inv_desc->hi;
          trace_vtd_inv_desc_wait_sw(status_addr, status_data);
@@ -2843,18 +2842,22 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
                               &status_data, sizeof(status_data),
                               MEMTXATTRS_UNSPECIFIED)) {
              trace_vtd_inv_desc_wait_write_fail(inv_desc->hi, inv_desc->lo);
-            return false;
+            ret = false;
          }
-    } else if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) {
+    }
+
+    if (inv_desc->lo & VTD_INV_DESC_WAIT_IF) {
          /* Interrupt flag */
          vtd_generate_completion_event(s);
-    } else {
+    }
+
+    if (!(inv_desc->lo & (VTD_INV_DESC_WAIT_IF | VTD_INV_DESC_WAIT_SW))) {
          error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
                            " (unknown type)", __func__, inv_desc->hi,
                            inv_desc->lo);
          return false;
      }

I think this "if branch" can be moved just after the inv_desc non-zero
reserved bit checking. Hence you don't need a ret at all. :) btw. I'm
also asking if VT-d spec allows it or not. So let's wait for a while..

-    return true;
+    return ret;
  }

static bool vtd_process_context_cache_desc(IntelIOMMUState *s,

--
Regards,
Yi Liu