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. 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.
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
