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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6257 bytes --]

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 :)).

>    ] }
>  
>  ##
> @@ -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?

> + *             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)?

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]