[PATCH v2 0/3] VT-d minor fixes

[PATCH v2 0/3] VT-d minor fixes

[CLEMENT MATHIEU--DRIF]

From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

Various fixes for VT-d

This series contains fixes that will be necessary
when adding in-guest (fully emulated) SVM support.

v2
    Make commit author consistent



Clément Mathieu--Drif (3):
  intel_iommu: fix FRCD construction macro.
  intel_iommu: make types match
  intel_iommu: Bypass barrier wait descriptor

 hw/i386/intel_iommu.c          | 8 +++++++-
 hw/i386/intel_iommu_internal.h | 4 ++--
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH v2 1/3] intel_iommu: fix FRCD construction macro.

[CLEMENT MATHIEU--DRIF]

From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

The constant must be unsigned, otherwise the two's complement
overrides the other fields when a PASID is present

Fixes: 1b2b12376c ("intel-iommu: PASID support")

Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
---
 hw/i386/intel_iommu_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index f8cf99bddf..cbc4030031 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -267,7 +267,7 @@
 /* For the low 64-bit of 128-bit */
 #define VTD_FRCD_FI(val)        ((val) & ~0xfffULL)
 #define VTD_FRCD_PV(val)        (((val) & 0xffffULL) << 40)
-#define VTD_FRCD_PP(val)        (((val) & 0x1) << 31)
+#define VTD_FRCD_PP(val)        (((val) & 0x1ULL) << 31)
 #define VTD_FRCD_IR_IDX(val)    (((val) & 0xffffULL) << 48)
 
 /* DMA Remapping Fault Conditions */
-- 
2.45.2

[PATCH v2 2/3] intel_iommu: make types match

[CLEMENT MATHIEU--DRIF]

From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

The 'level' field in vtd_iotlb_key is an unsigned integer.
We don't need to store level as an int in vtd_lookup_iotlb.

VTDIOTLBPageInvInfo.mask is used in binary operations with addresses.

Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu.c          | 2 +-
 hw/i386/intel_iommu_internal.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 37c21a0aec..be0cb39b5c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -358,7 +358,7 @@ static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
 {
     struct vtd_iotlb_key key;
     VTDIOTLBEntry *entry;
-    int level;
+    unsigned level;
 
     for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
         key.gfn = vtd_get_iotlb_gfn(addr, level);
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index cbc4030031..5fcbe2744f 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -436,7 +436,7 @@ struct VTDIOTLBPageInvInfo {
     uint16_t domain_id;
     uint32_t pasid;
     uint64_t addr;
-    uint8_t mask;
+    uint64_t mask;
 };
 typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
 
-- 
2.45.2

[PATCH v2 3/3] intel_iommu: Bypass barrier wait descriptor

[CLEMENT MATHIEU--DRIF]

From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

wait_desc with SW=0,IF=0,FN=1 must not be considered as an
invalid descriptor as it is used to implement section 7.10 of
the VT-d spec

Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index be0cb39b5c..12ea3a9aa0 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2561,6 +2561,12 @@ 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
+         * This kind of descriptor is defined in section 7.10 of VT-d
+         * 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 v2 2/3] intel_iommu: make types match

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 03:12:48PM +0000, CLEMENT MATHIEU--DRIF wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> The 'level' field in vtd_iotlb_key is an unsigned integer.
> We don't need to store level as an int in vtd_lookup_iotlb.
> 
> VTDIOTLBPageInvInfo.mask is used in binary operations with addresses.

this last sentence is a bit opaque. is there a bug ? E.g.
can mask ever get so big it does not fit in u8?

> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> ---
>  hw/i386/intel_iommu.c          | 2 +-
>  hw/i386/intel_iommu_internal.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 37c21a0aec..be0cb39b5c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -358,7 +358,7 @@ static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
>  {
>      struct vtd_iotlb_key key;
>      VTDIOTLBEntry *entry;
> -    int level;
> +    unsigned level;
>  
>      for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
>          key.gfn = vtd_get_iotlb_gfn(addr, level);
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index cbc4030031..5fcbe2744f 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -436,7 +436,7 @@ struct VTDIOTLBPageInvInfo {
>      uint16_t domain_id;
>      uint32_t pasid;
>      uint64_t addr;
> -    uint8_t mask;
> +    uint64_t mask;
>  };
>  typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
>  
> -- 
> 2.45.2

Re: [PATCH v2 2/3] intel_iommu: make types match

[Yi Liu]

On 2024/7/5 06:13, Michael S. Tsirkin wrote:
> On Thu, Jul 04, 2024 at 03:12:48PM +0000, CLEMENT MATHIEU--DRIF wrote:
>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>
>> The 'level' field in vtd_iotlb_key is an unsigned integer.
>> We don't need to store level as an int in vtd_lookup_iotlb.
>>
>> VTDIOTLBPageInvInfo.mask is used in binary operations with addresses.
> 
> this last sentence is a bit opaque. is there a bug ? E.g.
> can mask ever get so big it does not fit in u8?

yes, this looks to be a bug. It's initialized and used by below code.
The am is a u8. So it may make more sense to split this patch. One
is to make type match, another is to fix a bug.

info.mask = ~((1 << am) - 1);

uint64_t gfn = (info->addr >> VTD_PAGE_SHIFT_4K) & info->mask;

>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu.c          | 2 +-
>>   hw/i386/intel_iommu_internal.h | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 37c21a0aec..be0cb39b5c 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -358,7 +358,7 @@ static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
>>   {
>>       struct vtd_iotlb_key key;
>>       VTDIOTLBEntry *entry;
>> -    int level;
>> +    unsigned level;
>>   
>>       for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
>>           key.gfn = vtd_get_iotlb_gfn(addr, level);
>> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
>> index cbc4030031..5fcbe2744f 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -436,7 +436,7 @@ struct VTDIOTLBPageInvInfo {
>>       uint16_t domain_id;
>>       uint32_t pasid;
>>       uint64_t addr;
>> -    uint8_t mask;
>> +    uint64_t mask;
>>   };
>>   typedef struct VTDIOTLBPageInvInfo VTDIOTLBPageInvInfo;
>>   
>> -- 
>> 2.45.2
> 

-- 
Regards,
Yi Liu

Re: [PATCH v2 3/3] intel_iommu: Bypass barrier wait descriptor

[Yi Liu]

On 2024/7/4 23:12, CLEMENT MATHIEU--DRIF wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> wait_desc with SW=0,IF=0,FN=1 must not be considered as an
> invalid descriptor as it is used to implement section 7.10 of
> the VT-d spec

After a second thinking. t would be better to move this patch to the
PRI series [1]. Reason as below:

This wait descriptor is used to drain PRQ. While, the guest need not
to drain PRQ until the PRI series which advertises the PRI cap to the
guest. So QEMU won't get such a wait descriptor before that series.

[1] 
https://lore.kernel.org/qemu-devel/713ece39-bc1e-4189-a1d9-f81f9cdbd03b@eviden.com/

> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> ---
>   hw/i386/intel_iommu.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index be0cb39b5c..12ea3a9aa0 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2561,6 +2561,12 @@ 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
> +         * This kind of descriptor is defined in section 7.10 of VT-d
> +         * 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,

-- 
Regards,
Yi Liu

Re: [PATCH v2 1/3] intel_iommu: fix FRCD construction macro.

[Yi Liu]

On 2024/7/4 23:12, CLEMENT MATHIEU--DRIF wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> The constant must be unsigned, otherwise the two's complement
> overrides the other fields when a PASID is present

I'm not native speaker. But it's better to see a "." in the end
of the sentence. :)

> 
> Fixes: 1b2b12376c ("intel-iommu: PASID support")

you need more digits per the result of "grep Fixes 
docs/devel/submitting-a-patch.rst".

docs/devel/submitting-a-patch.rst:add an additional line with "Fixes: 
<at-least-12-digits-of-SHA-commit-id>

> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> ---
>   hw/i386/intel_iommu_internal.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index f8cf99bddf..cbc4030031 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -267,7 +267,7 @@
>   /* For the low 64-bit of 128-bit */
>   #define VTD_FRCD_FI(val)        ((val) & ~0xfffULL)
>   #define VTD_FRCD_PV(val)        (((val) & 0xffffULL) << 40)
> -#define VTD_FRCD_PP(val)        (((val) & 0x1) << 31)
> +#define VTD_FRCD_PP(val)        (((val) & 0x1ULL) << 31)
>   #define VTD_FRCD_IR_IDX(val)    (((val) & 0xffffULL) << 48)
>   
>   /* DMA Remapping Fault Conditions */

-- 
Regards,
Yi Liu