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

[PATCH v3 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.

v3
    FRCD construction macro :
    	- Longer sha1 for the 'Fixes' tag
    	- Add '.' at the end of the sentence
    
    Make types match :
    	- Split into 2 patches (one for the fix and one for type matching)
    
    Remove patch for wait descriptor handling (will be in the PRI series)

v2
    Make commit author consistent



Clément Mathieu--Drif (3):
  intel_iommu: fix FRCD construction macro.
  intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo
  intel_iommu: make types match

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

-- 
2.45.2

[PATCH v3 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: 1b2b12376c8a ("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

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

[Duan, Zhenzhong]

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: [PATCH v3 1/3] intel_iommu: fix FRCD construction macro.
>
>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: 1b2b12376c8a ("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)

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

VTD_FRCD_PV and VTD_FRCD_PP are MACROs for high 64-bit.
By this chance, maybe we can move them under:

/* For the high 64-bit of 128-bit */

Thanks
Zhenzhong

> #define VTD_FRCD_IR_IDX(val)    (((val) & 0xffffULL) << 48)
>
> /* DMA Remapping Fault Conditions */
>--
>2.45.2

[PATCH v3 2/3] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[CLEMENT MATHIEU--DRIF]

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

VTDIOTLBPageInvInfo.mask might not fit in an uint8_t.
Moreover, this field is used in binary operations with 64-bit addresses.

Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.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 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 v3 2/3] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[Duan, Zhenzhong]

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: [PATCH v3 2/3] intel_iommu: fix type of the mask field in
>VTDIOTLBPageInvInfo
>
>From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>
>VTDIOTLBPageInvInfo.mask might not fit in an uint8_t.
>Moreover, this field is used in binary operations with 64-bit addresses.
>
>Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> 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 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 v3 2/3] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 05:03:17AM +0000, CLEMENT MATHIEU--DRIF wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> VTDIOTLBPageInvInfo.mask might not fit in an uint8_t.

I think what you mean is that is assigned values that might not
fit .... it's u8 ATM so of course it fits.

> Moreover, this field is used in binary operations with 64-bit addresses.

So what?

> 
> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.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 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 v3 2/3] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[CLEMENT MATHIEU--DRIF]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]



On 05/07/2024 10:51, Michael S. Tsirkin 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 Fri, Jul 05, 2024 at 05:03:17AM +0000, CLEMENT MATHIEU--DRIF wrote:


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

VTDIOTLBPageInvInfo.mask might not fit in an uint8_t.



I think what you mean is that is assigned values that might not
fit .... it's u8 ATM so of course it fits.

What about :
"The mask stored into VTDIOTLBPageInvInfo.mask might not fit in an uint8_t. Use uint64_t to avoid overflows"





Moreover, this field is used in binary operations with 64-bit addresses.



So what?

I thing the first part of the message is enough, the issue comes from the fact that the mask does not fit into the type






Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com><mailto:clement.mathieu--drif@eviden.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 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






[-- Attachment #2: Type: text/html, Size: 2740 bytes --]

Re: [PATCH v3 2/3] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 09:52:48AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 05/07/2024 10:51, Michael S. Tsirkin 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 Fri, Jul 05, 2024 at 05:03:17AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
>         From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
>         VTDIOTLBPageInvInfo.mask might not fit in an uint8_t.
> 
>     I think what you mean is that is assigned values that might not
>     fit .... it's u8 ATM so of course it fits.
> 
> What about :
> "The mask stored into VTDIOTLBPageInvInfo.mask might not fit in an uint8_t. Use
> uint64_t to avoid overflows"

No, the mask stored there is u8.
You mean "that we are trying to store into ".

> 
> 
>         Moreover, this field is used in binary operations with 64-bit addresses.
> 
>     So what?
> 
> I thing the first part of the message is enough, the issue comes from the fact
> that the mask does not fit into the type
> 
> 
> 
>         Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.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 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 v3 3/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.

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

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);
-- 
2.45.2

RE: [PATCH v3 3/3] intel_iommu: make types match

[Duan, Zhenzhong]

>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Subject: [PATCH v3 3/3] intel_iommu: make types match
>
>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.
>
>Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>---
> hw/i386/intel_iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>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;

Will it bring any issue if int is used?

>
>     for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
>         key.gfn = vtd_get_iotlb_gfn(addr, level);
>--
>2.45.2

Re: [PATCH v3 3/3] intel_iommu: make types match

[CLEMENT MATHIEU--DRIF]

On 05/07/2024 10:51, Duan, Zhenzhong 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.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Subject: [PATCH v3 3/3] intel_iommu: make types match
>>
>> 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.
>>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> ---
>> hw/i386/intel_iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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;
> Will it bring any issue if int is used?
It shouldn't, but it might trigger static analyzer warnings.
Do you want me to drop the patch?
>
>>      for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
>>          key.gfn = vtd_get_iotlb_gfn(addr, level);
>> --
>> 2.45.2

Re: [PATCH v3 3/3] intel_iommu: make types match

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 09:24:50AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 05/07/2024 10:51, Duan, Zhenzhong 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.
> >
> >
> >> -----Original Message-----
> >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
> >> Subject: [PATCH v3 3/3] intel_iommu: make types match
> >>
> >> 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.
> >>
> >> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >> ---
> >> hw/i386/intel_iommu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> 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;
> > Will it bring any issue if int is used?
> It shouldn't, but it might trigger static analyzer warnings.
> Do you want me to drop the patch?


just write a better commit log.
"Not an issue by itself, but using unsigned here seems cleaner".


> >
> >>      for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
> >>          key.gfn = vtd_get_iotlb_gfn(addr, level);
> >> --
> >> 2.45.2