1. Patchew
  2. linux
  3. [PATCH 0/2] vtd: Minor cleanup
  4. View patch

[PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()

Zhenzhong Duan posted 2 patches 1 month ago
There is a newer version of this series
[PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
Posted by Zhenzhong Duan 1 month ago 
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@intel.com>
---
 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);
-	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]);
 
-- 
2.34.1
Re: [PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
Posted by Baolu Lu 1 month ago 
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@intel.com>

It seems that we still need a Fixes tag here.

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

> -	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
RE: [PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
Posted by Duan, Zhenzhong 1 month ago 

>-----Original Message-----
>From: Baolu Lu <baolu.lu@linux.intel.com>
>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@intel.com>
>
>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
Re: [PATCH 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
Posted by Baolu Lu 1 month ago 
On 2024/10/23 13:09, Duan, Zhenzhong wrote:
>>> ---
>>>    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?

It's fine. I just mixed it with the root entry.

Thanks,
baolu

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.