[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 05/18] vfio/common: Add VFIOBitmap and (de)alloc functions
From: |
Alex Williamson |
Subject: |
Re: [PATCH 05/18] vfio/common: Add VFIOBitmap and (de)alloc functions |
Date: |
Tue, 14 Feb 2023 14:28:33 -0700 |
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