Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object

["Cédric Le Goater" <clg@redhat.com>]

On 11/8/23 11:30, Markus Armbruster wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> Hello Markus,
>>
>> On 11/8/23 06:50, Markus Armbruster wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> On 11/2/23 08:12, Zhenzhong Duan wrote:
>>>>> From: Eric Auger <eric.auger@redhat.com>
>>>>> Introduce an iommufd object which allows the interaction
>>>>> with the host /dev/iommu device.
>>>>> The /dev/iommu can have been already pre-opened outside of qemu,
>>>>> in which case the fd can be passed directly along with the
>>>>> iommufd object:
>>>>> This allows the iommufd object to be shared accross several
>>>>> subsystems (VFIO, VDPA, ...). For example, libvirt would open
>>>>> the /dev/iommu once.
>>>>> If no fd is passed along with the iommufd object, the /dev/iommu
>>>>> is opened by the qemu code.
>>>>> The CONFIG_IOMMUFD option must be set to compile this new object.
>>>>> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>> v4: add CONFIG_IOMMUFD check, document default case
>>>>>     MAINTAINERS              |   7 ++
>>>>>     qapi/qom.json            |  22 ++++
>>>>>     include/sysemu/iommufd.h |  46 +++++++
>>>>>     backends/iommufd-stub.c  |  59 +++++++++
>>>>>     backends/iommufd.c       | 257 +++++++++++++++++++++++++++++++++++++++
>>>>>     backends/Kconfig         |   4 +
>>>>>     backends/meson.build     |   5 +
>>>>>     backends/trace-events    |  12 ++
>>>>>     qemu-options.hx          |  13 ++
>>>>>     9 files changed, 425 insertions(+)
>>>>>     create mode 100644 include/sysemu/iommufd.h
>>>>>     create mode 100644 backends/iommufd-stub.c
>>>>>     create mode 100644 backends/iommufd.c
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index cd8d6b140f..6f35159255 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -2135,6 +2135,13 @@ F: hw/vfio/ap.c
>>>>>     F: docs/system/s390x/vfio-ap.rst
>>>>>     L: qemu-s390x@nongnu.org
>>>>>     +iommufd
>>>>> +M: Yi Liu <yi.l.liu@intel.com>
>>>>> +M: Eric Auger <eric.auger@redhat.com>
>>>>> +S: Supported
>>>>> +F: backends/iommufd.c
>>>>> +F: include/sysemu/iommufd.h
>>>>> +
>>>>>     vhost
>>>>>     M: Michael S. Tsirkin <mst@redhat.com>
>>>>>     S: Supported
>>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>>> index c53ef978ff..27300add48 100644
>>>>> --- a/qapi/qom.json
>>>>> +++ b/qapi/qom.json
>>>>> @@ -794,6 +794,24 @@
>>>>>     { 'struct': 'VfioUserServerProperties',
>>>>>       'data': { 'socket': 'SocketAddress', 'device': 'str' } }
>>>>> +##
>>>>> +# @IOMMUFDProperties:
>>>>> +#
>>>>> +# Properties for iommufd objects.
>>>>> +#
>>>>> +# @fd: file descriptor name previously passed via 'getfd' command,
>>>>> +#     which represents a pre-opened /dev/iommu.  This allows the
>>>>> +#     iommufd object to be shared accross several subsystems
>>>>> +#     (VFIO, VDPA, ...), and the file descriptor to be shared
>>>>> +#     with other process, e.g. DPDK.  (default: QEMU opens
>>>>> +#     /dev/iommu by itself)
>>>>> +#
>>>>> +# Since: 8.2
>>>>> +##
>>>>> +{ 'struct': 'IOMMUFDProperties',
>>>>> +  'data': { '*fd': 'str' },
>>>>> +  'if': 'CONFIG_IOMMUFD' }
>>>>
>>>>
>>>> Activating or not IOMMUFD on a platform is a configuration choice
>>>> and it is not a dependency on an external resource. I would make
>>>> things simpler and drop all the #ifdef in the documentation files.
>>>
>>> What exactly are you proposing?
>>
>> I would like to simplify the configuration part of this new IOMMUFD
>> feature and avoid a ./configure option to enable/disable the feature
>> since it has no external dependencies and can be compiled on all
>> platforms.
>>
>> However, we know that it only makes sense to have the IOMMUFD backend
>> on platforms s390x, aarch64, x86_64. So I am proposing as an improvement
>> to enable IOMMUFD only on these platforms with this addition :
>>
>>    imply IOMMUFD
>>
>> to hw/{i386,s390x,arm}/Kconfig files.
>>
>> This gives us the possibility to compile out the feature downstream
>> if something goes wrong, using the files under : configs/devices/.
> 
> Shouldn't we then compile out the relevant parts of the QAPI schema,
> too?

Is it possible with Kconfig options ?
  
>> Given that the IOMMUFD feature doesn't have any external dependencies
>> and that the IOMMUFD backend object is common to all platforms, I am
>> also proposing to remove all the CONFIG_IOMMUFD define usage in the
>> documentation file "qemu-options.hx" and the schema file "qapi/qom.json".
> 
> Any CONFIG_IOMMUFD left elsewhere?

There are. To expose or not vfio properties. Stuff like :

ifdef CONFIG_IOMMUFD
     DEFINE_PROP_LINK("iommufd", VFIOPCIDevice, vbasedev.iommufd,
                      TYPE_IOMMUFD_BACKEND, IOMMUFDBackend *),
#endif
     DEFINE_PROP_END_OF_LIST(),

and

#ifdef CONFIG_IOMMUFD
     object_class_property_add_str(klass, "fd", NULL, vfio_pci_set_fd);
#endif


>>> The use of 'if': 'CONFIG_IOMMUFD' in the QAPI schema enables
>>> introspection with query-qmp-schema: when ObjectType @iommufd exists,
>>> QEMU supports creating the object.  Or am I confused?
>>
>> Object iommufd should always exist since it is common to all.
>>
>> Is that acceptable ?
> 
> Perhaps the question to ask is whether a management application needs to
> know whether this version of QEMU supports iommufd objects.  If yes,
> then query-qmp-schema is an obvious way to find out.  

Thanks for reminding me of this possibility. In that case, we should
indeed avoid returning iommufd support when !CONFIG_IOMMUFD.

Can it be implemented in qapi/qom.json with Kconfig options ?

> What could go
> wrong when this returns "supported" when it actually isn't?
  
The management layer would build an invalid QEMU command line and
QEMU would return "invalid object type: iommufd"

Thanks,

C.