Re: [PATCH 1/2] amd_iommu: Follow root pointer before page walk and use 1-based levels

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

On 3/23/26 7:01 AM, Sairaj Kodilkar wrote:
> 
> 
> On 3/12/2026 2:09 AM, Alejandro Jimenez wrote:
>> DTE[Mode] and PTE NextLevel encode page table levels as 1-based values, but
>> fetch_pte() currently uses a 0-based level counter, making the logic
>> harder to follow and requiring conversions between DTE mode and level.
>>
>> Switch the page table walk logic to use 1-based level accounting in
>> fetch_pte() and the relevant macro helpers. To further simplify the page
>> walking loop, split the root page table access from the walk i.e. rework
>> fetch_pte() to follow the DTE Page Table Root Pointer and retrieve the top
>> level pagetable entry before entering the loop, then iterate only over the
>> PDE/PTE entries.
>>
>> The reworked algorithm fixes a page walk bug where the page size was
>> calculated for the next level before checking if the current PTE was already
>> a leaf/hugepage. That caused hugepage mappings to be reported as 4K pages,
>> leading to performance degradation and failures in some setups.
>>
>> Fixes: a74bb3110a5b ("amd_iommu: Add helpers to walk AMD v1 Page Table
>> format")
>> Cc: qemu-stable@nongnu.org
>> Reported-by: David Hoppenbrouwers <qemu@demindiro.com>
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c | 132 ++++++++++++++++++++++++++++++--------------
>>   hw/i386/amd_iommu.h |  11 ++--
>>   2 files changed, 97 insertions(+), 46 deletions(-)
>>

[...]

> 
> Hi Alejandro,
> 
> amdvi_sync_shadow_page_table_range() does not check if DTE
> **valid and translation valid** bit are set. I added this check in
> my reply to david's patch. Do you think we should include that
> check as well to ensure that only DTEs with **valid and translation valid**
> bit are passed to the fetch_pte ?
> 

Right now the check for DTE[V] and DTE[TV] are implemented in
amdvi_as_to_dte(), which all callers of
amdvi_sync_shadow_page_table_range() use to extract the DTE that will be
passed as its argument. So I think if we were to add a check, an assert
would be more appropriate.

On the other hand, I am worried about sprinkling assertions around the
code, and I am actually considering removing the ones the original series
added to amdvi_sync_shadow_page_table_range(), fetch_pte(), and
amdvi_notify_iommu(), since any failures in these cases are likely to cause
immediate symptoms and I don't believe it creates any window for data
corruption.

With that in mind, to answer your question I would not add any additional
check on amdvi_sync_shadow_page_table_range(), and would rely on the
current convention to check when retrieving a DTE via the interface we
provided for it and all the code already uses i.e. amdvi_as_to_dte().


For a follow up cleanup, maybe amdvi_as_to_dte() should be renamed with a
more suggestive name that clearly indicates that it validates for those
conditions. But in context, all of its callers rely on the fact that it
does this verification and its return values also encode it.

While looking into this I noticed a chance for minor cleanup in this flow:
amdvi_do_translate()
    amdvi_as_to_dte()
    amdvi_page_walk()

Where amdvi_page_walk() does an unnecessary check for DTE[TV], that
amd_vi_as_to_dte() already ensures is set.

Thank you,
Alejandro

> Thanks
> Sairaj Kodilkar
>