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

2023-11-14 Thread Duan, Zhenzhong 


>-Original Message-
>From: Cédric Le Goater 
>Sent: Tuesday, November 14, 2023 5:41 PM
>Subject: Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object
>
>
>>> 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
>
>In that case, the management layer would define a crippled vfio-pci
>device. I'd rather let the error occur or find a way to move the
>"iommufd" object and properties to a target dependent file. I don't
>see how this could be done though.

I see, error occur is better than a crippled vfio-pci device. Or else we
need to teach libvirt to also check /dev/iommu existence.

Thanks
Zhenzhong

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

2023-11-14 Thread Cédric Le Goater 




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


In that case, the management layer would define a crippled vfio-pci
device. I'd rather let the error occur or find a way to move the
"iommufd" object and properties to a target dependent file. I don't
see how this could be done though.

Thanks,

C.

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

2023-11-09 Thread Duan, Zhenzhong 

>-Original Message-
>From: Markus Armbruster 
>Sent: Thursday, November 9, 2023 5:05 PM
>Subject: Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object
>
>Cédric Le Goater  writes:
>
>> On 11/8/23 11:30, Markus Armbruster wrote:
>>> Cédric Le Goater  writes:
>>>
>>>> Hello Markus,
>>>>
>>>> On 11/8/23 06:50, Markus Armbruster wrote:
>>>>> Cédric Le Goater  writes:
>>>>>
>>>>>> On 11/2/23 08:12, Zhenzhong Duan wrote:
>>>>>>> From: Eric Auger 
>>>>>>> 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 
>>>>>>> Signed-off-by: Eric Auger 
>>>>>>> Signed-off-by: Yi Liu 
>>>>>>> Signed-off-by: Zhenzhong Duan 
>>>>>>> ---
>>>>>>> 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 
>>>>>>> +M: Eric Auger 
>>>>>>> +S: Supported
>>>>>>> +F: backends/iommufd.c
>>>>>>> +F: include/sysemu/iommufd.h
>>>>>>> +
>>>>>>> vhost
>>>>>>> M: Michael S. Tsirkin 
>>>>>>> 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',
&g

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

2023-11-09 Thread Markus Armbruster 
Cédric Le Goater  writes:

> On 11/8/23 11:30, Markus Armbruster wrote:
>> Cédric Le Goater  writes:
>> 
>>> Hello Markus,
>>>
>>> On 11/8/23 06:50, Markus Armbruster wrote:
 Cédric Le Goater  writes:

> On 11/2/23 08:12, Zhenzhong Duan wrote:
>> From: Eric Auger 
>> 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 
>> Signed-off-by: Eric Auger 
>> Signed-off-by: Yi Liu 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>> 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 
>> +M: Eric Auger 
>> +S: Supported
>> +F: backends/iommufd.c
>> +F: include/sysemu/iommufd.h
>> +
>> vhost
>> M: Michael S. Tsirkin 
>> 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);

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

2023-11-08 Thread Cédric Le Goater 

On 11/8/23 11:30, Markus Armbruster wrote:

Cédric Le Goater  writes:


Hello Markus,

On 11/8/23 06:50, Markus Armbruster wrote:

Cédric Le Goater  writes:


On 11/2/23 08:12, Zhenzhong Duan wrote:

From: Eric Auger 
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 
Signed-off-by: Eric Auger 
Signed-off-by: Yi Liu 
Signed-off-by: Zhenzhong Duan 
---
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 
+M: Eric Auger 
+S: Supported
+F: backends/iommufd.c
+F: include/sysemu/iommufd.h
+
vhost
M: Michael S. Tsirkin 
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 ?
 

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 ?


What could go
wrong when this returns

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

2023-11-08 Thread Markus Armbruster 
Cédric Le Goater  writes:

> Hello Markus,
>
> On 11/8/23 06:50, Markus Armbruster wrote:
>> Cédric Le Goater  writes:
>> 
>>> On 11/2/23 08:12, Zhenzhong Duan wrote:
 From: Eric Auger 
 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 
 Signed-off-by: Eric Auger 
 Signed-off-by: Yi Liu 
 Signed-off-by: Zhenzhong Duan 
 ---
 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 
 +M: Eric Auger 
 +S: Supported
 +F: backends/iommufd.c
 +F: include/sysemu/iommufd.h
 +
vhost
M: Michael S. Tsirkin 
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?

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

2023-11-08 Thread Cédric Le Goater 

Hello Markus,

On 11/8/23 06:50, Markus Armbruster wrote:

Cédric Le Goater  writes:


On 11/2/23 08:12, Zhenzhong Duan wrote:

From: Eric Auger 
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 
Signed-off-by: Eric Auger 
Signed-off-by: Yi Liu 
Signed-off-by: Zhenzhong Duan 
---
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 
+M: Eric Auger 
+S: Supported
+F: backends/iommufd.c
+F: include/sysemu/iommufd.h
+
   vhost
   M: Michael S. Tsirkin 
   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/.


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



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 ?

Thanks,

C.




There might be a way to remove the documentation also. Not a big
issue for now.



+
   ##
   # @RngProperties:
   #
@@ -934,6 +952,8 @@
  'input-barrier',
  { 'name': 'input-linux',
'if': 'CONFIG_LINUX' },
+{ 'name': 'iommufd',
+  'if': 'CONFIG_IOMMUFD' },
  'iothread',
  'main-loop',
  { 'name': 'memory-backend-epc',
@@ -1003,6 +1023,8 @@
'input-barrier':  'InputBarrierProperties',
'input-linux':{ 'type': 'InputLinuxProperties',
 'if': 'CONFIG_LINUX' },
+  'iommufd':{ 'type': 'IOMMUFDProperties',
+  'if': 'CONFIG_IOMMUFD' },
'iothread':   'IothreadProperties',
'main-loop':  'MainLoopProperties',
'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties',


[...]

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

2023-11-08 Thread Duan, Zhenzhong 


>-Original Message-
>From: Cédric Le Goater 
>Sent: Wednesday, November 8, 2023 5:41 PM
>Subject: Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object
>
>>>> +  hwaddr iova, ram_addr_t size)
>>>> +{
>>>> +int ret;
>>>> +struct iommu_ioas_unmap unmap = {
>>>> +.size = sizeof(unmap),
>>>> +.ioas_id = ioas_id,
>>>> +.iova = iova,
>>>> +.length = size,
>>>> +};
>>>> +
>>>> +ret = ioctl(be->fd, IOMMU_IOAS_UNMAP, );
>>>> +trace_iommufd_backend_unmap_dma(be->fd, ioas_id, iova, size, ret);
>>>> +/*
>>>> + * TODO: IOMMUFD doesn't support mapping PCI BARs for now.
>>>> + * It's not a problem if there is no p2p dma, relax it here
>>>> + * and avoid many noisy trigger from vIOMMU side.
>>>
>>> Should we add a warn_report() ?
>>
>> The purpose of checking "ret && errno == ENOENT" is to avoid many
>> error_report() for PCI BARs, If we add warn_report(), there will still be
>> many print for PCI BARs.
>
>a trace event then ?

Good idea, will do.

Thanks
Zhenzhong

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

2023-11-08 Thread Cédric Le Goater 

+  hwaddr iova, ram_addr_t size)
+{
+int ret;
+struct iommu_ioas_unmap unmap = {
+.size = sizeof(unmap),
+.ioas_id = ioas_id,
+.iova = iova,
+.length = size,
+};
+
+ret = ioctl(be->fd, IOMMU_IOAS_UNMAP, );
+trace_iommufd_backend_unmap_dma(be->fd, ioas_id, iova, size, ret);
+/*
+ * TODO: IOMMUFD doesn't support mapping PCI BARs for now.
+ * It's not a problem if there is no p2p dma, relax it here
+ * and avoid many noisy trigger from vIOMMU side.


Should we add a warn_report() ?


The purpose of checking "ret && errno == ENOENT" is to avoid many
error_report() for PCI BARs, If we add warn_report(), there will still be
many print for PCI BARs.


a trace event then ?

Thanks,

C.

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

2023-11-07 Thread Markus Armbruster 
Cédric Le Goater  writes:

> On 11/2/23 08:12, Zhenzhong Duan wrote:
>> From: Eric Auger 
>> 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 
>> Signed-off-by: Eric Auger 
>> Signed-off-by: Yi Liu 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>> 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 
>> +M: Eric Auger 
>> +S: Supported
>> +F: backends/iommufd.c
>> +F: include/sysemu/iommufd.h
>> +
>>   vhost
>>   M: Michael S. Tsirkin 
>>   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?

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?

> There might be a way to remove the documentation also. Not a big
> issue for now.
>
>
>> +
>>   ##
>>   # @RngProperties:
>>   #
>> @@ -934,6 +952,8 @@
>>  'input-barrier',
>>  { 'name': 'input-linux',
>>'if': 'CONFIG_LINUX' },
>> +{ 'name': 'iommufd',
>> +  'if': 'CONFIG_IOMMUFD' },
>>  'iothread',
>>  'main-loop',
>>  { 'name': 'memory-backend-epc',
>> @@ -1003,6 +1023,8 @@
>>'input-barrier':  'InputBarrierProperties',
>>'input-linux':{ 'type': 'InputLinuxProperties',
>> 'if': 'CONFIG_LINUX' },
>> +  'iommufd':{ 'type': 'IOMMUFDProperties',
>> +  'if': 'CONFIG_IOMMUFD' },
>>'iothread':   'IothreadProperties',
>>'main-loop':  'MainLoopProperties',
>>'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties',

[...]

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

2023-11-07 Thread Duan, Zhenzhong 


>-Original Message-
>From: Cédric Le Goater 
>Sent: Tuesday, November 7, 2023 9:33 PM
>Subject: Re: [PATCH v4 26/41] backends/iommufd: Introduce the iommufd object
>
>On 11/2/23 08:12, Zhenzhong Duan wrote:
>> From: Eric Auger 
[...]
>> 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.

Yes, that will be cleaner.

>
>There might be a way to remove the documentation also. Not a big
>issue for now.
[...]
>> diff --git a/backends/iommufd-stub.c b/backends/iommufd-stub.c
>
>I don't think this stub file is needed. Please drop.

Will do.

>
>> new file mode 100644
>> index 00..02ac844c17
>> --- /dev/null
>> +++ b/backends/iommufd-stub.c
>> @@ -0,0 +1,59 @@
>> +/*
>> + * iommufd container backend stub
>> + *
>> + * Copyright (C) 2023 Intel Corporation.
>> + * Copyright Red Hat, Inc. 2023
>> + *
>> + * Authors: Yi Liu 
>> + *  Eric Auger 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> +
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> +
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "sysemu/iommufd.h"
>> +#include "qemu/error-report.h"
>> +
>> +int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp)
>> +{
>> +return 0;
>> +}
>> +void iommufd_backend_disconnect(IOMMUFDBackend *be)
>> +{
>> +}
>> +void iommufd_backend_free_id(int fd, uint32_t id)
>> +{
>> +}
>> +int iommufd_backend_get_ioas(IOMMUFDBackend *be, uint32_t *ioas_id)
>> +{
>> +return 0;
>> +}
>> +void iommufd_backend_put_ioas(IOMMUFDBackend *be, uint32_t ioas_id)
>> +{
>> +}
>> +int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>hwaddr iova,
>> +ram_addr_t size, void *vaddr, bool readonly)
>> +{
>> +return 0;
>> +}
>> +int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>> +  hwaddr iova, ram_addr_t size)
>> +{
>> +return 0;
>> +}
>> +int iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id,
>> +   uint32_t pt_id, uint32_t *out_hwpt)
>> +{
>> +return 0;
>> +}
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> new file mode 100644
>> index 00..a526d58824
>> --- /dev/null
>> +++ b/backends/iommufd.c
>> @@ -0,0 +1,257 @@
>> +/*
>> + * iommufd container backend
>> + *
>> + * Copyright (C) 2023 Intel Corporation.
>> + * Copyright Red Hat, Inc. 2023
>> + *
>> + * Authors: Yi Liu 
>> + *  Eric Auger 
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "sysemu/iommufd.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qmp/qerror.h"
>

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

2023-11-07 Thread Cédric Le Goater 

On 11/2/23 08:12, Zhenzhong Duan wrote:

From: Eric Auger 

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 
Signed-off-by: Eric Auger 
Signed-off-by: Yi Liu 
Signed-off-by: Zhenzhong Duan 
---
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 
+M: Eric Auger 
+S: Supported
+F: backends/iommufd.c
+F: include/sysemu/iommufd.h
+
  vhost
  M: Michael S. Tsirkin 
  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.

There might be a way to remove the documentation also. Not a big
issue for now.



+
  ##
  # @RngProperties:
  #
@@ -934,6 +952,8 @@
  'input-barrier',
  { 'name': 'input-linux',
'if': 'CONFIG_LINUX' },
+{ 'name': 'iommufd',
+  'if': 'CONFIG_IOMMUFD' },
  'iothread',
  'main-loop',
  { 'name': 'memory-backend-epc',
@@ -1003,6 +1023,8 @@
'input-barrier':  'InputBarrierProperties',
'input-linux':{ 'type': 'InputLinuxProperties',
'if': 'CONFIG_LINUX' },
+  'iommufd':{ 'type': 'IOMMUFDProperties',
+  'if': 'CONFIG_IOMMUFD' },
'iothread':   'IothreadProperties',
'main-loop':  'MainLoopProperties',
'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties',
diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
new file mode 100644
index 00..f0e5c7eeb8
--- /dev/null
+++ b/include/sysemu/iommufd.h
@@ -0,0 +1,46 @@
+#ifndef SYSEMU_IOMMUFD_H
+#define SYSEMU_IOMMUFD_H
+
+#include "qom/object.h"
+#include "qemu/thread.h"
+#include "exec/hwaddr.h"
+#include "exec/cpu-common.h"
+
+#define TYPE_IOMMUFD_BACKEND "iommufd"
+OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass,
+IOMMUFD_BACKEND)
+#define IOMMUFD_BACKEND(obj) \
+OBJECT_CHECK(IOMMUFDBackend, (obj), TYPE_IOMMUFD_BACKEND)
+#define IOMMUFD_BACKEND_GET_CLASS(obj) \
+OBJECT_GET_CLASS(IOMMUFDBackendClass, (obj), TYPE_IOMMUFD_BACKEND)
+#define IOMMUFD_BACKEND_CLASS(klass) \
+OBJECT_CLASS_CHECK(IOMMUFDBackendClass, (klass), TYPE_IOMMUFD_BACKEND)
+struct IOMMUFDBackendClass {
+ObjectClass parent_class;
+};
+
+struct IOMMUFDBackend {
+Object parent;
+
+/*< protected >*/
+int fd;/* /dev/iommu file descriptor */
+bool owned;/* is the /dev/iommu opened internally */
+QemuMutex lock;
+uint32_t users;
+
+/*< public >*/
+};
+
+int iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
+void iommufd_backend_disconnect(IOMMUFDBackend *be);
+
+int iommufd_backend_get_ioas(IOMMUFDBackend *be, uint32_t *ioas_id);
+void iommufd_backend_put_ioas(IOMMUFDBackend *be, uint32_t ioas_id);
+void iommufd_backend_free_id(int fd,