Re: [PATCH v2 16/20] amd_iommu: Set all address spaces to default translation mode on reset

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

Hey Sairaj,

On 5/29/25 2:16 AM, Sairaj Kodilkar wrote:
> 
> 
> On 5/2/2025 7:46 AM, Alejandro Jimenez wrote:
>> On reset, restore the default address translation mode for all the
>> address spaces managed by the vIOMMU.
>>
>> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>> ---
>>   hw/i386/amd_iommu.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 71018d70dd10..90491367594b 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -962,6 +962,33 @@ static void 
>> amdvi_switch_address_space(AMDVIAddressSpace *amdvi_as)
>>       }
>>   }
>> +/*
>> + * For all existing address spaces managed by the IOMMU, enable/ 
>> disable the
>> + * corresponding memory regions depending on the address translation 
>> mode
>> + * as determined by the global and individual address space settings.
>> + */
>> +static void amdvi_switch_address_space_all(AMDVIState *s)
>> +{
>> +    AMDVIAddressSpace **iommu_as;
>> +
>> +    for (int bus_num = 0; bus_num < PCI_BUS_MAX; bus_num++) {
>> +
>> +        /* Nothing to do if there are no devices on the current bus */
>> +        if (!s->address_spaces[bus_num]) {
>> +            continue;
>> +        }
>> +        iommu_as = s->address_spaces[bus_num];
>> +
>> +        for (int devfn = 0; devfn < PCI_DEVFN_MAX; devfn++) {
>> +
>> +            if (!iommu_as[devfn]) {
>> +                continue;
>> +            }
>> +            amdvi_switch_address_space(iommu_as[devfn]);
>> +        }
>> +    }
>> +}
>> +
>>   /* log error without aborting since linux seems to be using reserved 
>> bits */
>>   static void amdvi_inval_devtab_entry(AMDVIState *s, uint64_t *cmd)
>>   {
>> @@ -2199,6 +2226,7 @@ static void amdvi_sysbus_reset(DeviceState *dev)
>>       /* Discard all mappings on device reset */
>>       amdvi_address_space_unmap_all(s);
>> +    amdvi_switch_address_space_all(s);
> 
> Hi Alejandro
> 
> I think amdvi_sysbus_reset should set iommu_as->addr_translation flag to
> "false" before switching all the address spaces. Without this, the
> devices will keep using IOMMU address space.
> 
My first impulse is to agree with you, from the standpoint of 
considering the no_dma mode as the "default mode", and a reset should 
bring us back to default. But I wonder that is necessarily the 
architectural behavior...

After a reset, in order for any device transactions to be processed, a 
guest driver must reinitialize the IOMMU data structures, including the 
Device Table and specifically the table entry for the device. That must 
trigger a INVAL_DEVTAB_ENTRY that will be intercepted and 
as->addr_translation will be set correctly. If the guest driver doesn't 
do these operations, then a device won't be able to use the IOMMU 
because it doesn't have a valid DTE, right? The earlier mappings were 
already dropped, so it doesn't affect the host.

Again, I see your point, and making this change is likely the right 
thing to do, which is why I'll make the change for v3. Just wondering if 
implementing such behavior is actually architecturally accurate or just 
the "common sense" approach...

Thank you for your attention to detail and all the valuable feedback. I 
will be out next week, and will send v3 once I am back online.

Alejandro

> Regards
> Sairaj Kodilkar
>>   }
>>   static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
>