Hi Cédric,
>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>Sent: Thursday, October 19, 2023 8:18 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/19/23 04:29, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Cédric Le Goater <c...@redhat.com>
>>> Sent: Wednesday, October 18, 2023 4:04 PM
>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer
>and
>>> targetted interface
>>>
>>> On 10/18/23 04:41, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <c...@redhat.com>
>>>>> Sent: Tuesday, October 17, 2023 11:51 PM
>>>>> Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for
>>>>> VFIOContainer
>>> and
>>>>> targetted interface
>>>>>
>>>>> On 10/16/23 10:31, Zhenzhong Duan wrote:
>>>>>> From: Eric Auger <eric.au...@redhat.com>
>>>>>>
>>>>>> Introduce a dumb VFIOContainer base object and its targetted interface.
>>>>>> 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 | 82
>>> +++++++++++++++++++++++++++
>>>>>> 2 files changed, 84 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 34648e518e..9651cf921c 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 VFIOLegacyContainer {
>>>>>> + VFIOContainer bcontainer;
>>>>>
>>>>> That's the parent class, right ?
>>>>
>>>> Right.
>>>>
>>>>>
>>>>>> VFIOAddressSpace *space;
>>>>>> int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>>>> MemoryListener listener;
>>>>>> @@ -200,12 +202,6 @@ typedef struct VFIODisplay {
>>>>>> } dmabuf;
>>>>>> } VFIODisplay;
>>>>>>
>>>>>> -typedef struct {
>>>>>> - unsigned long *bitmap;
>>>>>> - hwaddr size;
>>>>>> - hwaddr pages;
>>>>>> -} VFIOBitmap;
>>>>>> -
>>>>>> void vfio_host_win_add(VFIOLegacyContainer *container,
>>>>>> hwaddr min_iova, hwaddr max_iova,
>>>>>> uint64_t iova_pgsizes);
>>>>>> diff --git a/include/hw/vfio/vfio-container-base.h
>>>>>> b/include/hw/vfio/vfio-
>>>>> container-base.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..afc8543d22
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/vfio/vfio-container-base.h
>>>>>> @@ -0,0 +1,82 @@
>>>>>> +/*
>>>>>> + * 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/>.
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef HW_VFIO_VFIO_BASE_CONTAINER_H
>>>>>> +#define HW_VFIO_VFIO_BASE_CONTAINER_H
>>>>>> +
>>>>>> +#include "exec/memory.h"
>>>>>> +#ifndef CONFIG_USER_ONLY
>>>>>> +#include "exec/hwaddr.h"
>>>>>> +#endif
>>>>>> +
>>>>>> +typedef struct VFIOContainer VFIOContainer;
>>>>>> +typedef struct VFIODevice VFIODevice;
>>>>>> +typedef struct VFIOIOMMUBackendOpsClass
>VFIOIOMMUBackendOpsClass;
>>>>>> +
>>>>>> +typedef struct {
>>>>>> + unsigned long *bitmap;
>>>>>> + hwaddr size;
>>>>>> + hwaddr pages;
>>>>>> +} VFIOBitmap;
>>>>>> +
>>>>>> +/*
>>>>>> + * This is the base object for vfio container backends
>>>>>> + */
>>>>>> +struct VFIOContainer {
>>>>>> + VFIOIOMMUBackendOpsClass *ops;
>>>>>
>>>>> This is unexpected.
>>>>>
>>>>> I thought that an abstract QOM model for VFIOContainer was going
>>>>> to be introduced with a VFIOContainerClass with the ops below
>>>>> (VFIOIOMMUBackendOpsClass).
>>>>>
>>>>> Then, we would call :
>>>>>
>>>>> VFIOContainerClass *vcc = VFIO_CONTAINER_GET_CLASS(container);
>>>>>
>>>>> to get the specific implementation for the current container.
>>>>>
>>>>> I don't understand the VFIOIOMMUBackendOpsClass pointer and
>>>>> TYPE_VFIO_IOMMU_BACKEND_OPS. It seems redundant.
>>>>
>>>> The original implementation was abstract QOM model. But it wasn't
>accepted,
>>>> see https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ for details.
>>>
>>> I see the idea was challenged, not rejected. I need to dig in further and
>>> this
>>> will take time.
>> Thanks for help looking into it.
>>
>> +David, Hi David, I'm digging into your concern of using QOM to abstract base
>> container and legacy VFIOContainer:
>> "The QOM class of things is visible to the user/config layer via QMP (and
>sometimes command line).
>> It doesn't necessarily correspond to guest visible differences, but it often
>> does."
>>
>> AIUI, while it's true when the QOM type includes TYPE_USER_CREATABLE
>interface,
>> otherwise, user can't create an object of this type. Only difference is user
>> will
>see
>> "object type '%s' isn't supported by object-add" instead of "invalid object
>type: %s".
>>
>> Is your expectation to not permit user to create this object or only want
>> user
>> to see "invalid object type: %s".
>> If you mean the first, then I think QOM could be suitable if we don't include
>> TYPE_USER_CREATABLE interface?
>
>I was imagining some kind of QOM hierarchy under the vfio device
>with various QOM interfaces (similar to the ops) to define the
>possible IOMMU backends. The fact that we use the IOMMUFD object
>from the command line made it more plausible. I might be mistaking.
Got your point.
This way we introduce a new QOM type "vfio-pci-iommufd" for iommufd support,
and vfio-pci keep same for legacy backend, e.g:
#qemu -object iommufd,id=iommufd0 \
-device vfio-pci-iommufd,iommufd=iommufd0,id=vfio0... \
-device vfio-pci,id=vfio1...
Below is a draft for vfio-pci based on your imagination. not pasted here the
change
for platform/ap/ccw vfio device which should be similar.
Not clear if this fully match your imagination.
diff --git a/include/hw/vfio/vfio-container-base.h
b/include/hw/vfio/vfio-container-base.h
index 5345986993..54a6ce4d73 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -124,7 +124,7 @@ DECLARE_CLASS_CHECKERS(VFIOIOMMUBackendOpsClass,
struct VFIOIOMMUBackendOpsClass {
/*< private >*/
- ObjectClass parent_class;
+ InterfaceClass parent_class;
/*< public >*/
/* required */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index edb787d3d1..829deddc7d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3725,6 +3725,11 @@ static void vfio_pci_dev_class_init(ObjectClass *klass,
void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass);
+ IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+ be_ops->dma_map = vfio_legacy_dma_map;
+ be_ops->dma_unmap = vfio_legacy_dma_unmap;
+ ...
dc->reset = vfio_pci_reset;
device_class_set_props(dc, vfio_pci_dev_properties);
@@ -3749,10 +3754,40 @@ static const TypeInfo vfio_pci_dev_info = {
.interfaces = (InterfaceInfo[]) {
{ INTERFACE_PCIE_DEVICE },
{ INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { INTERFACE_IOMMU_BACKEND_OPS },
{ }
},
};
+static Property vfio_pci_dev_iommufd_properties[] = {
+#ifdef CONFIG_IOMMUFD
+ DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
+ TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
+#endif
+};
+
+static void vfio_pci_iommufd_dev_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ IOMMU_Backend_Ops_Class *be_ops = IOMMU_BACKEND_OPS_CLASS(klass);
+
+ device_class_set_props(dc, vfio_pci_dev_iommufd_properties);
+
+ be_ops->dma_map = iommufd_map;
+ be_ops->dma_unmap = iommufd_unmap;
+ ...
+ /* Unimplemented ops */
+ be_ops->set_dirty_page_tracking = NULL;
+ ...
+}
+
+static const TypeInfo vfio_pci_iommufd_dev_info = {
+ .name = TYPE_VFIO_PCI_IOMMUFD,
+ .parent = TYPE_VFIO_PCI,
+ .instance_size = sizeof(VFIOPCIDevice),
+ .class_init = vfio_pci_iommufd_dev_class_init,
+};
+
static Property vfio_pci_dev_nohotplug_properties[] = {
DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
@@ -3770,13 +3805,20 @@ static void
vfio_pci_nohotplug_dev_class_init(ObjectClass *klass, void *data)
static const TypeInfo vfio_pci_nohotplug_dev_info = {
.name = TYPE_VFIO_PCI_NOHOTPLUG,
- .parent = TYPE_VFIO_PCI,
+ .parent = TYPE_VFIO_PCI_IOMMUFD,
.instance_size = sizeof(VFIOPCIDevice),
.class_init = vfio_pci_nohotplug_dev_class_init,
};
static void register_vfio_pci_dev_type(void)
{
+ static const TypeInfo iommu_be_ops_interface_info = {
+ .name = TYPE_IOMMU_BACKEND_OPS,
+ .parent = TYPE_INTERFACE,
+ .class_size = sizeof(VFIOIOMMUBackendOpsClass),
+ };
+
+ type_register_static(&iommu_be_ops_interface_info);
type_register_static(&vfio_pci_dev_info);
type_register_static(&vfio_pci_nohotplug_dev_info);
}
Thanks
Zhenzhong
>
>Anyhow, the series looks pretty good. There are other aspect to
>check, like are all this iommu ops well suited for the need ?
>Let's stress the models in parallel of the reviews. If we could get
>some of it for 8.2 that'd be nice. It's on top of my list now.
>
>Thanks,
>
>C.
>
>
>> Thanks
>> Zhenzhong
