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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Markus Armbruster <armbru@redhat.com>
>Sent: Friday, October 27, 2023 4:31 PM
>Subject: Re: [PATCH v3 24/37] backends/iommufd: Introduce the iommufd object
>
>"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".

Understood.

>
>>>
>>>> +#     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).

Got it.

Thanks
Zhenzhong