Re: [PATCH v3 24/37] backends/iommufd: Introduce the iommufd object

[Markus Armbruster]

"Duan, Zhenzhong" <zhenzhong.duan@intel.com> writes:

>>-----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Thursday, October 26, 2023 9:28 PM
>> Subject: Re: [PATCH v3 24/37] backends/iommufd: Introduce the iommufd object
>>
>> Zhenzhong Duan <zhenzhong.duan@intel.com> writes:
>>
>>> 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>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>  MAINTAINERS              |   7 +
>>>  qapi/qom.json            |  20 +++
>>>  include/sysemu/iommufd.h |  46 +++++++
>>>  backends/iommufd-stub.c  |  59 +++++++++
>>>  backends/iommufd.c       | 268 +++++++++++++++++++++++++++++++++++++++
>>>  backends/Kconfig         |   4 +
>>>  backends/meson.build     |   5 +
>>>  backends/trace-events    |  12 ++
>>>  qemu-options.hx          |  13 ++
>>>  9 files changed, 434 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 7f9912baa0..7aa57ab16f 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2109,6 +2109,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..ef0b50f107 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -794,6 +794,22 @@
>>>  { '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
>>
>> Two spaces between sentences for consistency, please.
>
> Presume you mean "  'data': { '*fd': 'str' } }" line, not above line.

I'd like you to format the doc comment like this:


     # @fd: file descriptor name previously passed via 'getfd' command,
     #     which represents a pre-opened /dev/iommu.  This allows the

Note the two spaces after the period in "/dev/iommu.  This allows".

>>
>>> +#     iommufd object to be shared accross several subsystems
>>> +#     (VFIO, VDPA, ...) and file descriptor to be shared with
>>
>> Comma before "and file".
>>
>> Either "the file descriptor", or "file descriptors".
>>
>>> +#     other process, e.g: DPDK.
>>
>> e.g.
>>
>> Alternatively "such as DPDK."
>
> Will fix.
>
>>
>>> +#
>>> +# Since: 8.2
>>> +##
>>> +{ 'struct': 'IOMMUFDProperties',
>>> +        'data': { '*fd': 'str' } }
>>
>> @fd is optional.  How does the iommufd object behave when @fd is absent?
>
> If no fd is passed along with the iommufd object, the /dev/iommu
> is opened by the qemu code. Let me know if this also needs to be documented.

When something is optional, we pretty much always need to document
behavior when it's absent.

Ideally, behavior when absent is identical to behavior when present with
a certain default value.  Then documenting the default value suffices.
We commonly do this like (default: VALUE).

>>> +
>>>  ##
>>>  # @RngProperties:
>>>  #
>>> @@ -934,6 +950,8 @@
>>   ##
>>   # @ObjectType:
>>
>> None of the enum members are documented.  I'm not asking you to fix that
>> now.
>>
>>   #
>>   # Features:
>>   #
>>   # @unstable: Member @x-remote-object is experimental.
>>   #
>>   # Since: 6.0
>>   ##
>>   { 'enum': 'ObjectType',
>>     'data': [
>>       [...]
>>>      'input-barrier',
>>>      { 'name': 'input-linux',
>>>        'if': 'CONFIG_LINUX' },
>>> +    { 'name': 'iommufd',
>>> +      'if': 'CONFIG_IOMMUFD' },
>>
>> Should struct IOMMUFDProperties also have 'if': 'CONFIG_IOMMUFD'?
>
> Yes, thanks for point out, will fix.

You're welcome!