Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor

[cmd]

On 03/07/2024 09:29, Yi Liu wrote:
> On 2024/7/2 23:29, CLEMENT MATHIEU--DRIF wrote:
> > On 02/07/2024 15:33, Yi Liu wrote:
> > > Caution: External email. Do not open attachments or click links,
> > > unless this email comes from a known sender and you know the content
> > > is safe.
> > >
> > >
> > > On 2024/7/2 13:52, CLEMENT MATHIEU--DRIF wrote:
> > > > From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> > > >
> > > > Signed-off-by: Clément Mathieu--Drif 
> > > > <clement.mathieu--drif@eviden.com>
> > > > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > > > ---
> > > >    hw/i386/intel_iommu.c | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index 98996ededc..71cebe2fd3 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -3500,6 +3500,11 @@ static bool
> > > > vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
> > > >        } else 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_FN) {
> > > > +        /*
> > > > +         * SW = 0, IF = 0, FN = 1
> > > > +         * Nothing to do as we process the events sequentially
> > > > +         */
> > >
> > > This code looks a bit weird. SW field does not co-exist with IF. But
> > > either
> > > SW or IF can co-exist with FN flag. Is it? Have you already seen a wait
> > > descriptor that only has FN flag set but no SW nor IF flag?
> > Yes, my test suite triggers that condition
>
> I see. Spec indeed has such usage. Please add a comment for it.
> Since it does not need a response, so QEMU can just bypass it. Also
> please adjust the subject a bit. It's misleading. Perhaps
>
> "intel_iommu: Bypass barrier wait descriptor"
Fine, will do
> Spec CH 7.10
> a. Submit Invalidation Wait Descriptor (inv_wait_dsc) with Fence flag 
> (FN=1) Set to Invalidation
> Queue. This ensures that all requests submitted to the Invalidation 
> Queue ahead of this wait
> descriptor are processed and completed by remapping hardware before 
> processing requests
> after the Invalidation Wait Descriptor. It is not required to specify 
> SW flag (or IF flag) in this
> descriptor or for software to wait on its completion, as its function 
> is to only act as a barrier.
>
> > > >        } else {
> > > >            error_report_once("%s: invalid wait desc: hi=%"PRIx64",
> > > > lo=%"PRIx64
> > > >                              " (unknown type)", __func__, 
> > > > inv_desc->hi,
> > >
> > > --
> > > Regards,
> > > Yi Liu