> On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi <stefa...@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
