RE: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and targetted interface

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Monday, October 23, 2023 11:29 PM
><david@gibson.dropbear.id.au>
>Subject: Re: [PATCH v2 02/27] vfio: Introduce base object for VFIOContainer and
>targetted interface
>
>On 10/20/23 10:19, Eric Auger wrote:
>> Hi,
>> On 10/20/23 07:48, Duan, Zhenzhong wrote:
>>> Hi Cédric,
>>>
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@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 <clg@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 <clg@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.auger@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.auger@redhat.com>
>>>>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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.liu@intel.com>
>>>>>>>>> + *          Eric Auger <eric.auger@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.
>
>yes.
>
>I took a closer look at the first part which adds the backend ops,
>including patch 19 adding the iommufd backend, not saying that I have
>identified all the dark corners.
>
>A QOM-like design would have introduced a VFIOLegacyContainer,
>inheriting from VFIOContainer (same for VFIOIOMMUFDContainer) with a
>VFIOContainerClass to implement the specific backend ops.
>VFIOspaprContainer would have made sense also.
>
>But QOM doesn't seem well adapted for the current needs. So let's try
>a simpler approach. It seems that VFIOIOMMUBackendOpsClass is
>useless. IMO, it could be a callbacks structure like we have for
>memory regions initialized with vfio_container_init(). This would
>remove some noise around the QOM typeinfo definitions.

Yes, good suggestion, will do.

>
>'struct vfio_iommu_ops' reads/sounds like a good name.
>
>Can we try that in a v3 ? It should not be such an earthquake.

Sure.

>
>spapr has some singularities which would be good to isolate in a
>vfio_iommu_spapr_ops to remove all the VFIO_SPAPR_TCE_*_IOMMU code in
>container.c. vfio_legacy_{add,del}_section_window are SPAPR specific.

Yes, let me try it.

Thanks
Zhenzhong
>
>FYI, I did some adjustements bc of the recent introduction of iova_ranges
>in my branch :
>
>  https://github.com/legoater/qemu/commits/vfio-8.2
>
>Thanks,
>
>C.
>