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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Markus Armbruster <armbru@redhat.com>
>Sent: Thursday, November 9, 2023 5:05 PM
>Subject: Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object
>
>Cédric Le Goater <clg@redhat.com> writes:
>
>> 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 ?
>
>See below.
>
>>>> 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 ?
>
>The only tool we have for configuring the schema is the 'if'
>conditional.  'if': 'CONFIG_IOMMUFD' compiles to #if
>defined(CONFIG_IOMMUFD) ... #endif.  Your use of #ifdef CONFIG_IOMMUFD
>above suggests this is fine here.
>
>Symbols that are only defined in target-dependent compiles (see
>exec/poison.h) can only be used in target-dependent schema modules,
>i.e. the *-target.json.

I'm fresh on Kconfig & qapi, but I have a weak idea:
Remove conditional check for backends/iommufd.c, like:

system_ss.add(files('iommufd.c'))

Then iommufd object is common and always supported, we will not see
"invalid object type: iommufd", even for platform other than i386,s390x,arm.

On those platform not supporting iommufd, we can create an iommufd object
which is dummy, as no one will link to it to open /dev/iommufd

Thanks
Zhenzhong

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