RE: [PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
From: Duan, Zhenzhong
Date: Wed Oct 23 2024 - 01:09:32 EST
>-----Original Message-----
>From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
>Subject: Re: [PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
>
>On 2024/10/22 17:50, Zhenzhong Duan wrote:
>> In dmar_fault_dump_ptes(), return value of phys_to_virt() is used for
>> checking if an entry is present. It's never NULL on x86 platform at least.
>> This makes some zero entries are dumped like below:
>>
>> [ 442.240357] DMAR: pasid dir entry: 0x000000012c83e001
>> [ 442.246661] DMAR: pasid table entry[0]: 0x0000000000000000
>> [ 442.253429] DMAR: pasid table entry[1]: 0x0000000000000000
>> [ 442.260203] DMAR: pasid table entry[2]: 0x0000000000000000
>> [ 442.266969] DMAR: pasid table entry[3]: 0x0000000000000000
>> [ 442.273733] DMAR: pasid table entry[4]: 0x0000000000000000
>> [ 442.280479] DMAR: pasid table entry[5]: 0x0000000000000000
>> [ 442.287234] DMAR: pasid table entry[6]: 0x0000000000000000
>> [ 442.293989] DMAR: pasid table entry[7]: 0x0000000000000000
>> [ 442.300742] DMAR: PTE not present at level 2
>>
>> Fix it by checking present bit in all entries.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
>
>It seems that we still need a Fixes tag here.
OK, will add.
>
>> ---
>> drivers/iommu/intel/iommu.c | 31 ++++++++++++++++++++-----------
>> 1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index a564eeaf2375..8288b0ee7a61 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -733,12 +733,17 @@ void dmar_fault_dump_ptes(struct intel_iommu
>*iommu, u16 source_id,
>> u8 devfn = source_id & 0xff;
>> u8 bus = source_id >> 8;
>> struct dma_pte *pgtable;
>> + u64 entry;
>>
>> pr_info("Dump %s table entries for IOVA 0x%llx\n", iommu->name, addr);
>>
>> /* root entry dump */
>> rt_entry = &iommu->root_entry[bus];
>> - if (!rt_entry) {
>> + entry = rt_entry->lo;
>> + if (sm_supported(iommu) && devfn >= 0x80)
>> + entry = rt_entry->hi;
>> +
>> + if (!(entry & 1)) {
>> pr_info("root table entry is not present\n");
>> return;
>> }
>> @@ -766,28 +771,32 @@ void dmar_fault_dump_ptes(struct intel_iommu
>*iommu, u16 source_id,
>> goto pgtable_walk;
>> }
>>
>> + /* For request-without-pasid, get the pasid from context entry */
>> + if (pasid == IOMMU_PASID_INVALID)
>> + pasid = IOMMU_NO_PASID;
>> +
>> /* get the pointer to pasid directory entry */
>> dir = phys_to_virt(ctx_entry->lo & VTD_PAGE_MASK);
>
>Is above code correct in the scalable mode?
I didn't find issue, could you show some light?
Thanks
Zhenzhong
>
>> - if (!dir) {
>> + dir_index = pasid >> PASID_PDE_SHIFT;
>> + pde = &dir[dir_index];
>> +
>> + if (!pasid_pde_is_present(pde)) {
>> pr_info("pasid directory entry is not present\n");
>> return;
>> }
>> - /* For request-without-pasid, get the pasid from context entry */
>> - if (intel_iommu_sm && pasid == IOMMU_PASID_INVALID)
>> - pasid = IOMMU_NO_PASID;
>>
>> - dir_index = pasid >> PASID_PDE_SHIFT;
>> - pde = &dir[dir_index];
>> pr_info("pasid dir entry: 0x%016llx\n", pde->val);
>>
>> /* get the pointer to the pasid table entry */
>> - entries = get_pasid_table_from_pde(pde);
>> - if (!entries) {
>> + entries = phys_to_virt(READ_ONCE(pde->val) & PDE_PFN_MASK);
>> + index = pasid & PASID_PTE_MASK;
>> + pte = &entries[index];
>> +
>> + if (!pasid_pte_is_present(pte)) {
>> pr_info("pasid table entry is not present\n");
>> return;
>> }
>> - index = pasid & PASID_PTE_MASK;
>> - pte = &entries[index];
>> +
>> for (i = 0; i < ARRAY_SIZE(pte->val); i++)
>> pr_info("pasid table entry[%d]: 0x%016llx\n", i, pte->val[i]);
>>
>
>Thanks,
>baolu