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

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

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"

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

-- 
Regards,
Yi Liu