Re: [PATCH v2 07/20] vfio/common: Add VFIOBitmap and (de)alloc functions

[Cédric Le Goater]

Hello Joao,

On 3/2/23 14:24, Joao Martins wrote:
> On 27/02/2023 14:09, Cédric Le Goater wrote:
> > On 2/22/23 18:49, Avihai Horon wrote:
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -320,6 +320,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);
> >
> > I think using g_malloc0() for the VFIOBitmap should be fine. If QEMU can
> > not allocate a couple of bytes, we are in trouble anyway.
>
> OOM situations are rather unpredictable, and switching to g_malloc0 means we
> will exit ungracefully in the middle of fetching dirty bitmaps. And this
> function (vfio_bitmap_alloc) overall will be allocating megabytes for terabyte
> guests.
>
> It would be ok if we are initializing, but this is at runtime when we do
> migration. I think we should stick with g_try_new0. exit on failure should be
> reserved to failure to switch the kernel migration state whereby we are likely
> to be dealing with a hardware failure and thus requires something more drastic.

I agree for large allocation :

    vbmap->bitmap = g_try_malloc0(vbmap->size);

but not for the smaller ones, like VFIOBitmap. You would have to
convert some other g_malloc0() calls, like the one allocating 'unmap'
in vfio_dma_unmap_bitmap(), to be consistent.

Given the size of VFIOBitmap, I think it could live on the stack in
routine vfio_dma_unmap_bitmap() and routine vfio_get_dirty_bitmap()
since the reference is not kept.

The 'vbmap' attribute of vfio_giommu_dirty_notifier does not need
to be a pointer either.

vfio_bitmap_alloc(hwaddr size) could then become
vfio_bitmap_init(VFIOBitmap *vbmap, hwaddr size).

Anyhow, this is minor. It would simplify a bit the exit path
and error handling.

Thanks,

C.