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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 6/12/2025 1:57 PM, Ethan MILON wrote:
> Hi,
> 
> On 5/2/25 4:16 AM, Alejandro Jimenez wrote:
>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>>
>>
>> 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 | 78 +++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 76 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index a2df73062bf7..75a92067f35f 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -991,18 +991,92 @@ static void amdvi_switch_address_space_all(AMDVIState *s)
>>       }
>>   }
>>
>> +/*
>> + * A guest driver must issue the INVALIDATE_DEVTAB_ENTRY command to the IOMMU
>> + * after changing a Device Table entry. We can use this fact to detect when a
>> + * Device Table entry is created for a device attached to a paging domain and
>> + * enable the corresponding IOMMU memory region to allow for DMA translation if
>> + * appropriate.
>> + */
>> +static void amdvi_update_addr_translation_mode(AMDVIState *s, uint16_t devid)
>> +{
>> +    uint8_t bus_num, devfn, dte_mode;
>> +    AMDVIAddressSpace *as;
>> +    uint64_t dte[4] = { 0 };
>> +    IOMMUNotifier *n;
>> +    int ret;
>> +
>> +    /*
>> +     * Convert the devid encoded in the command to a bus and devfn in
>> +     * order to retrieve the corresponding address space.
>> +     */
>> +    bus_num = PCI_BUS_NUM(devid);
>> +    devfn = devid & 0xff;
>> +
>> +    /*
>> +     * The main buffer of size (AMDVIAddressSpace *) * (PCI_BUS_MAX) has already
>> +     * been allocated within AMDVIState, but must be careful to not access
>> +     * unallocated devfn.
>> +     */
>> +    if (!s->address_spaces[bus_num] || !s->address_spaces[bus_num][devfn]) {
>> +        return;
>> +    }
>> +    as = s->address_spaces[bus_num][devfn];
>> +
>> +    ret = amdvi_as_to_dte(as, dte);
>> +
>> +    if (!ret) {
>> +        dte_mode = (dte[0] >> AMDVI_DEV_MODE_RSHIFT) & AMDVI_DEV_MODE_MASK;
>> +    }
>> +
>> +    if ((ret < 0) || (!ret && !dte_mode)) {
>> +        /*
>> +         * The DTE could not be retrieved, it is not valid, or it is not setup
>> +         * for paging. In either case, ensure that if paging was previously in
>> +         * use then invalidate all existing mappings and then switch to use the
>> +         * no_dma memory region.
>> +         */
> 
> If the DTE is malformed or could not be retrieved, wouldn't it be safer
> to default to the DMA region rather than falling back to direct access?
> Or am I missing something?
> 

Hi Ethan

I also agree with you.
I think right way should be keep the IOMMU DMA region if error codes are
AMDVI_FR_DTE_RTR_ERR and AMDVI_FR_DTE_TV. Otherwise when error code is
AMDVI_FR_DTE_V, switch to direct access.

@vasant what do you think ?

Thanks
Sairaj