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? 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? >> + 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