Re: [PATCH 6/6] amd_iommu: Do not assume passthrough translation for devices with DTE[TV]=0

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

On 3/19/25 2:06 AM, Vasant Hegde wrote:
> Alejandro,
> 
> 
> On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
>> The AMD I/O Virtualization Technology (IOMMU) Specification (see Table 8: V,
>> TV, and GV Fields in Device Table Entry), specifies that a DTE with V=0,
>> TV=1 does not contain a valid address translation information.  If a request
>> requires a table walk, the walk is terminated when this condition is
>> encountered.
>>
>> Do not assume that addresses for a device with DTE[TV]=0 are passed through
>> (i.e. not remapped) and instead terminate the page table walk early.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> 
> I did double check and I think this patch matches HW behaviour. I did run few
> tests w/ this series. It seems to work fine for me.

Thank you for confirming and reviewing.

My understanding from the Linux driver code is that the TV field is not 
really used to control behavior and DTE[V] ends up doing that job 
(though I need to take a closer look at the more involved scenarios with 
SNP enabled). So I have not seen the case with V=1,TV=0 in the basic 
scenarios I have run, but my intention is to adhere to the spec as 
closely as possible. So I appreciate the confirmation.


> 
> 
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> 
> 

[...]


>> +        oldlevel = level;
>> +        level = get_pte_translation_mode(pte);
> 
> Unrelated to this patch.. We may want to add check to make sure level is
> decreasing. Something like
> 	if (oldlevel <= level)
> 		error out
> 
> Otherwise bad page table can cause the issue.

ACK. I have considered replacing this (amdvi_page_walk()) with code that 
more closely mimics the fetch_pte() algorithm in the Linux driver. I've 
been using it on my (coming soon) RFC for DMA remapping, and comparing 
against amdvi_page_walk() for verification. I put in assertions like the 
one you proposed in that new code. But if that approach is not taken, 
I'll add the safety checks you suggested here.

Thank you,
Alejandro

> 
> -Vasant
> 
> 
>> +    } while (level > 0 && level < 7);
>>