RE: [PATCH v2 07/17] intel_iommu: Check if the input address is canonical

[Duan, Zhenzhong]

>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v2 07/17] intel_iommu: Check if the input address is
>canonical
>
>On 2024/8/5 14:27, Zhenzhong Duan wrote:
>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>>
>> First stage translation must fail if the address to translate is
>> not canonical.
>>
>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  2 ++
>>   hw/i386/intel_iommu.c          | 21 +++++++++++++++++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>> index 51e9b1fc43..668583aeca 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -320,6 +320,8 @@ typedef enum VTDFaultReason {
>>       VTD_FR_PASID_ENTRY_P = 0x59,
>>       VTD_FR_PASID_TABLE_ENTRY_INV = 0x5b,  /*Invalid PASID table entry
>*/
>>
>> +    VTD_FR_FS_NON_CANONICAL = 0x80, /* SNG.1 : Address for FS not
>canonical.*/
>> +
>>       /* Output address in the interrupt address range for scalable mode */
>>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
>>       VTD_FR_MAX,                 /* Guard */
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 0bcbd5b777..6121cca4cd 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1821,6 +1821,7 @@ static const bool vtd_qualified_faults[] = {
>>       [VTD_FR_PASID_ENTRY_P] = true,
>>       [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>> +    [VTD_FR_FS_NON_CANONICAL] = true,
>>       [VTD_FR_MAX] = false,
>>   };
>>
>> @@ -1924,6 +1925,20 @@ static inline bool vtd_flpte_present(uint64_t
>flpte)
>>       return !!(flpte & VTD_FL_P);
>>   }
>>
>> +/* Return true if IOVA is canonical, otherwise false. */
>> +static bool vtd_iova_fl_check_canonical(IntelIOMMUState *s, uint64_t
>iova,
>> +                                        VTDContextEntry *ce, uint32_t pasid)
>> +{
>> +    uint64_t iova_limit = vtd_iova_limit(s, ce, s->aw_bits, pasid);
>> +    uint64_t upper_bits_mask = ~(iova_limit - 1);
>> +    uint64_t upper_bits = iova & upper_bits_mask;
>> +    bool msb = ((iova & (iova_limit >> 1)) != 0);
>> +    return !(
>> +             (!msb && (upper_bits != 0)) ||
>> +             (msb && (upper_bits != upper_bits_mask))
>> +            );
>> +}
>> +
>
>will the below be clearer?
>
>     if (msb)
>         return upper_bits == upper_bits_mask;
>     else
>         return !upper_bits;

Yes, clearer, will do.

Thanks
Zhenzhong