Re: [PATCH v3 06/37] vfio: Introduce base object for VFIOContainer and targetted interface

["Cédric Le Goater" <clg@redhat.com>]

On 10/30/23 03:40, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Friday, October 27, 2023 10:03 PM
>> Subject: Re: [PATCH v3 06/37] vfio: Introduce base object for VFIOContainer and
>> targetted interface
>>
>> On 10/26/23 12:30, Zhenzhong Duan wrote:
>>> Introduce a dumb VFIOContainer base object and its targetted interface.
>>
>> targeted
> 
> Will fix.
> 
>>
>>
>>> This is willingly not a QOM object because we don't want it to be
>>> visible from the user interface.  The VFIOContainer will be smoothly
>>> populated in subsequent patches as well as interfaces.
>>>
>>> No fucntional change intended.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/hw/vfio/vfio-common.h         |  8 +---
>>>    include/hw/vfio/vfio-container-base.h | 64 +++++++++++++++++++++++++++
>>>    2 files changed, 66 insertions(+), 6 deletions(-)
>>>    create mode 100644 include/hw/vfio/vfio-container-base.h
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index b9c7a7e588..d8f293cb57 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -30,6 +30,7 @@
>>>    #include <linux/vfio.h>
>>>    #endif
>>>    #include "sysemu/sysemu.h"
>>> +#include "hw/vfio/vfio-container-base.h"
>>>
>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>
>>> @@ -81,6 +82,7 @@ typedef struct VFIOAddressSpace {
>>>    struct VFIOGroup;
>>>
>>>    typedef struct VFIOContainer {
>>> +    VFIOContainerBase bcontainer;
>>>        VFIOAddressSpace *space;
>>>        int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>        MemoryListener listener;
>>> @@ -201,12 +203,6 @@ typedef struct VFIODisplay {
>>>        } dmabuf;
>>>    } VFIODisplay;
>>>
>>> -typedef struct {
>>> -    unsigned long *bitmap;
>>> -    hwaddr size;
>>> -    hwaddr pages;
>>> -} VFIOBitmap;
>>> -
>>>    VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
>>>    void vfio_put_address_space(VFIOAddressSpace *space);
>>>    bool vfio_devices_all_running_and_saving(VFIOContainer *container);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>>> new file mode 100644
>>> index 0000000000..5becbd51a7
>>> --- /dev/null
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -0,0 +1,64 @@
>>> +/*
>>> + * VFIO BASE CONTAINER
>>> + *
>>> + * Copyright (C) 2023 Intel Corporation.
>>> + * Copyright Red Hat, Inc. 2023
>>> + *
>>> + * Authors: Yi Liu <yi.l.liu@intel.com>
>>> + *          Eric Auger <eric.auger@redhat.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> +
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> +
>>> + * You should have received a copy of the GNU General Public License along
>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>
>> This should be enough :
>>
>>    SPDX-License-Identifier: GPL-2.0-or-later
> 
> Will do.
> 
>>
>>> + */
>>> +
>>> +#ifndef HW_VFIO_VFIO_BASE_CONTAINER_H
>>> +#define HW_VFIO_VFIO_BASE_CONTAINER_H
>>
>> HW_VFIO_VFIO_CONTAINER_BASE_H
>>
>>> +
>>> +#include "exec/memory.h"
>>> +#ifndef CONFIG_USER_ONLY
>>> +#include "exec/hwaddr.h"
>>> +#endif
>>
>> including "exec/memory.h" should be enough.
> 
> Will do.
> 
>>
>>
>>> +
>>> +typedef struct VFIODevice VFIODevice;
>>> +typedef struct VFIOIOMMUOps VFIOIOMMUOps;
>>> +
>>> +typedef struct {
>>> +    unsigned long *bitmap;
>>> +    hwaddr size;
>>> +    hwaddr pages;
>>> +} VFIOBitmap;
>>> +
>>> +/*
>>> + * This is the base object for vfio container backends
>>> + */
>>> +typedef struct VFIOContainerBase {
>>> +    const VFIOIOMMUOps *ops;
>>> +} VFIOContainerBase;
>>> +
>>> +struct VFIOIOMMUOps {
>>> +    /* basic feature */
>>> +    int (*dma_map)(VFIOContainerBase *bcontainer,
>>> +                   hwaddr iova, ram_addr_t size,
>>> +                   void *vaddr, bool readonly);
>>> +    int (*dma_unmap)(VFIOContainerBase *bcontainer,
>>> +                     hwaddr iova, ram_addr_t size,
>>> +                     IOMMUTLBEntry *iotlb);
>>
>> Could the VFIOContainerBase *parameter be const ?
> 
> Yes, VFIOContainerBase is not changed by dma_unmap and other
> functions dma_unmap calls. I tried and found making it const here
> would impact all functions it called with same parameter be const
> in following patches which looks unrelated to the patch itself
> to avoid compile error.
> 
> E.g. below functions are impacted,
> vfio_devices_all_running_and_mig_active
> vfio_devices_all_device_dirty_tracking
> vfio_devices_query_dirty_bitmap
> vfio_container_query_dirty_bitmap
> vfio_legacy_query_dirty_bitmap>
> To make following patches cleaner to review, I would like to keep
> current code except you or others have a strong opinion.
> Another choice I can think of is to add const to all impact functions
> in a separate patch.

It would be good to have, for later.

On a related topic, I find the code difficult to read because it is
complex, of course, and dealing with a thick layer of software constructs
but also because it lacks a consistent naming scheme in the different
submodules. For instance, iommufd.c has various routine prefixes for
local routines. This is quite confusing. Same for :

   hw/vfio/common.c
   hw/vfio/helpers.c
   hw/vfio/container.c
   hw/vfio/container-base.c

This is not necessarily introduced by the changes of this series.
Something to improve for sure though.

Thanks,

C.


> 
>>
>>> +    int (*attach_device)(char *name, VFIODevice *vbasedev,
>>
>> cont char *name  ?
> 
> Yes, will fix.
> 
> Thanks
> Zhenzhong
> 
>>
>> The rest looks good to me.
>>
>> Thanks
>>
>> C.
>>
>>
>>> +                         AddressSpace *as, Error **errp);
>>> +    void (*detach_device)(VFIODevice *vbasedev);
>>> +    /* migration feature */
>>> +    int (*set_dirty_page_tracking)(VFIOContainerBase *bcontainer, bool start);
>>> +    int (*query_dirty_bitmap)(VFIOContainerBase *bcontainer, VFIOBitmap
>> *vbmap,
>>> +                              hwaddr iova, hwaddr size);
>>> +};
>>> +#endif /* HW_VFIO_VFIO_BASE_CONTAINER_H */
>