Re: [PATCH v3 18/22] amd_iommu: Toggle address translation mode on devtab entry invalidation

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

On 10/6/25 2:25 AM, Sairaj Kodilkar wrote:
>
>
> On 10/6/2025 11:45 AM, Michael S. Tsirkin wrote:
>> On Mon, Oct 06, 2025 at 11:38:28AM +0530, Sairaj Kodilkar wrote:
>>>
>>> On 9/20/2025 3:05 AM, Alejandro Jimenez wrote:
>>>> A guest must issue an INVALIDATE_DEVTAB_ENTRY command after changing a
>>>> Device Table entry (DTE) e.g. after attaching a device and setting 
>>>> up its
>>>> DTE. When intercepting this event, determine if the DTE has been 
>>>> configured
>>>> for paging or not, and toggle the appropriate memory regions to 
>>>> allow DMA
>>>> address translation for the address space if needed. Requires 
>>>> dma-remap=on.
>>>>
>>>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>> ---
>>>>    hw/i386/amd_iommu.c | 122 
>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 120 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>> index ce5d4c36624fd..e916dcb2be381 100644
>>>> --- a/hw/i386/amd_iommu.c
>>>> +++ b/hw/i386/amd_iommu.c
>>>> @@ -1032,18 +1032,136 @@ static void 
>>>> amdvi_reset_address_translation_all(AMDVIState *s)
>>>>        }
>>>>    }
>>>> +static void enable_dma_mode(AMDVIAddressSpace *as, bool 
>>>> inval_current)
>>>> +{
>>>> +    /*
>>>> +     * When enabling DMA mode for the purpose of isolating guest 
>>>> devices on
>>>> +     * a failure to retrieve or invalid DTE, all existing mappings 
>>>> must be
>>>> +     * dropped.
>>>> +     */
>>>> +    if (inval_current) {
>>>> +        IOMMUNotifier *n;
>>>> +        IOMMU_NOTIFIER_FOREACH(n, &as->iommu) {
>>>> +            amdvi_address_space_unmap(as, n);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if (as->addr_translation) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* Installing DTE enabling translation, activate region */
>>>> +    as->addr_translation = true;
>>>> +    amdvi_switch_address_space(as);
>>>> +    /* Sync shadow page tables */
>>>> +    amdvi_address_space_sync(as);
>>> Hi Alejandro,
>>> I think we can skip amdvi_address_space_sync, because
>>> amdvi_switch_address_space will trigger
>>> amdvi_iommu_replay. this replay should unmap all the old mappings 
>>> and sync
>>> shadow page table.
>>>
>>> Thanks
>>> Sairaj
>> Well I queued this but this speedup can be done on top.
ACK

I rather be explicit and avoid relying on replay(), but sync is 
expensive so this could be worth the trouble with an added comment. I'll 
test and will include Sairaj's optimization in a different patchset.

Please if possible also add Sairaj's R-b to this series, he provided 
valuable feedback and testing so I'd like it to be recognized.

Alejandro

>>
> Sorry for the delay in reviewing, I was on vacation for 2 weeks.
> I have reviewed all the patches.
>
> Reviewed-by: Sairaj Kodilkar <sarunkod@amd.com>Thanks Sairaj