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

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

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


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

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

Thanks,

C.

> 
>> There might be a way to remove the documentation also. Not a big
>> issue for now.
>>
>>
>>> +
>>>    ##
>>>    # @RngProperties:
>>>    #
>>> @@ -934,6 +952,8 @@
>>>       'input-barrier',
>>>       { 'name': 'input-linux',
>>>         'if': 'CONFIG_LINUX' },
>>> +    { 'name': 'iommufd',
>>> +      'if': 'CONFIG_IOMMUFD' },
>>>       'iothread',
>>>       'main-loop',
>>>       { 'name': 'memory-backend-epc',
>>> @@ -1003,6 +1023,8 @@
>>>         'input-barrier':              'InputBarrierProperties',
>>>         'input-linux':                { 'type': 'InputLinuxProperties',
>>>                                          'if': 'CONFIG_LINUX' },
>>> +      'iommufd':                    { 'type': 'IOMMUFDProperties',
>>> +                                      'if': 'CONFIG_IOMMUFD' },
>>>         'iothread':                   'IothreadProperties',
>>>         'main-loop':                  'MainLoopProperties',
>>>         'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
> 
> [...]
>