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 <avihaih@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);
ret = -E2BIG;
goto unmap_exit;
}
- bitmap->data = g_try_malloc0(bitmap->size);
- if (!bitmap->data) {
- ret = -ENOMEM;
- goto unmap_exit;
- }
-
ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
if (!ret) {
- cpu_physical_memory_set_dirty_lebitmap((unsigned long *)bitmap->data,
- iotlb->translated_addr, pages);
+ cpu_physical_memory_set_dirty_lebitmap(vbmap->bitmap,
+ iotlb->translated_addr, vbmap->pages);
} else {
error_report("VFIO_UNMAP_DMA with DIRTY_BITMAP : %m");
}
- g_free(bitmap->data);
unmap_exit:
g_free(unmap);
+ vfio_bitmap_dealloc(vbmap);
+
return ret;
}
@@ -1282,7 +1315,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
{
struct vfio_iommu_type1_dirty_bitmap *dbitmap;
struct vfio_iommu_type1_dirty_bitmap_get *range;
- uint64_t pages;
+ VFIOBitmap *vbmap;
int ret;
if (!container->dirty_pages_supported) {
@@ -1292,6 +1325,11 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
return 0;
}
+ vbmap = vfio_bitmap_alloc(size);
+ if (!vbmap) {
+ return -errno;
+ }
+
dbitmap = g_malloc0(sizeof(*dbitmap) + sizeof(*range));
dbitmap->argsz = sizeof(*dbitmap) + sizeof(*range);
@@ -1306,15 +1344,8 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
* to qemu_real_host_page_size.
*/
range->bitmap.pgsize = qemu_real_host_page_size();
-
- 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);
- if (!range->bitmap.data) {
- ret = -ENOMEM;
- goto err_out;
- }
+ range->bitmap.size = vbmap->size;
+ range->bitmap.data = (__u64 *)vbmap->bitmap;
ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, dbitmap);
if (ret) {
@@ -1325,14 +1356,14 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
goto err_out;
}
- cpu_physical_memory_set_dirty_lebitmap((unsigned long *)range->bitmap.data,
- ram_addr, pages);
+ cpu_physical_memory_set_dirty_lebitmap(vbmap->bitmap, ram_addr,
+ vbmap->pages);
trace_vfio_get_dirty_bitmap(container->fd, range->iova, range->size,
range->bitmap.size, ram_addr);
err_out:
- g_free(range->bitmap.data);
g_free(dbitmap);
+ vfio_bitmap_dealloc(vbmap);
return ret;
}
--
2.26.3On Thu, 26 Jan 2023 20:49:35 +0200
Avihai Horon <avihaih@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 <avihaih@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
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 <avihaih@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 <avihaih@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?
Thanks.
On Sun, 12 Feb 2023 17:36:49 +0200
Avihai Horon <avihaih@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 <avihaih@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 <avihaih@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
© 2016 - 2026 Red Hat, Inc.