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

[Joao Martins]

On 02/03/2023 14:52, Cédric Le Goater wrote:
> 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.
> 

Both good points. Specially the g_malloc0 ones, though the dma unmap wouldn't be
in use for a device that supports dirty tracking. But there's one where we add
by mistake and that's the one vfio_device_feature_dma_logging_start_create(). It
shouldn't be g_malloc0 there too. The rest, except dma_unmap and type1-iommu
get_dirty_Bitmap functions, the others would argue that only happen in the
initialization.

> 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.
> 
By simplify presumably it's because vfio_bitmap_free() would be a single line
and thus avoiding the new helper and instead we would just live with the
vfio_bitmap_alloc(). I am at two minds with alloc vs init, considering we are
still allocating the actual bitmap. Still lingering more over staying with alloc
than init.