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? Thanks,
Alex
