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

[CLEMENT MATHIEU--DRIF]

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