Re: [PATCH 03/18] amd_iommu: Add support for IOMMU notifier

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

On 4/16/25 8:14 AM, Sairaj Kodilkar wrote:

>> +
>> +    /* DMA address translation support */
>> +    IOMMUNotifierFlag notifier_flags;
>> +    /* entry in list of Address spaces with registered notifiers */
>> +    QLIST_ENTRY(AMDVIAddressSpace) next;
>> +    /* DMA address translation active */
>> +    bool addr_translation;
> 
> I dont see any use of addr_translation in current patch.
> maybe define it when you are really need this flag ?

ACK. I can move it to a later patch. My rationale was that this patch is 
adding all the new structure members that will be needed, but it makes 
sense to split it.


>>       return 0;
>>   }
>> @@ -1700,6 +1721,7 @@ static void amdvi_sysbus_realize(DeviceState 
>> *dev, Error **errp)
>>   static const Property amdvi_properties[] = {
>>       DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
>> +    DEFINE_PROP_BOOL("dma-remap", AMDVIState, dma_remap, false),
>>   };
> 
> Please add a description in commit message for this flag

Will change in next revision.

> 
>>   static const VMStateDescription vmstate_amdvi_sysbus = {
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index 28125130c6fc..e12ecade4baa 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -365,12 +365,18 @@ struct AMDVIState {
>>       /* for each served device */
>>       AMDVIAddressSpace **address_spaces[PCI_BUS_MAX];
>> +    /* list of address spaces with registered notifiers */
>> +    QLIST_HEAD(, AMDVIAddressSpace) amdvi_as_with_notifiers;
>> +
>>       /* IOTLB */
>>       GHashTable *iotlb;
>>       /* Interrupt remapping */
>>       bool ga_enabled;
>>       bool xtsup;
>> +
>> +    /* DMA address translation */
>> +    bool dma_remap;
> 
> I think you should use this flag in the remapping path as well.
> I am aware that you are using it later in this series to switch the
> address space, but current patch will make things inconsistent for
> emulated and vfio devices (possibly breaking the bisect).

The change in behavior happens only if the user explicitly sets 
dma-remap=on property in the command line, which is why I made it off by 
default.

To eliminate the possibility of using dma-remap=on before all the 
infrastructure to support it is in place, I will move this patch and 
[5/18] to the end of the series. Does that address your concern?

> 
> Also newer AMD IVRS format provides a HATDis bit (Table 96 in IOMMU
> Specs [1]), informing the guest kernel that V1 page table is disabled in
> the IOMMU. 

Sounds like this mechanism could have been used until now to prevent the 
scenario where we can have the guest request DMA remapping and the 
vIOMMU doesn't have the capability. Seems moot now that we are actually 
adding the capability.


Its good idea to set this bit in IVRS when dma_remap=false.
> This way a "HATDis bit aware" guest can enable iommu.passthrough.

I'd need to research how to implement this, but isn't this enhancement 
better added in separate series, since it also requires a companion 
Linux change? I don't recall the Linux driver being "HATDis aware" (or 
even HATS aware for that matter), but perhaps I am mistaken...


> 
> Also, is it a good idea to have default value for dma_remap=false ?
> Consider the guest which is not aware of HATDis bit. Things will break 
> if such guest boots with iommu.passthrough=0 (recreating the pt=on
> scenario).

That is not new, that is the current behavior that this series is trying 
to fix by adding the missing functionality.

As far as the default value for dma-remap property, I think it must be 
set to 0/false (i.e. current behavior unchanged) until we deem the DMA 
remapping feature stable enough to be made available for guests.
On that topic, maybe it should be an experimental feature for now i.e. 
"x-dma-remap".


> 
> [1] https://www.amd.com/content/dam/amd/en/documents/processor-tech- 
> docs/specifications/48882_IOMMU.pdf
> 
> Regards
> Sairaj Kodilkar
> 
> PS: Sorry If I missed something here, I haven't progressed much in the 
> series.

No problem at all, the feedback is appreciated.

Thank you,
Alejandro


> 
>>   };
>>   uint64_t amdvi_extended_feature_register(AMDVIState *s);
>