Hi Alex, On 2021/3/10 7:17, Alex Williamson wrote: > On Thu, 4 Mar 2021 21:34:46 +0800 > Kunkun Jiang <jiangkun...@huawei.com> wrote: > >> The cpu_physical_memory_set_dirty_lebitmap() can quickly deal with >> the dirty pages of memory by bitmap-traveling, regardless of whether >> the bitmap is aligned correctly or not. >> >> cpu_physical_memory_set_dirty_lebitmap() supports pages in bitmap of >> host page size. So it'd better to set bitmap_pgsize to host page size >> to support more translation granule sizes. >> >> Fixes: 87ea529c502 (vfio: Get migration capability flags for container) >> Signed-off-by: Kunkun Jiang <jiangkun...@huawei.com> >> --- >> hw/vfio/common.c | 44 ++++++++++++++++++++++---------------------- >> 1 file changed, 22 insertions(+), 22 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 6ff1daa763..69fb5083a4 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -378,7 +378,7 @@ static int vfio_dma_unmap_bitmap(VFIOContainer >> *container, >> { >> struct vfio_iommu_type1_dma_unmap *unmap; >> struct vfio_bitmap *bitmap; >> - uint64_t pages = TARGET_PAGE_ALIGN(size) >> TARGET_PAGE_BITS; >> + uint64_t pages = REAL_HOST_PAGE_ALIGN(size) / qemu_real_host_page_size; >> int ret; >> >> unmap = g_malloc0(sizeof(*unmap) + sizeof(*bitmap)); >> @@ -390,12 +390,12 @@ static int vfio_dma_unmap_bitmap(VFIOContainer >> *container, >> bitmap = (struct vfio_bitmap *)&unmap->data; >> >> /* >> - * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of >> - * TARGET_PAGE_SIZE to mark those dirty. Hence set bitmap_pgsize to >> - * TARGET_PAGE_SIZE. >> + * cpu_physical_memory_set_dirty_lebitmap() supports pages in bitmap of >> + * qemu_real_host_page_size to mark those dirty. Hence set bitmap_pgsize >> + * to qemu_real_host_page_size. > > > I don't see that this change is well supported by the code, > cpu_physical_memory_set_dirty_lebitmap() seems to operate on > TARGET_PAGE_SIZE, and the next three patch chunks take a detour through > memory listener code that seem unrelated to the change described in the > commit log. This claims to fix something, what is actually broken? Actually I have reported this bug long ago. cpu_physical_memory_set_dirty_lebitmap() expects the granule of @bitmap to be qemu_real_host_page_size, as it uses @hpratio to convert the bitmap.
Thanks, Keqian > Thanks, > > Alex > >> */ >> >> - bitmap->pgsize = TARGET_PAGE_SIZE; >> + bitmap->pgsize = qemu_real_host_page_size; >> bitmap->size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) / >> BITS_PER_BYTE; >> >> @@ -674,16 +674,16 @@ static void vfio_listener_region_add(MemoryListener >> *listener, >> return; >> } >> >> - if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) >> != >> - (section->offset_within_region & ~TARGET_PAGE_MASK))) { >> + if (unlikely((section->offset_within_address_space & >> ~qemu_real_host_page_mask) != >> + (section->offset_within_region & >> ~qemu_real_host_page_mask))) { >> error_report("%s received unaligned region", __func__); >> return; >> } >> >> - iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); >> + iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> llend = int128_make64(section->offset_within_address_space); >> llend = int128_add(llend, section->size); >> - llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK)); >> + llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask)); >> >> if (int128_ge(int128_make64(iova), llend)) { >> return; >> @@ -892,8 +892,8 @@ static void vfio_listener_region_del(MemoryListener >> *listener, >> return; >> } >> >> - if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) >> != >> - (section->offset_within_region & ~TARGET_PAGE_MASK))) { >> + if (unlikely((section->offset_within_address_space & >> ~qemu_real_host_page_mask) != >> + (section->offset_within_region & >> ~qemu_real_host_page_mask))) { >> error_report("%s received unaligned region", __func__); >> return; >> } >> @@ -921,10 +921,10 @@ static void vfio_listener_region_del(MemoryListener >> *listener, >> */ >> } >> >> - iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); >> + iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> llend = int128_make64(section->offset_within_address_space); >> llend = int128_add(llend, section->size); >> - llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK)); >> + llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask)); >> >> if (int128_ge(int128_make64(iova), llend)) { >> return; >> @@ -1004,13 +1004,13 @@ static int vfio_get_dirty_bitmap(VFIOContainer >> *container, uint64_t iova, >> range->size = size; >> >> /* >> - * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of >> - * TARGET_PAGE_SIZE to mark those dirty. Hence set bitmap's pgsize to >> - * TARGET_PAGE_SIZE. >> + * cpu_physical_memory_set_dirty_lebitmap() supports pages in bitmap of >> + * qemu_real_host_page_size to mark those dirty. Hence set bitmap's >> pgsize >> + * to qemu_real_host_page_size. >> */ >> - range->bitmap.pgsize = TARGET_PAGE_SIZE; >> + range->bitmap.pgsize = qemu_real_host_page_size; >> >> - pages = TARGET_PAGE_ALIGN(range->size) >> TARGET_PAGE_BITS; >> + pages = REAL_HOST_PAGE_ALIGN(range->size) / qemu_real_host_page_size; >> range->bitmap.size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) / >> BITS_PER_BYTE; >> range->bitmap.data = g_try_malloc0(range->bitmap.size); >> @@ -1114,7 +1114,7 @@ static int vfio_sync_dirty_bitmap(VFIOContainer >> *container, >> section->offset_within_region; >> >> return vfio_get_dirty_bitmap(container, >> - >> TARGET_PAGE_ALIGN(section->offset_within_address_space), >> + >> REAL_HOST_PAGE_ALIGN(section->offset_within_address_space), >> int128_get64(section->size), ram_addr); >> } >> >> @@ -1655,10 +1655,10 @@ static void >> vfio_get_iommu_info_migration(VFIOContainer *container, >> header); >> >> /* >> - * cpu_physical_memory_set_dirty_lebitmap() expects pages in bitmap of >> - * TARGET_PAGE_SIZE to mark those dirty. >> + * cpu_physical_memory_set_dirty_lebitmap() supports pages in bitmap of >> + * qemu_real_host_page_size to mark those dirty. >> */ >> - if (cap_mig->pgsize_bitmap & TARGET_PAGE_SIZE) { >> + if (cap_mig->pgsize_bitmap & qemu_real_host_page_size) { >> container->dirty_pages_supported = true; >> container->max_dirty_bitmap_size = cap_mig->max_dirty_bitmap_size; >> container->dirty_pgsizes = cap_mig->pgsize_bitmap; > > . >