Hi Alejandro,
On 4/14/2025 7:32 AM, Alejandro Jimenez wrote:
In order to enable device assignment with IOMMU protection and guest
DMA address translation, IOMMU notifier support is necessary to allow
users like VFIO to synchronize the shadow page tables i.e. to receive
notifications when the guest updates its IO page tables and replay the
mappings onto host IO page tables.
This requires the vIOMMU is configured with the NpCache capability,
so the guest issues IOMMU invalidations for both map() and unmap()
operations. This capability is already part of AMDVI_CAPAB_FEATURES,
and is written to the configuration in amdvi_pci_realize().
Signed-off-by: Alejandro Jimenez <alejandro.j.jime...@oracle.com>
---
hw/i386/amd_iommu.c | 34 ++++++++++++++++++++++++++++------
hw/i386/amd_iommu.h | 6 ++++++
2 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 22d648c2e0e3..8dbb10d91339 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -66,6 +66,13 @@ struct AMDVIAddressSpace {
MemoryRegion iommu_nodma; /* Alias of shared nodma memory region */
MemoryRegion iommu_ir; /* Device's interrupt remapping region */
AddressSpace as; /* device's corresponding address space */
+
+ /* 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 ?
};
/* AMDVI cache entry */
@@ -1561,14 +1568,28 @@ static int
amdvi_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
Error **errp)
{
AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
+ AMDVIState *s = as->iommu_state;
+
+ /* DMA remapping capability is required to implement IOMMU notifier */
+ if (!s->dma_remap && (new & IOMMU_NOTIFIER_MAP)) {
+ error_setg_errno(errp, ENOTSUP,
+ "device %02x.%02x.%x requires dma-remap=1",
+ as->bus_num, PCI_SLOT(as->devfn), PCI_FUNC(as->devfn));
+ return -ENOTSUP;
+ }
- if (new & IOMMU_NOTIFIER_MAP) {
- error_setg(errp,
- "device %02x.%02x.%x requires iommu notifier which is not "
- "currently supported", as->bus_num, PCI_SLOT(as->devfn),
- PCI_FUNC(as->devfn));
- return -EINVAL;
+ /*
+ * Update notifier flags for address space and the list of address spaces
+ * with registered notifiers.
+ */
+ as->notifier_flags = new;
+
+ if (old == IOMMU_NOTIFIER_NONE) {
+ QLIST_INSERT_HEAD(&s->amdvi_as_with_notifiers, as, next);
+ } else if (new == IOMMU_NOTIFIER_NONE) {
+ QLIST_REMOVE(as, next);
}
+
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
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).
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. Its good idea to set this bit in IVRS when dma_remap=false.
This way a "HATDis bit aware" guest can enable iommu.passthrough.
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).
[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.
};
uint64_t amdvi_extended_feature_register(AMDVIState *s);
