On Sun, 12 Feb 2023 17:36:49 +0200 Avihai Horon <avih...@nvidia.com> wrote:
> On 27/01/2023 23:11, Alex Williamson wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, 26 Jan 2023 20:49:35 +0200 > > Avihai Horon <avih...@nvidia.com> wrote: > > > >> There are already two places where dirty page bitmap allocation and > >> calculations are done in open code. With device dirty page tracking > >> being added in next patches, there are going to be even more places. > >> > >> To avoid code duplication, introduce VFIOBitmap struct and corresponding > >> alloc and dealloc functions and use them where applicable. > >> > >> Signed-off-by: Avihai Horon <avih...@nvidia.com> > >> --- > >> hw/vfio/common.c | 89 ++++++++++++++++++++++++++++++++---------------- > >> 1 file changed, 60 insertions(+), 29 deletions(-) > >> > >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> index 8e8ffbc046..e554573eb5 100644 > >> --- a/hw/vfio/common.c > >> +++ b/hw/vfio/common.c > >> @@ -319,6 +319,41 @@ const MemoryRegionOps vfio_region_ops = { > >> * Device state interfaces > >> */ > >> > >> +typedef struct { > >> + unsigned long *bitmap; > >> + hwaddr size; > >> + hwaddr pages; > >> +} VFIOBitmap; > >> + > >> +static VFIOBitmap *vfio_bitmap_alloc(hwaddr size) > >> +{ > >> + VFIOBitmap *vbmap = g_try_new0(VFIOBitmap, 1); > >> + if (!vbmap) { > >> + errno = ENOMEM; > >> + > >> + return NULL; > >> + } > >> + > >> + vbmap->pages = REAL_HOST_PAGE_ALIGN(size) / > >> qemu_real_host_page_size(); > >> + vbmap->size = ROUND_UP(vbmap->pages, sizeof(__u64) * BITS_PER_BYTE) / > >> + BITS_PER_BYTE; > >> + vbmap->bitmap = g_try_malloc0(vbmap->size); > >> + if (!vbmap->bitmap) { > >> + g_free(vbmap); > >> + errno = ENOMEM; > >> + > >> + return NULL; > >> + } > >> + > >> + return vbmap; > >> +} > >> + > >> +static void vfio_bitmap_dealloc(VFIOBitmap *vbmap) > >> +{ > >> + g_free(vbmap->bitmap); > >> + g_free(vbmap); > >> +} > >> + > >> bool vfio_mig_active(void) > >> { > >> VFIOGroup *group; > >> @@ -421,9 +456,14 @@ static int vfio_dma_unmap_bitmap(VFIOContainer > >> *container, > >> { > >> struct vfio_iommu_type1_dma_unmap *unmap; > >> struct vfio_bitmap *bitmap; > >> - uint64_t pages = REAL_HOST_PAGE_ALIGN(size) / > >> qemu_real_host_page_size(); > >> + VFIOBitmap *vbmap; > >> int ret; > >> > >> + vbmap = vfio_bitmap_alloc(size); > >> + if (!vbmap) { > >> + return -errno; > >> + } > >> + > >> unmap = g_malloc0(sizeof(*unmap) + sizeof(*bitmap)); > >> > >> unmap->argsz = sizeof(*unmap) + sizeof(*bitmap); > >> @@ -437,35 +477,28 @@ static int vfio_dma_unmap_bitmap(VFIOContainer > >> *container, > >> * qemu_real_host_page_size to mark those dirty. Hence set > >> bitmap_pgsize > >> * to qemu_real_host_page_size. > >> */ > >> - > >> bitmap->pgsize = qemu_real_host_page_size(); > >> - bitmap->size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) / > >> - BITS_PER_BYTE; > >> + bitmap->size = vbmap->size; > >> + bitmap->data = (__u64 *)vbmap->bitmap; > >> > >> - if (bitmap->size > container->max_dirty_bitmap_size) { > >> - error_report("UNMAP: Size of bitmap too big 0x%"PRIx64, > >> - (uint64_t)bitmap->size); > >> + if (vbmap->size > container->max_dirty_bitmap_size) { > >> + error_report("UNMAP: Size of bitmap too big 0x%"PRIx64, > >> vbmap->size); > > Why not pass the container to the alloc function so we can test this > > consistently for each bitmap we allocate? > > Hi, sorry for the delay. > > This test is relevant only for VFIO IOMMU dirty tracking. With device > dirty tracking it should be skipped. > Do you think we should still move it to the alloc function? Ah, ok. Sounds like we'll have to live with a separate test for the container path. Thanks, Alex