>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Sent: Friday, October 20, 2023 4:19 PM
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>Hi,
>On 10/20/23 07:48, Duan, Zhenzhong wrote:
>> 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...
>you would need to do that for all types for vfio devices, ap, ccw,
>platform. Looks heavy to me.
Yes, this is a try out following Cédric's imagination, pasted here just for
discuss.
>Why would we need to use a different
>vfio-pci-* device while we could switch the iommu backend according to
>the "iommufd" prop presence. The initial discussion was about QOMyfying
>the container instead.
We indeed are switching backend according to "iommufd" prop though not
QOMtfying the container in this series.
Thanks
Zhenzhong
>
>Thanks
>
>Eric
>
>>
>> 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
