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

[Markus Armbruster]

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?

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

>> 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.  What could go
wrong when this returns "supported" when it actually isn't?