>-----Original Message-----
>From: Cédric Le Goater <[email protected]>
>Subject: Re: [PULL 28/41] vfio/listener: Add missing dirty tracking in
>region_del
>
>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.
It can be reproduced with below steps:
1. Start guest with VFIO device in legacy iommu mode, then start dirty rate
tracking.
(qemu) calc_dirty_rate -b 60
2. disable VFIO device bar region by clear bit1 in COMMAND register in guest.
# setpci -s 01:00.0 COMMAND=0005
3. below trace triggers:
vfio_listener_region_del region_del 0xe0400020000 - 0xe0400021fff
Thread 10 "CPU 6/KVM" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffde5dfb640 (LWP 217374)]
0x000055555613c8a0 in bitmap_set_atomic (map=0x0, start=32, nr=1) at
../util/bitmap.c:213
213 qatomic_or(p, mask_to_set);
(gdb) bt
#0 0x000055555613c8a0 in bitmap_set_atomic (map=0x0, start=32, nr=1) at
../util/bitmap.c:213
#1 0x0000555555d6a0e2 in physical_memory_set_dirty_range
(start=15410342789120, length=4096, mask=5 '\005') at ../system/physmem.c:1042
#2 0x0000555555d6b2c9 in physical_memory_set_dirty_lebitmap
(bitmap=0x7ffdc80ac690, start=15410342789120, pages=2) at
../system/physmem.c:1305
#3 0x0000555555ca97ee in vfio_legacy_dma_unmap_get_dirty_bitmap
(container=0x5555576fc980, iova=15410342789120, size=8192,
iotlb=0x7ffde5df9e20) at ../hw/vfio/container-legacy.c:112
#4 0x0000555555ca9927 in vfio_legacy_dma_unmap_one (container=0x5555576fc980,
iova=15410342789120, size=8192, flags=0, iotlb=0x7ffde5df9e20) at
../hw/vfio/container-legacy.c:145
#5 0x0000555555ca9aba in vfio_legacy_dma_unmap (bcontainer=0x5555576fc980,
iova=15410342789120, size=8192, iotlb=0x7ffde5df9e20, unmap_all=false) at
../hw/vfio/container-legacy.c:196
#6 0x0000555555ca84b5 in vfio_container_dma_unmap (bcontainer=0x5555576fc980,
iova=15410342789120, size=8192, iotlb=0x7ffde5df9e20, unmap_all=false) at
../hw/vfio/container.c:131
#7 0x0000555555ca68cd in vfio_listener_region_del (listener=0x5555576fc9b0,
section=0x7ffde5df9ed0) at ../hw/vfio/listener.c:753
#8 0x0000555555d5d9a9 in address_space_update_topology_pass (as=0x5555574c1880
<address_space_memory>, old_view=0x7ffd7c001580, new_view=0x7ffdc8000c00,
adding=false)
>
>That seemed to fix it
>
>- entry.translated_addr = iova;
>+ entry.translated_addr = memory_region_get_ram_addr(section->mr) +
>+ section->offset_within_region;
I verified this works.
>
>
>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;
Above change needs below tweak to work:
@@ -134,7 +134,6 @@ 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;
@@ -143,14 +142,15 @@ static int vfio_legacy_dma_unmap_one(const
VFIOLegacyContainer *container,
RCU_READ_LOCK_GUARD();
- mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
- if (!mr) {
- return -EINVAL;
- }
+ if (iotlb && vfio_container_dirty_tracking_is_started(bcontainer)) {
+ MemoryRegion *mr = vfio_translate_iotlb(iotlb, &xlat, &local_err);
- translated_addr = memory_region_get_ram_addr(mr) + xlat;
+ 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,
Thanks
Zhenzhong
