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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 3/24/2026 7:55 PM, Alejandro Jimenez wrote:
>
> 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.

Hi Alejandro,
I agree with you about sprinkling assertions, I think some of the assertions
should ideally deliver a events to the guest. But we can have this in later
series

Also for both the patches

Reviewed-by: Sairaj Kodilkar <sarunkod@amd.com>

Thanks
Sairaj

>
> Thank you,
> Alejandro
>
>> Thanks
>> Sairaj Kodilkar
>>