On 10/30/23 03:40, Duan, Zhenzhong wrote:
-----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.
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 */
