Re: [PATCH RFC server v2 02/11] vfio-user: define vfio-user object

[Jag Raman <jag.raman@oracle.com>]

> On Sep 8, 2021, at 8:37 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Fri, Aug 27, 2021 at 01:53:21PM -0400, Jagannathan Raman wrote:
>> Define vfio-user object which is remote process server for QEMU. Setup
>> object initialization functions and properties necessary to instantiate
>> the object
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> ---
>> qapi/qom.json             |  20 ++++++-
>> hw/remote/vfio-user-obj.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++
>> MAINTAINERS               |   1 +
>> hw/remote/meson.build     |   1 +
>> hw/remote/trace-events    |   3 +
>> 5 files changed, 168 insertions(+), 2 deletions(-)
>> create mode 100644 hw/remote/vfio-user-obj.c
>> 
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index a25616b..3e941ee 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -689,6 +689,20 @@
>>   'data': { 'fd': 'str', 'devid': 'str' } }
>> 
>> ##
>> +# @VfioUserProperties:
>> +#
>> +# Properties for vfio-user objects.
>> +#
>> +# @socket: path to be used as socket by the libvfiouser library
>> +#
>> +# @devid: the id of the device to be associated with the file descriptor
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'struct': 'VfioUserProperties',
>> +  'data': { 'socket': 'str', 'devid': 'str' } }
> 
> Please use 'SocketAddress' for socket instead of 'str'. That way file
> descriptor passing is easy to support and additional socket address
> families can be supported in the future.

OK, will do.

> 
>> +
>> +##
>> # @RngProperties:
>> #
>> # Properties for objects of classes derived from rng.
>> @@ -812,7 +826,8 @@
>>     'tls-creds-psk',
>>     'tls-creds-x509',
>>     'tls-cipher-suites',
>> -    'x-remote-object'
>> +    'x-remote-object',
>> +    'vfio-user'
>>   ] }
>> 
>> ##
>> @@ -868,7 +883,8 @@
>>       'tls-creds-psk':              'TlsCredsPskProperties',
>>       'tls-creds-x509':             'TlsCredsX509Properties',
>>       'tls-cipher-suites':          'TlsCredsProperties',
>> -      'x-remote-object':            'RemoteObjectProperties'
>> +      'x-remote-object':            'RemoteObjectProperties',
>> +      'vfio-user':                  'VfioUserProperties'
> 
> "vfio-user" doesn't communicate whether this is a client or server. Is
> "vfio-user-server" clearer?

“vfio-user-server” sounds better.

> 
>>   } }
>> 
>> ##
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> new file mode 100644
>> index 0000000..4a1e297
>> --- /dev/null
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -0,0 +1,145 @@
>> +/**
>> + * QEMU vfio-user server object
>> + *
>> + * Copyright © 2021 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL-v2, version 2 or later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +/**
>> + * Usage: add options:
>> + *     -machine x-remote
>> + *     -device <PCI-device>,id=<pci-dev-id>
>> + *     -object vfio-user,id=<id>,socket=<socket-path>,devid=<pci-dev-id>
> 
> I suggest renaming devid= to device= or pci-device= (similar to drive=
> and netdev=) for consistency and to avoid confusion with PCI Device IDs.

OK, will do.

> 
>> diff --git a/hw/remote/meson.build b/hw/remote/meson.build
>> index fb35fb8..cd44dfc 100644
>> --- a/hw/remote/meson.build
>> +++ b/hw/remote/meson.build
>> @@ -6,6 +6,7 @@ remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
>> remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
>> remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('proxy.c'))
>> remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('iohub.c'))
>> +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('vfio-user-obj.c'))
> 
> If you use CONFIG_VFIO_USER_SERVER then it's easier to separate mpqemu
> from vfio-user. Sharing CONFIG_MULTIPROCESS could become messy later.

OK, got it.

--
Jag