It's not accurate to dump super page as non-present page,
meanwhile bit7 in first level page table entry is PAT bit,
also pointer pte is never NULL in pgtable_walk() context.

Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when DMA fault occurs")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/iommu/intel/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8288b0ee7a61..fec5cc1147f3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -707,14 +707,14 @@ static void pgtable_walk(struct intel_iommu *iommu, unsigned long pfn,
 	while (1) {
 		offset = pfn_level_offset(pfn, level);
 		pte = &parent[offset];
-		if (!pte || (dma_pte_superpage(pte) || !dma_pte_present(pte))) {
+		if (!dma_pte_present(pte)) {
 			pr_info("PTE not present at level %d\n", level);
 			break;
 		}
 
 		pr_info("pte level: %d, pte value: 0x%016llx\n", level, pte->val);
 
-		if (level == 1)
+		if (level == 1 || dma_pte_superpage(pte))
 			break;
 
 		parent = phys_to_virt(dma_pte_addr(pte));
-- 
2.34.1

On 2024/10/22 17:50, Zhenzhong Duan wrote:
> It's not accurate to dump super page as non-present page,
> meanwhile bit7 in first level page table entry is PAT bit,

Can you please explain how the 'bit7 in first level page table entry' is
relevant to the changes made in this patch?

Also, please make full use of the maximum length of the commit message
line.

> also pointer pte is never NULL in pgtable_walk() context.
> 
> Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when DMA fault occurs")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 8288b0ee7a61..fec5cc1147f3 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -707,14 +707,14 @@ static void pgtable_walk(struct intel_iommu *iommu, unsigned long pfn,
>   	while (1) {
>   		offset = pfn_level_offset(pfn, level);
>   		pte = &parent[offset];
> -		if (!pte || (dma_pte_superpage(pte) || !dma_pte_present(pte))) {
> +		if (!dma_pte_present(pte)) {
>   			pr_info("PTE not present at level %d\n", level);
>   			break;
>   		}
>   
>   		pr_info("pte level: %d, pte value: 0x%016llx\n", level, pte->val);
>   
> -		if (level == 1)
> +		if (level == 1 || dma_pte_superpage(pte))
>   			break;
>   
>   		parent = phys_to_virt(dma_pte_addr(pte));

Thanks,
baolu

>-----Original Message-----
>From: Baolu Lu <baolu.lu@linux.intel.com>
>Sent: Wednesday, October 23, 2024 12:50 PM
>Subject: Re: [PATCH 2/2] iommu/vt-d: Fix checks in pgtable_walk()
>
>On 2024/10/22 17:50, Zhenzhong Duan wrote:
>> It's not accurate to dump super page as non-present page,
>> meanwhile bit7 in first level page table entry is PAT bit,
>
>Can you please explain how the 'bit7 in first level page table entry' is
>relevant to the changes made in this patch?

When iterate to level 1, it is leaf page table entry, bit 7 is PAT bit instead of PS bit.
dma_pte_superpage(pte) may return true, then " PTE not present at level 1" may
be printed out.

>
>Also, please make full use of the maximum length of the commit message
>line.

Sure.

Thanks
Zhenzhong

>
>> also pointer pte is never NULL in pgtable_walk() context.
>>
>> Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when
>DMA fault occurs")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 8288b0ee7a61..fec5cc1147f3 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -707,14 +707,14 @@ static void pgtable_walk(struct intel_iommu *iommu,
>unsigned long pfn,
>>   	while (1) {
>>   		offset = pfn_level_offset(pfn, level);
>>   		pte = &parent[offset];
>> -		if (!pte || (dma_pte_superpage(pte) || !dma_pte_present(pte))) {
>> +		if (!dma_pte_present(pte)) {
>>   			pr_info("PTE not present at level %d\n", level);
>>   			break;
>>   		}
>>
>>   		pr_info("pte level: %d, pte value: 0x%016llx\n", level, pte->val);
>>
>> -		if (level == 1)
>> +		if (level == 1 || dma_pte_superpage(pte))
>>   			break;
>>
>>   		parent = phys_to_virt(dma_pte_addr(pte));
>
>Thanks,
>baolu

On 2024/10/23 13:16, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Wednesday, October 23, 2024 12:50 PM
>> Subject: Re: [PATCH 2/2] iommu/vt-d: Fix checks in pgtable_walk()
>>
>> On 2024/10/22 17:50, Zhenzhong Duan wrote:
>>> It's not accurate to dump super page as non-present page,
>>> meanwhile bit7 in first level page table entry is PAT bit,
>> Can you please explain how the 'bit7 in first level page table entry' is
>> relevant to the changes made in this patch?
> When iterate to level 1, it is leaf page table entry, bit 7 is PAT bit instead of PS bit.
> dma_pte_superpage(pte) may return true, then " PTE not present at level 1" may
> be printed out.

I see. Thank you!

If you have a new version, can you please make it a bit clearer? My
understanding is that dma_pte_superpage(pte) should not check against
the leaf page table entries, right?

Thanks,
baolu

>-----Original Message-----
>From: Baolu Lu <baolu.lu@linux.intel.com>
>Subject: Re: [PATCH 2/2] iommu/vt-d: Fix checks in pgtable_walk()
>
>On 2024/10/23 13:16, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Wednesday, October 23, 2024 12:50 PM
>>> Subject: Re: [PATCH 2/2] iommu/vt-d: Fix checks in pgtable_walk()
>>>
>>> On 2024/10/22 17:50, Zhenzhong Duan wrote:
>>>> It's not accurate to dump super page as non-present page,
>>>> meanwhile bit7 in first level page table entry is PAT bit,
>>> Can you please explain how the 'bit7 in first level page table entry' is
>>> relevant to the changes made in this patch?
>> When iterate to level 1, it is leaf page table entry, bit 7 is PAT bit instead of PS
>bit.
>> dma_pte_superpage(pte) may return true, then " PTE not present at level 1"
>may
>> be printed out.
>
>I see. Thank you!
>
>If you have a new version, can you please make it a bit clearer? My
>understanding is that dma_pte_superpage(pte) should not check against
>the leaf page table entries, right?

Right, will do.

Thanks
Zhenzhong