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

["Arun Kodilkar, Sairaj" <sarunkod@amd.com>]

On 3/20/2025 10:26 PM, Alejandro Jimenez wrote:
> Hi Sairaj Kodilkar,
> 
> On 3/20/25 1:11 AM, Arun Kodilkar, Sairaj wrote:
>>
>>
>> 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>
>>> ---
>>>   hw/i386/amd_iommu.c | 88 +++++++++++++++++++++++++--------------------
>>>   1 file changed, 49 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>> index cf00450ebe..31d5522a62 100644
>>> --- a/hw/i386/amd_iommu.c
>>> +++ b/hw/i386/amd_iommu.c
>>> @@ -932,51 +932,61 @@ static void amdvi_page_walk(AMDVIAddressSpace 
>>> *as, uint64_t *dte,
>>>       uint64_t pte = dte[0], pte_addr, page_mask;
>>>       /* make sure the DTE has TV = 1 */
>>> -    if (pte & AMDVI_DEV_TRANSLATION_VALID) {
>>> -        level = get_pte_translation_mode(pte);
>>> -        if (level >= 7) {
>>> -            trace_amdvi_mode_invalid(level, addr);
>>> +    if (!(pte & AMDVI_DEV_TRANSLATION_VALID)) {
>>> +        /*
>>> +         * A DTE with V=1, TV=0 does not have a valid Page Table 
>>> Root Pointer.
>>> +         * An IOMMU processing a request that requires a table walk 
>>> terminates
>>> +         * the walk when it encounters this condition. Do the same 
>>> and return
>>> +         * instead of assuming that the address is forwarded without 
>>> translation
>>> +         * i.e. the passthrough case, as it is done for the case 
>>> where DTE[V]=0.
>>> +         */
>> Hi Alejandro,
>> According to AMD IOMMU specs TABLE 44 (IO_PAGE_FAULT Event Types), IOMMU
>> reports IO_PAGE_FAULT event when TV bit is not set in the DTE.
>>
>> Hence you should use amdvi_page_fault to report the IO_PAGE_FAULT
>> event before returning.
> 
> Good point, I had not considered this (and really haven't paid attention 
> to the page fault reporting until now). On first impression, I tend to 
> agree with your observation and will include in on v2. That being said...
> 
> The current role of amdvi_page_walk() is to be a helper for the 
> translate() method and ultimately the IOMMU replay() functionality. 
> These are needed for emulation but do not necessarily correspond 1:1 
> with guest-initiated operations. e.g. VFIO code will call 
> memory_region_iommu_replay() (which ends up calling amdvi_walk_page())
> when syncing the dirty bitmap or after registering a new notifier. The 
> guest would get IO_PAGE_FAULT events for all the regions where a mapping 
> doesn't currently exist, which doesn't seem correct.
> 
> IOW, I think even this existing call to amdvi_page_fault() is not 
> necessary/correct:
> 
>      do {
>          pte_perms = amdvi_get_perms(pte);
>          present = pte & 1;
>          if (!present || perms != (perms & pte_perms)) {
>              amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
>              trace_amdvi_page_fault(addr);
>              return;
>          }
>     [...]
> 
> I am aware I am jumping ahead a bit too much, but the point is that the 
> whole event reporting likely needs a comprehensive review.
>
Hi Alejandro,
I agree with this and we can do a comprehensive review before reporting
page fault in this path.
I think in future we should separate these two paths (replay and normal 
DMA translation). That way we can report page faults in DMA translation 
path and keep the replay path clean.

> I need to study the area a lot more since even the simplest/only current 
> use of amdvi_page_fault() right now is not very clear to me.
> 
> Alejandro
> 
> P.S.
> Also confusing is this assertion in 2.5.3 IO_PAGE_FAULT Event:
> 
> "I/O page faults detected for translation requests return a translation- 
> not-present response (R=W=0) to the device and are not logged in the 
> event log."
> 
> so this suggests we should not emit a page fault i.e. don't log it in 
> the event log.
> 

Here "translation requests" means ATS translation requests and not the 
DMA read/write. Basically IOMMU emits the IO_PAGE_FAULT when it fails to 
translate the DMA request but returns "translation-not-present" response
to the device for ATS request

Regards
Sairaj Kodilkar
> 
>>
>> Regards
>> Sairaj Kodilkar
> 
>