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.
Is igb VF a real hw passthrough to L1, then L1 pass it to L2?
Seems L2 crash, not L1?
>
>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
