Hi,
On 16/05/2024 08:41, 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--d...@eviden.com>
>> Subject: Re: [PATCH ats_vtd v1 03/24] intel_iommu: check if the input
>> address is canonical
>>
>> Hi zhenzhong,
>>
>> On 14/05/2024 09:34, 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.
>>>
>>> Hi Clement,
>>>
>>>> -----Original Message-----
>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com>
>>>> Subject: [PATCH ats_vtd v1 03/24] intel_iommu: check if the input
>> address
>>>> is canonical
>>>>
>>>> First stage translation must fail if the address to translate is
>>>> not canonical.
>>>>
>>>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--
>> d...@eviden.com>
>>>> ---
>>>> hw/i386/intel_iommu.c | 22 ++++++++++++++++++++++
>>>> hw/i386/intel_iommu_internal.h | 2 ++
>>>> 2 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 80cdf37870..240ecb8f72 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -1912,6 +1912,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,
>>>> };
>>>>
>>>> @@ -2023,6 +2024,21 @@ static inline uint64_t
>>>> vtd_get_flpte_addr(uint64_t flpte, uint8_t aw)
>>>> return flpte & VTD_FL_PT_BASE_ADDR_MASK(aw);
>>>> }
>>>>
>>>> +/* Return true if IOVA is canonical, otherwise false. */
>>>> +static bool vtd_iova_fl_check_canonical(IntelIOMMUState *s,
>>>> + uint64_t iova, VTDContextEntry
>>>> *ce,
>>>> + uint8_t aw, uint32_t pasid)
>>>> +{
>>>> + uint64_t iova_limit = vtd_iova_limit(s, ce, aw, pasid);
>>> According to spec:
>>>
>>> "Input-address in the request subjected to first-stage translation is not
>>> canonical (i.e., address bits 63:N are not same value as address bits [N-
>>> 1], where N is 48 bits with 4-level paging and 57 bits with 5-level
>>> paging)."
>>>
>>> So it looks not correct to use aw filed in pasid entry to calculate
>>> iova_limit.
>>> Aw can be a value configured by guest and it's used for stage-2 table. See
>> spec:
>>> " This field is treated as Reserved(0) for implementations not supporting
>> Second-stage
>>> Translation (SSTS=0 in the Extended Capability Register).
>>> This field indicates the adjusted guest-address-width (AGAW) to be used by
>> hardware
>>> for second-stage translation through paging structures referenced through
>> the
>>> SSPTPTR field.
>>> • The following encodings are defined for this field:
>>> • 001b: 39-bit AGAW (3-level page table)
>>> • 010b: 48-bit AGAW (4-level page table)
>>> • 011b: 57-bit AGAW (5-level page table)
>>> • 000b,100b-111b: Reserved
>>> When not treated as Reserved(0), hardware ignores this field for first-
>> stage-only
>>> (PGTT=001b) and pass-through (PGTT=100b) translations."
>>>
>>> Thanks
>>> Zhenzhong
>>>
>> Not sure to understand.
>> Are you talking about the aw field of Scalable-Mode PASID Table Entry?
> Yes.
>
>> The aw parameter is set to s->aw_bits in vtd_do_iommu_translate so I
>> think it's safe to use it for canonical address check.
>> Maybe we can just use s->aw_bits directly from
>> vtd_iova_fl_check_canonical to avoid any mistake?
> Agaw can be different from s->aw_bits.
> Yes, I think using s->aw_bits is safe.
>
> Thanks
> Zhenzhong
Ok fine.
I did the change in the v2 I sent yesterday.
Thanks
>
>>>> + 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))
>>>> + );
>>>> +}
>>>> +
>>>> /*
>>>> * Given the @iova, get relevant @flptep. @flpte_level will be the last
>> level
>>>> * of the translation, can be used for deciding the size of large page.
>>>> @@ -2038,6 +2054,12 @@ static int
>> vtd_iova_to_flpte(IntelIOMMUState *s,
>>>> VTDContextEntry *ce,
>>>> uint32_t offset;
>>>> uint64_t flpte;
>>>>
>>>> + if (!vtd_iova_fl_check_canonical(s, iova, ce, aw_bits, pasid)) {
>>>> + error_report_once("%s: detected non canonical IOVA (iova=0x%"
>>>> PRIx64 ","
>>>> + "pasid=0x%" PRIx32 ")", __func__, iova, pasid);
>>>> + return -VTD_FR_FS_NON_CANONICAL;
>>>> + }
>>>> +
>>>> while (true) {
>>>> offset = vtd_iova_fl_level_offset(iova, level);
>>>> flpte = vtd_get_flpte(addr, offset);
>>>> diff --git a/hw/i386/intel_iommu_internal.h
>>>> b/hw/i386/intel_iommu_internal.h
>>>> index 901691afb9..e9448291a4 100644
>>>> --- a/hw/i386/intel_iommu_internal.h
>>>> +++ b/hw/i386/intel_iommu_internal.h
>>>> @@ -324,6 +324,8 @@ typedef enum VTDFaultReason {
>>>> VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt-
>> entry is
>>>> 0 */
>>>> 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 */
>>>> --
>>>> 2.44.0
