On 6/24/26 10:02, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <[email protected]>
Subject: Re: [PULL 28/41] vfio/listener: Add missing dirty tracking in
region_del
Hello Zhenzhong,
On 1/13/26 10:36, Cédric Le Goater wrote:
From: Zhenzhong Duan <[email protected]>
If a VFIO device in guest switches from passthrough(PT) domain to block
domain, the whole memory address space is unmapped, but we passed a NULL
iotlb entry to unmap_bitmap, then bitmap query didn't happen and we lost
dirty pages.
By constructing an iotlb entry with iova = gpa for unmap_bitmap, it can
set dirty bits correctly.
For IOMMU address space, we still send NULL iotlb because VFIO don't know
the actual mappings in guest. It's vIOMMU's responsibility to send actual
unmapping notifications, e.g., vtd_address_space_unmap_in_dirty_tracking().
Signed-off-by: Zhenzhong Duan <[email protected]>
Tested-by: Giovannio Cabiddu <[email protected]>
Reviewed-by: Yi Liu <[email protected]>
Link: https://lore.kernel.org/qemu-devel/20251218062643.624796-8-
[email protected]
Signed-off-by: Cédric Le Goater <[email protected]>
---
hw/vfio/listener.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index
62699cb772d786c1510318dff73973ef4d297177..813621f22f8b5ec284388f9c5f71
9525ec5f282c 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -713,14 +713,34 @@ static void vfio_listener_region_del(MemoryListener
*listener,
if (try_unmap) {
bool unmap_all = false;
+ IOMMUTLBEntry entry = {}, *iotlb = NULL;
if (int128_eq(llsize, int128_2_64())) {
assert(!iova);
unmap_all = true;
llsize = int128_zero();
}
+
+ /*
+ * Fake an IOTLB entry for identity mapping which is needed by dirty
+ * tracking when switch out of PT domain. In fact, in unmap_bitmap,
+ * only translated_addr field is used to set dirty bitmap.
+ *
+ * Note: When switch into PT domain from DMA domain, the whole
IOMMU
+ * MR is deleted without iotlb, before that happen, we depend on
+ * vIOMMU to send unmap notification with accurate iotlb entry to
+ * VFIO. See vtd_address_space_unmap_in_dirty_tracking() for example,
+ * it is triggered during switching to block domain because vtd does
+ * not support direct switching from DMA to PT domain.
+ */
+ if (global_dirty_tracking && memory_region_is_ram(section->mr)) {
+ entry.iova = iova;
+ entry.translated_addr = iova;
In an experiment involving a nested setup with L1 + intel_iommu,
an L2 VM with an assigned igb VF can trigger the following QEMU
segv :
bitmap_set_atomic(map=NULL, start=1, nr=1)
physical_memory_set_dirty_range(start=0x380004040000, length=4096)
physical_memory_set_dirty_lebitmap(start=0x380004040000, pages=3)
vfio_container_query_dirty_bitmap(translated_addr=0x380004040000)
vfio_legacy_dma_unmap_one(iova=0x380004040000, size=12288)
vfio_listener_region_del()
Hmm, looks a range in PCI space, I suspect it's a PCI bar but it has 3 pages.
It's the nested VT-d interrupt remapping table.
Is igb VF a real hw passthrough to L1, then L1 pass it to L2?
Iit's an L1 emulated igb VF assigned to an L2.
Seems L2 crash, not L1?
yes. It occured a few times and I can't reproduce anymore. hmm.
That seemed to fix it
- entry.translated_addr = iova;
+ entry.translated_addr = memory_region_get_ram_addr(section->mr) +
+ section->offset_within_region;
Thanks,
C.
The VT-d interrupt-remapping table is mapped pretty high and
translated_addr ends up as a very large value. It seems that
the correct value should be :
translated_addr = memory_region_get_ram_addr(section->mr) +
section->offset_within_region;
as found in vfio_iommu_map_dirty_notify() and
and vfio_ram_discard_query_dirty_bitmap().
Is that correct ?
Good catch, it looks we lack a conversion from iotlb->translated_addr to
dirty_block offset just like in vfio_iommu_map_dirty_notify().
iotlb->translated_addr is designed to hold translation result from vIOMMU. How
about below(untested)?
--- a/hw/vfio/container-legacy.c
+++ b/hw/vfio/container-legacy.c
@@ -71,7 +71,7 @@ static int vfio_ram_block_discard_disable(VFIOLegacyContainer
*container,
static int
vfio_legacy_dma_unmap_get_dirty_bitmap(const VFIOLegacyContainer *container,
hwaddr iova, uint64_t size,
- IOMMUTLBEntry *iotlb)
+ hwaddr translated_addr)
{
const VFIOContainer *bcontainer = VFIO_IOMMU(container);
struct vfio_iommu_type1_dma_unmap *unmap;
@@ -109,8 +109,8 @@ vfio_legacy_dma_unmap_get_dirty_bitmap(const
VFIOLegacyContainer *container,
ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
if (!ret) {
- physical_memory_set_dirty_lebitmap(vbmap.bitmap,
- iotlb->translated_addr, vbmap.pages);
+ physical_memory_set_dirty_lebitmap(vbmap.bitmap, translated_addr,
+ vbmap.pages);
} else {
error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
}
@@ -134,16 +134,27 @@ static int vfio_legacy_dma_unmap_one(const
VFIOLegacyContainer *container,
.size = size,
};
bool need_dirty_sync = false;
+ MemoryRegion *mr;
+ hwaddr translated_addr, xlat;
int ret;
Error *local_err = NULL;
g_assert(!cpr_is_incoming());
+ RCU_READ_LOCK_GUARD();
+
+ mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
+ if (!mr) {
+ return -EINVAL;
+ }
+
+ translated_addr = memory_region_get_ram_addr(mr) + xlat;
+
if (iotlb && vfio_container_dirty_tracking_is_started(bcontainer)) {
if (!vfio_container_devices_dirty_tracking_is_supported(bcontainer) &&
bcontainer->dirty_pages_supported) {
return vfio_legacy_dma_unmap_get_dirty_bitmap(container, iova,
size,
- iotlb);
+ translated_addr);
}
need_dirty_sync = true;
@@ -155,7 +166,7 @@ static int vfio_legacy_dma_unmap_one(const
VFIOLegacyContainer *container,
if (need_dirty_sync) {
ret = vfio_container_query_dirty_bitmap(bcontainer, iova, size, 0,
- iotlb->translated_addr, &local_err);
+ translated_addr, &local_err);
if (ret) {
error_report_err(local_err);
return ret;
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 68f2ae6f9f..969629b0f5 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -65,6 +65,8 @@ static int iommufd_cdev_unmap(const VFIOContainer *bcontainer,
IOMMUFDBackend *be = container->be;
uint32_t ioas_id = container->ioas_id;
bool need_dirty_sync = false;
+ hwaddr translated_addr, xlat;
+ MemoryRegion *mr;
Error *local_err = NULL;
int ret, unmap_ret;
@@ -72,12 +74,21 @@ static int iommufd_cdev_unmap(const VFIOContainer
*bcontainer,
size = UINT64_MAX;
}
+ RCU_READ_LOCK_GUARD();
+
+ mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
+ if (!mr) {
+ return -EINVAL;
+ }
+
+ translated_addr = memory_region_get_ram_addr(mr) + xlat;
+
if (iotlb && vfio_container_dirty_tracking_is_started(bcontainer)) {
if (!vfio_container_devices_dirty_tracking_is_supported(bcontainer) &&
bcontainer->dirty_pages_supported) {
ret = vfio_container_query_dirty_bitmap(bcontainer, iova, size,
IOMMU_HWPT_GET_DIRTY_BITMAP_NO_CLEAR,
- iotlb->translated_addr,
+ translated_addr,
&local_err);
if (ret) {
error_report_err(local_err);
@@ -103,7 +114,7 @@ static int iommufd_cdev_unmap(const VFIOContainer
*bcontainer,
if (need_dirty_sync) {
ret = vfio_container_query_dirty_bitmap(bcontainer, iova, size, 0,
- iotlb->translated_addr,
+ translated_addr,
&local_err);
if (ret) {
error_report_err(local_err);
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index 109b5d61af..e1b078a7b3 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -97,8 +97,8 @@ static bool vfio_listener_skipped_section(MemoryRegionSection
*section,
* Called with rcu_read_lock held.
* The returned MemoryRegion must not be accessed after calling
rcu_read_unlock.
*/
-static MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
- Error **errp)
+MemoryRegion *vfio_translate_iotlb(IOMMUTLBEntry *iotlb, hwaddr *xlat_p,
+ Error **errp)
{
MemoryRegion *mr;
Thanks
Zhenzhong
