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

[Cédric Le Goater]

On 10/26/23 12:30, Zhenzhong Duan wrote:
> Introduce a dumb VFIOContainer base object and its targetted interface.

targeted


> 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

> + */
> +
> +#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.


> +
> +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 ?

> +    int (*attach_device)(char *name, VFIODevice *vbasedev,

cont char *name  ?

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 */