1. Patchew
  2. QEMU
  3. [PATCH ats_vtd v5 00/22] ATS support for VT-d
  4. View patch

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

CLEMENT MATHIEU--DRIF posted 22 patches 2 months, 2 weeks ago
[PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by CLEMENT MATHIEU--DRIF 2 months, 2 weeks ago 
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
+         */
     } else {
         error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
                           " (unknown type)", __func__, inv_desc->hi,
-- 
2.45.2
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by Yi Liu 2 months, 2 weeks ago 
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?

>       } else {
>           error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
>                             " (unknown type)", __func__, inv_desc->hi,

-- 
Regards,
Yi Liu
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by CLEMENT MATHIEU--DRIF 2 months, 2 weeks ago 
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
>
>>       } else {
>>           error_report_once("%s: invalid wait desc: hi=%"PRIx64", 
>> lo=%"PRIx64
>>                             " (unknown type)", __func__, inv_desc->hi,
>
> -- 
> Regards,
> Yi Liu
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by Yi Liu 2 months, 2 weeks ago 
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
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by CLEMENT MATHIEU--DRIF 2 months, 2 weeks ago 
On 03/07/2024 09:29, 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 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"
good idea, 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
>
> -- 
> Regards,
> Yi Liu
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by cmd 2 months, 2 weeks ago 
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
>
Re: [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor
Posted by cmd 2 months, 2 weeks ago 
On 02/07/2024 17: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 think it comes from the kernel function intel_drain_pasid_prq 
(https://elixir.bootlin.com/linux/latest/source/drivers/iommu/intel/svm.c#L467)
>>>        } else {
>>>            error_report_once("%s: invalid wait desc: hi=%"PRIx64",
>>> lo=%"PRIx64
>>>                              " (unknown type)", __func__, inv_desc->hi,
>> -- 
>> Regards,
>> Yi Liu

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.