Cédric Le Goater <c...@redhat.com> writes:
> On 11/8/23 11:30, Markus Armbruster wrote:
>> Cédric Le Goater <c...@redhat.com> writes:
>>
>>> Hello Markus,
>>>
>>> On 11/8/23 06:50, Markus Armbruster wrote:
>>>> Cédric Le Goater <c...@redhat.com> writes:
>>>>
>>>>> On 11/2/23 08:12, Zhenzhong Duan wrote:
>>>>>> From: Eric Auger <eric.au...@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.william...@redhat.com>
>>>>>> Signed-off-by: Eric Auger <eric.au...@redhat.com>
>>>>>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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-s3...@nongnu.org
>>>>>> +iommufd
>>>>>> +M: Yi Liu <yi.l....@intel.com>
>>>>>> +M: Eric Auger <eric.au...@redhat.com>
>>>>>> +S: Supported
>>>>>> +F: backends/iommufd.c
>>>>>> +F: include/sysemu/iommufd.h
>>>>>> +
>>>>>> vhost
>>>>>> M: Michael S. Tsirkin <m...@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.
>> 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.
