[v6] VT-d minor fixes

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

Posted by CLEMENT MATHIEU--DRIF 4 months, 2 weeks ago

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.

Fixes: b5a280c00840 ("intel-iommu: add IOTLB using hash table")
Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
Reviewed-by: Minwoo Im <minwoo.im@samsung.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

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

Posted by Yi Liu 4 months, 2 weeks ago

On 2024/7/9 14:18, 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.

no need to paste the comment here. Especially the last sentence. :) Just
highlight that overflow is possible.

> 
> 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.
> 
> Fixes: b5a280c00840 ("intel-iommu: add IOTLB using hash table")

looks like this is a very old commit. The VTD_MAMV is set as 9 at that
time. If guest sets am=9 in the PSI iotlb invalidation, there is overflow.
Just curious why there is no bug reported here. Have you encountered any
issue before this commit? Anyhow, looks good to me. Remember to refine
the commit message a bit.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> Reviewed-by: Minwoo Im <minwoo.im@samsung.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 v6 3/4] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

Posted by CLEMENT MATHIEU--DRIF 4 months, 2 weeks ago


On 09/07/2024 16:01, 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/9 14:18, 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.
>
> no need to paste the comment here. Especially the last sentence. :) Just
> highlight that overflow is possible.
Oops, sorry, will send v7
>
>>
>> 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.
>>
>> Fixes: b5a280c00840 ("intel-iommu: add IOTLB using hash table")
>
> looks like this is a very old commit. The VTD_MAMV is set as 9 at that
> time. If guest sets am=9 in the PSI iotlb invalidation, there is 
> overflow.
> Just curious why there is no bug reported here. Have you encountered any
> issue before this commit? Anyhow, looks good to me. Remember to refine
> the commit message a bit.
>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Reviewed-by: Minwoo Im <minwoo.im@samsung.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 v6 3/4] intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo

Posted by CLEMENT MATHIEU--DRIF 4 months, 2 weeks ago


On 09/07/2024 16:01, 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/9 14:18, 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.
>
> no need to paste the comment here. Especially the last sentence. :) Just
> highlight that overflow is possible.
>
>>
>> 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.
>>
>> Fixes: b5a280c00840 ("intel-iommu: add IOTLB using hash table")
>
> looks like this is a very old commit. The VTD_MAMV is set as 9 at that
> time. If guest sets am=9 in the PSI iotlb invalidation, there is 
> overflow.
> Just curious why there is no bug reported here. Have you encountered any
> issue before this commit? Anyhow, looks good to me. Remember to refine
> the commit message a bit.
I detected weird values in gdb when I started to implement pasid-based 
invalidations for SVM
>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Reviewed-by: Minwoo Im <minwoo.im@samsung.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