1. Patchew
  2. QEMU
  3. View series

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

David Woodhouse posted 1 patch 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/e15012b9776a25cbfdbcc9797595669d3ae4ef36.camel@infradead.org
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Yi Liu <yi.l.liu@intel.com>, "Clément Mathieu--Drif" <clement.mathieu--drif@eviden.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
hw/i386/intel_iommu.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
[PATCH] intel_iommu: Allow both Status Write and Interrupt Flag in QI wait
Posted by David Woodhouse 5 months ago 
From: David Woodhouse <dwmw@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).

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 <dwmw@amazon.co.uk>
---
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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 69d72ad35c..3bfaf9bd07 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,7 +2834,8 @@ 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));
+        if (inv_desc->lo & VTD_INV_DESC_WAIT_IF)
+            vtd_generate_completion_event(s);
 
         /* FIXME: need to be masked with HAW? */
         dma_addr_t status_addr = inv_desc->hi;
@@ -2843,18 +2845,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;
     }
-    return true;
+    return ret;
 }
 
 static bool vtd_process_context_cache_desc(IntelIOMMUState *s,
-- 
2.43.0
Re: [PATCH] intel_iommu: Allow both Status Write and Interrupt Flag in QI wait
Posted by Michael S. Tsirkin 5 months ago 
On Fri, Jul 11, 2025 at 10:47:47AM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@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).
> 
> 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 <dwmw@amazon.co.uk>
> ---
> 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 | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 69d72ad35c..3bfaf9bd07 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,7 +2834,8 @@ 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));
> +        if (inv_desc->lo & VTD_INV_DESC_WAIT_IF)
> +            vtd_generate_completion_event(s);
>  
>          /* FIXME: need to be masked with HAW? */
>          dma_addr_t status_addr = inv_desc->hi;

Follow QEMU coding style, please.



> @@ -2843,18 +2845,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;
>      }
> -    return true;
> +    return ret;
>  }
>  
>  static bool vtd_process_context_cache_desc(IntelIOMMUState *s,
> -- 
> 2.43.0
> 
>
Re: [PATCH] intel_iommu: Allow both Status Write and Interrupt Flag in QI wait
Posted by David Woodhouse 5 months ago 
On Sun, 2025-07-13 at 17:29 -0400, Michael S. Tsirkin wrote:
> 
> > +        if (inv_desc->lo & VTD_INV_DESC_WAIT_IF)
> > +            vtd_generate_completion_event(s);
> >   
> >           /* FIXME: need to be masked with HAW? */
> >           dma_addr_t status_addr = inv_desc->hi;
> 
> Follow QEMU coding style, please.

Hm, actually that one doesn't need to be there at all, since it's done
a few lines later (*after* the status write). I'll send v2; thanks.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.