>-----Original Message-----
>From: Cédric Le Goater <c...@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.au...@redhat.com>
>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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....@intel.com>
>> + * Eric Auger <eric.au...@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.
>
>> + 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 */
