Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object

[Jag Raman]

> On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 0222bb4506..97de79cc36 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -705,6 +705,20 @@
>> { 'struct': 'RemoteObjectProperties',
>>   'data': { 'fd': 'str', 'devid': 'str' } }
>> 
>> +##
>> +# @VfioUserServerProperties:
>> +#
>> +# Properties for vfio-user-server objects.
>> +#
>> +# @socket: socket to be used by the libvfiouser library
>> +#
>> +# @device: the id of the device to be emulated at the server
>> +#
>> +# Since: 6.0
> 
> 6.2
> 
>> +##
>> +{ 'struct': 'VfioUserServerProperties',
>> +  'data': { 'socket': 'SocketAddress', 'device': 'str' } }
>> +
>> ##
>> # @RngProperties:
>> #
>> @@ -830,7 +844,8 @@
>>     'tls-creds-psk',
>>     'tls-creds-x509',
>>     'tls-cipher-suites',
>> -    'x-remote-object'
>> +    'x-remote-object',
>> +    'vfio-user-server'
> 
> Should it have the experimental prefix ('x-') or is the QAPI interface
> stable? I think some things to think about are whether a single process
> can host multiple device servers, whether hotplug is possible, etc. If
> the interface is stable then it must be able to accomodate future
> features (at least ones we can anticipate right now :)).

We did test out hotplug support.

We’ll get back to you on the multiple device servers scenario.

> 
>>   ] }
>> 
>> ##
>> @@ -887,7 +902,8 @@
>>       'tls-creds-psk':              'TlsCredsPskProperties',
>>       'tls-creds-x509':             'TlsCredsX509Properties',
>>       'tls-cipher-suites':          'TlsCredsProperties',
>> -      'x-remote-object':            'RemoteObjectProperties'
>> +      'x-remote-object':            'RemoteObjectProperties',
>> +      'vfio-user-server':           'VfioUserServerProperties'
>>   } }
>> 
>> ##
>> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
>> new file mode 100644
>> index 0000000000..c2a300f0ff
>> --- /dev/null
>> +++ b/hw/remote/vfio-user-obj.c
>> @@ -0,0 +1,173 @@
>> +/**
>> + * QEMU vfio-user-server 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-server,id=<id>,type=unix,path=<socket-path>,
> 
> I expected socket.type= and socket.path= based on the QAPI schema. Is
> this command-line example correct?

When I tried the “socket.path” approach, QEMU was not able to parse the
arguments. So I had to break it down to a series of individual members.

If “socket.path” is the expected way, I’ll see why the parser is not working
as expected. 

> 
>> + *             device=<pci-dev-id>
>> + *
>> + * Note that vfio-user-server object must be used with x-remote machine 
>> only.
>> + * This server could only support PCI devices for now.
>> + *
>> + * type - SocketAddress type - presently "unix" alone is supported. Required
>> + *        option
>> + *
>> + * path - named unix socket, it will be created by the server. It is
>> + *        a required option
>> + *
>> + * device - id of a device on the server, a required option. PCI devices
>> + *          alone are supported presently.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu-common.h"
>> +
>> +#include "qom/object.h"
>> +#include "qom/object_interfaces.h"
>> +#include "qemu/error-report.h"
>> +#include "trace.h"
>> +#include "sysemu/runstate.h"
>> +#include "hw/boards.h"
>> +#include "hw/remote/machine.h"
>> +#include "qapi/error.h"
>> +#include "qapi/qapi-visit-sockets.h"
>> +
>> +#define TYPE_VFU_OBJECT "vfio-user-server"
>> +OBJECT_DECLARE_TYPE(VfuObject, VfuObjectClass, VFU_OBJECT)
>> +
>> +struct VfuObjectClass {
>> +    ObjectClass parent_class;
>> +
>> +    unsigned int nr_devs;
>> +
>> +    /* Maximum number of devices the server could support */
>> +    unsigned int max_devs;
>> +};
>> +
>> +struct VfuObject {
>> +    /* private */
>> +    Object parent;
>> +
>> +    SocketAddress *socket;
>> +
>> +    char *device;
>> +};
>> +
>> +static void vfu_object_set_socket(Object *obj, Visitor *v, const char *name,
>> +                                  void *opaque, Error **errp)
>> +{
>> +    VfuObject *o = VFU_OBJECT(obj);
>> +
>> +    g_free(o->socket);
> 
> qapi_free_SocketAddress(o->socket)?

OK, will use that.

Didn’t know the visitors also define a function for free’ing. Thank you!

> 
>> +
>> +    o->socket = NULL;
>> +
>> +    visit_type_SocketAddress(v, name, &o->socket, errp);
>> +
>> +    if (o->socket->type != SOCKET_ADDRESS_TYPE_UNIX) {
>> +        error_setg(&error_abort, "vfu: Unsupported socket type - %s",
>> +                   o->socket->u.q_unix.path);
> 
> o->socket must be freed and set it to NULL again, otherwise setting the
> property appears to fail but the SocketAddress actually retains the
> invalid value.

OK, got it.

> 
>> +        return;
>> +    }
>> +
>> +    trace_vfu_prop("socket", o->socket->u.q_unix.path);
>> +}
>> +
>> +static void vfu_object_set_device(Object *obj, const char *str, Error 
>> **errp)
>> +{
>> +    VfuObject *o = VFU_OBJECT(obj);
>> +
>> +    g_free(o->device);
>> +
>> +    o->device = g_strdup(str);
>> +
>> +    trace_vfu_prop("device", str);
>> +}
>> +
>> +static void vfu_object_init(Object *obj)
>> +{
>> +    VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
>> +
>> +    if (!object_dynamic_cast(OBJECT(current_machine), TYPE_REMOTE_MACHINE)) 
>> {
>> +        error_setg(&error_abort, "vfu: %s only compatible with %s machine",
>> +                   TYPE_VFU_OBJECT, TYPE_REMOTE_MACHINE);
>> +        return;
>> +    }
>> +
>> +    if (k->nr_devs >= k->max_devs) {
>> +        error_setg(&error_abort,
>> +                   "Reached maximum number of vfio-user-server devices: %u",
>> +                   k->max_devs);
>> +        return;
>> +    }
>> +
>> +    k->nr_devs++;
>> +}
>> +
>> +static void vfu_object_finalize(Object *obj)
>> +{
>> +    VfuObjectClass *k = VFU_OBJECT_GET_CLASS(obj);
>> +    VfuObject *o = VFU_OBJECT(obj);
>> +
>> +    k->nr_devs--;
>> +
>> +    g_free(o->socket);
> 
> qapi_free_SocketAddress(o->socket)?
> 
>> +
>> +    g_free(o->device);
>> +
>> +    if (k->nr_devs == 0) {
>> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +    }
> 
> This won't work for all use cases. The user may wish to create/delete
> vhost-user servers at runtime without terminating the process when there
> are none left. An boolean option can be added in the future to control
> this behavior, so it's okay to leave this as is.

Thank you, we’ll make a note of this. I’ll add a TODO comment here to ensure we
don’t lose this thought.

--
Jag