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

[Yi Liu <yi.l.liu@intel.com>]

Hi David,

On 2025/7/14 16:00, 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).

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 <dwmw@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