[PATCH v4 0/4] VT-d minor fixes

[PATCH v4 0/4] 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.

v4
    - Move declarations of VTD_FRCD_PV and VTD_FRCD_PP
    - intel_iommu: make types match:
    	- edit commit message to explain that we are not fixing a bug
    - intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo
    	- edit commit message

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 (4):
  intel_iommu: fix FRCD construction macro
  intel_iommu: move VTD_FRCD_PV and VTD_FRCD_PP declarations
  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 | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.45.2

[PATCH v4 1/4] 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>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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 v4 2/4] intel_iommu: move VTD_FRCD_PV and VTD_FRCD_PP declarations

[CLEMENT MATHIEU--DRIF]

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

These 2 macros are for high 64-bit of the FRCD registers.
Declarations have to be moved accordingly.

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

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index cbc4030031..faea23e8d6 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -264,10 +264,10 @@
 #define VTD_FRCD_FR(val)        (((val) & 0xffULL) << 32)
 #define VTD_FRCD_SID_MASK       0xffffULL
 #define VTD_FRCD_SID(val)       ((val) & VTD_FRCD_SID_MASK)
-/* 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) & 0x1ULL) << 31)
+/* For the low 64-bit of 128-bit */
+#define VTD_FRCD_FI(val)        ((val) & ~0xfffULL)
 #define VTD_FRCD_IR_IDX(val)    (((val) & 0xffffULL) << 48)
 
 /* DMA Remapping Fault Conditions */
-- 
2.45.2

[PATCH v4 3/4] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

[CLEMENT MATHIEU--DRIF]

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

The mask we are trying to store into VTDIOTLBPageInvInfo.mask might not
fit in an uint8_t. Use uint64_t to avoid overflows

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 faea23e8d6..5f32c36943 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 v4 4/4] 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.

This is not an issue by itself, but using unsigned here seems cleaner.

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 v4 1/4] intel_iommu: fix FRCD construction macro

[Yi Liu]

On 2024/7/5 19:01, 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.
> 
> Fixes: 1b2b12376c8a ("intel-iommu: PASID support")
> 

The extra line behind the "Fixes tag" is not needed.

> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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 */

It might be fine to squash patch 02 of this series into this one. @Jason?

-- 
Regards,
Yi Liu

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

[Yi Liu]

On 2024/7/5 19:01, CLEMENT MATHIEU--DRIF wrote:
> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> 
> The mask we are trying to store into VTDIOTLBPageInvInfo.mask might not
> fit in an uint8_t. Use uint64_t to avoid overflows

Per the below code, it can overflow as am can be larger than 8 according
to the CH 6.5.2.3 IOTLB Invalidate. And you may want a fix tag as well.

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

CH 6.5.2.3 IOTLB Invalidate

Address Mask (AM): For page-selective-within-domain invalidations, the 
Address Mask specifies
the number of low order bits of the ADDR field that must be masked for the 
invalidation operation.
This field enables software to request invalidation of contiguous mappings 
for size-aligned
regions. Refer to Table 19 for encodings of this field. When invalidating a 
large-page translation,
software must use the appropriate Address Mask value (0 for 4KByte page, 9 
for 2-MByte page,
and 18 for 1-GByte page). Hardware implementations report the maximum 
supported address
mask value through the Capability register

> 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 faea23e8d6..5f32c36943 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;
>   

-- 
Regards,
Yi Liu

Re: [PATCH v4 4/4] intel_iommu: make types match

[Yi Liu]

On 2024/7/5 19:01, 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.
> 
> This is not an issue by itself, but using unsigned here seems cleaner.

a nit to the subject. s/"make types match"/"make type match"/

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

-- 
Regards,
Yi Liu

Re: [PATCH v4 1/4] intel_iommu: fix FRCD construction macro

[Jason Wang]

On Mon, Jul 8, 2024 at 3:04 PM Yi Liu <yi.l.liu@intel.com> wrote:
>
> On 2024/7/5 19:01, 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.
> >
> > Fixes: 1b2b12376c8a ("intel-iommu: PASID support")
> >
>
> The extra line behind the "Fixes tag" is not needed.
>
> > Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> > Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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 */
>
> It might be fine to squash patch 02 of this series into this one. @Jason?

Not sure, we may need this for -stable. So having a standalone patch
doesn't hurt.

Thanks

>
> --
> Regards,
> Yi Liu
>

Re: [PATCH v4 1/4] intel_iommu: fix FRCD construction macro

[Yi Liu]

On 2024/7/9 10:51, Jason Wang wrote:
> On Mon, Jul 8, 2024 at 3:04 PM Yi Liu <yi.l.liu@intel.com> wrote:
>>
>> On 2024/7/5 19:01, 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.
>>>
>>> Fixes: 1b2b12376c8a ("intel-iommu: PASID support")
>>>
>>
>> The extra line behind the "Fixes tag" is not needed.
>>
>>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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 */
>>
>> It might be fine to squash patch 02 of this series into this one. @Jason?
> 
> Not sure, we may need this for -stable. So having a standalone patch
> doesn't hurt.

I see. :)

-- 
Regards,
Yi Liu

Re: [PATCH v4 0/4] VT-d minor fixes

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 11:01:55AM +0000, CLEMENT MATHIEU--DRIF wrote:
> 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.

Clément are you going to address Yi Liu's comments?

> v4
>     - Move declarations of VTD_FRCD_PV and VTD_FRCD_PP
>     - intel_iommu: make types match:
>     	- edit commit message to explain that we are not fixing a bug
>     - intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo
>     	- edit commit message
> 
> 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 (4):
>   intel_iommu: fix FRCD construction macro
>   intel_iommu: move VTD_FRCD_PV and VTD_FRCD_PP declarations
>   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 | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.45.2

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

[Michael S. Tsirkin]

On Mon, Jul 08, 2024 at 03:12:27PM +0800, Yi Liu wrote:
> On 2024/7/5 19:01, CLEMENT MATHIEU--DRIF wrote:
> > From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> > 
> > The mask we are trying to store into VTDIOTLBPageInvInfo.mask might not
> > fit in an uint8_t. Use uint64_t to avoid overflows
> 
> Per the below code, it can overflow as am can be larger than 8 according
> to the CH 6.5.2.3 IOTLB Invalidate.

I don't understand what you are saying. What can overflow?
Are you suggesting text for commit log here?

> And you may want a fix tag as well.

why not.

> info.mask = ~((1 << am) - 1);
> 
> CH 6.5.2.3 IOTLB Invalidate
> 
> Address Mask (AM): For page-selective-within-domain invalidations, the
> Address Mask specifies
> the number of low order bits of the ADDR field that must be masked for the
> invalidation operation.
> This field enables software to request invalidation of contiguous mappings
> for size-aligned
> regions. Refer to Table 19 for encodings of this field. When invalidating a
> large-page translation,
> software must use the appropriate Address Mask value (0 for 4KByte page, 9
> for 2-MByte page,
> and 18 for 1-GByte page). Hardware implementations report the maximum
> supported address
> mask value through the Capability register
> 
> > 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 faea23e8d6..5f32c36943 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;
> 
> -- 
> Regards,
> Yi Liu

Re: [PATCH v4 4/4] intel_iommu: make types match

[Michael S. Tsirkin]

On Mon, Jul 08, 2024 at 03:20:45PM +0800, Yi Liu wrote:
> On 2024/7/5 19:01, 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.
> > 
> > This is not an issue by itself, but using unsigned here seems cleaner.
> 
> a nit to the subject. s/"make types match"/"make type match"/

Hmm not sure I agree.
There are two variables, they have types. We make them match.


> > 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);
> 
> -- 
> Regards,
> Yi Liu

Re: [PATCH v4 0/4] VT-d minor fixes

[Michael S. Tsirkin]

On Sat, Jul 20, 2024 at 02:44:18PM -0400, Michael S. Tsirkin wrote:
> On Fri, Jul 05, 2024 at 11:01:55AM +0000, CLEMENT MATHIEU--DRIF wrote:
> > 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.
> 
> Clément are you going to address Yi Liu's comments?


sorry this is old version, pls ignore

> > v4
> >     - Move declarations of VTD_FRCD_PV and VTD_FRCD_PP
> >     - intel_iommu: make types match:
> >     	- edit commit message to explain that we are not fixing a bug
> >     - intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo
> >     	- edit commit message
> > 
> > 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 (4):
> >   intel_iommu: fix FRCD construction macro
> >   intel_iommu: move VTD_FRCD_PV and VTD_FRCD_PP declarations
> >   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 | 6 +++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.45.2

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

[Michael S. Tsirkin]

On Sat, Jul 20, 2024 at 02:45:29PM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 08, 2024 at 03:12:27PM +0800, Yi Liu wrote:
> > On 2024/7/5 19:01, CLEMENT MATHIEU--DRIF wrote:
> > > From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> > > 
> > > The mask we are trying to store into VTDIOTLBPageInvInfo.mask might not
> > > fit in an uint8_t. Use uint64_t to avoid overflows
> > 
> > Per the below code, it can overflow as am can be larger than 8 according
> > to the CH 6.5.2.3 IOTLB Invalidate.
> 
> I don't understand what you are saying. What can overflow?
> Are you suggesting text for commit log here?
> 
> > And you may want a fix tag as well.
> 
> why not.

ignore pls this is on old version.

> > info.mask = ~((1 << am) - 1);
> > 
> > CH 6.5.2.3 IOTLB Invalidate
> > 
> > Address Mask (AM): For page-selective-within-domain invalidations, the
> > Address Mask specifies
> > the number of low order bits of the ADDR field that must be masked for the
> > invalidation operation.
> > This field enables software to request invalidation of contiguous mappings
> > for size-aligned
> > regions. Refer to Table 19 for encodings of this field. When invalidating a
> > large-page translation,
> > software must use the appropriate Address Mask value (0 for 4KByte page, 9
> > for 2-MByte page,
> > and 18 for 1-GByte page). Hardware implementations report the maximum
> > supported address
> > mask value through the Capability register
> > 
> > > 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 faea23e8d6..5f32c36943 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;
> > 
> > -- 
> > Regards,
> > Yi Liu

Re: [PATCH v4 4/4] intel_iommu: make types match

[Michael S. Tsirkin]

On Sat, Jul 20, 2024 at 02:46:14PM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 08, 2024 at 03:20:45PM +0800, Yi Liu wrote:
> > On 2024/7/5 19:01, 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.
> > > 
> > > This is not an issue by itself, but using unsigned here seems cleaner.
> > 
> > a nit to the subject. s/"make types match"/"make type match"/
> 
> Hmm not sure I agree.
> There are two variables, they have types. We make them match.


ignore pls this is on old version

> 
> > > 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);
> > 
> > -- 
> > Regards,
> > Yi Liu