Re: [PATCH v12 10/19] multi-process: Associate fd of a PCIDevice with its object

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

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

Hi

On Wed, Dec 2, 2020 at 12:25 AM Jagannathan Raman <jag.raman@oracle.com>
wrote:

> Associate the file descriptor for a PCIDevice in remote process with
> DeviceState 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/remote/remote-obj.h |  42 +++++++++++
>  hw/remote/message.c            |   1 +
>  hw/remote/remote-obj.c         | 154
> +++++++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                    |   2 +
>  hw/remote/meson.build          |   1 +
>  5 files changed, 200 insertions(+)
>  create mode 100644 include/hw/remote/remote-obj.h
>  create mode 100644 hw/remote/remote-obj.c
>
> diff --git a/include/hw/remote/remote-obj.h
> b/include/hw/remote/remote-obj.h
> new file mode 100644
> index 0000000..0e95813
> --- /dev/null
> +++ b/include/hw/remote/remote-obj.h
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright © 2020 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.
> + *
> + */
> +
> +#ifndef REMOTE_OBJECT_H
> +#define REMOTE_OBJECT_H
> +
> +#include "io/channel.h"
> +#include "qemu/notify.h"
> +
> +#define TYPE_REMOTE_OBJECT "x-remote-object"
> +#define REMOTE_OBJECT(obj) \
> +    OBJECT_CHECK(RemoteObject, (obj), TYPE_REMOTE_OBJECT)
> +#define REMOTE_OBJECT_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(RemoteObjectClass, (obj), TYPE_REMOTE_OBJECT)
> +#define REMOTE_OBJECT_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(RemoteObjectClass, (klass), TYPE_REMOTE_OBJECT)
> +
> +typedef struct {
> +    ObjectClass parent_class;
> +
> +    unsigned int nr_devs;
> +    unsigned int max_devs;
> +} RemoteObjectClass;
> +
> +typedef struct {
> +    /* private */
> +    Object parent;
> +
> +    Notifier machine_done;
> +
> +    int fd;
> +    char *devid;
> +    QIOChannel *ioc;
> +} RemoteObject;
> +
> +#endif
>

There is no need for a header if the header isn't actually shared with
various .c units. In this series, you can just declare those structs in
remote-obj.c

diff --git a/hw/remote/message.c b/hw/remote/message.c
> index 5d87bf4..1f2edc7 100644
> --- a/hw/remote/message.c
> +++ b/hw/remote/message.c
> @@ -56,6 +56,7 @@ void coroutine_fn mpqemu_remote_msg_loop_co(void *data)
>          }
>      }
>      qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +    g_free(com);
>


Should be squashed with the previous patch


>      return;
>  }
> diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
> new file mode 100644
> index 0000000..3b4c0d4
> --- /dev/null
> +++ b/hw/remote/remote-obj.c
> @@ -0,0 +1,154 @@
> +/*
> + * Copyright © 2020 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.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +
> +#include "hw/remote/remote-obj.h"
> +#include "qemu/error-report.h"
> +#include "qom/object_interfaces.h"
> +#include "hw/qdev-core.h"
> +#include "io/channel.h"
> +#include "hw/qdev-core.h"
> +#include "hw/remote/machine.h"
> +#include "io/channel-util.h"
> +#include "qapi/error.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/pci/pci.h"
> +
> +static void remote_object_set_fd(Object *obj, const char *str, Error
> **errp)
> +{
> +    RemoteObject *o = REMOTE_OBJECT(obj);
> +
> +    o->fd = atoi(str);
>

 qemu_strtoi() has better error handling semantics. You may also want to
check it's a valid socket fd with fd_is_socket().

Alternatively, you could use qemu_open() which allows to open from QMP
fdset ("/dev/fdset/..."). This should be more flexible.

+}
> +
> +static void remote_object_set_devid(Object *obj, const char *str, Error
> **errp)
> +{
> +    RemoteObject *o = REMOTE_OBJECT(obj);
> +
> +    g_free(o->devid);
> +
> +    o->devid = g_strdup(str);
> +}
> +
> +static void property_release_remote_object(Object *obj, const char *name,
> +                                           void *opaque)
> +{
> +    Object *remote_object = OBJECT(opaque);
> +
> +    object_unref(remote_object);
> +}
>

Hmm, an object property, discussed below.

+
> +static void remote_object_machine_done(Notifier *notifier, void *data)
> +{
> +    RemoteObject *o = container_of(notifier, RemoteObject, machine_done);
> +    DeviceState *dev = NULL;
> +    QIOChannel *ioc = NULL;
> +    Coroutine *co = NULL;
> +    RemoteCommDev *comdev = NULL;
> +    Error *err = NULL;
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), o->devid);
> +    if (!dev || !object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        error_report("%s is not a PCI device", o->devid);
> +        return;
> +    }
> +
> +    ioc = qio_channel_new_fd(o->fd, &err);
> +    if (!ioc) {
> +        error_report_err(err);
> +        return;
> +    }
> +    qio_channel_set_blocking(ioc, false, NULL);
> +
> +    object_property_add(OBJECT(dev), "remote-object", "object", NULL,
> NULL,
> +                        property_release_remote_object, (void
> *)OBJECT(o));
>

In general, we are trying to avoid runtime/dynamic properties and slowly
replacing them with class properties.

Furthermore, this is not the way QOM handles object properties. You should
use object_class_property_add_link().

+    /* co-routine should free this. */
> +    comdev = g_new0(RemoteCommDev, 1);
> +    *comdev = (RemoteCommDev) {
> +        .ioc = ioc,
> +        .dev = PCI_DEVICE(dev),
> +    };
>
+
> +    co = qemu_coroutine_create(mpqemu_remote_msg_loop_co, comdev);
> +    qemu_coroutine_enter(co);
> +}
> +
> +static void remote_object_init(Object *obj)
> +{
> +    RemoteObjectClass *k = REMOTE_OBJECT_GET_CLASS(obj);
> +    RemoteObject *o = REMOTE_OBJECT(obj);
> +
> +    if (k->nr_devs >= k->max_devs) {
> +        error_report("Reached maximum number of devices: %u",
> k->max_devs);
> +        return;
> +    }
> +
> +    o->ioc = NULL;
> +    o->fd = -1;
> +    o->devid = NULL;
> +
> +    k->nr_devs++;
> +
> +    object_property_add_str(obj, "fd", NULL, remote_object_set_fd);
> +    object_property_set_description(obj, "fd",
> +                                    "file descriptor for the object");
> +    object_property_add_str(obj, "devid", NULL, remote_object_set_devid);
> +    object_property_set_description(obj, "devid",
> +                                    "id of device to associate");
>

Please use class properties

+
> +    o->machine_done.notify = remote_object_machine_done;
> +    qemu_add_machine_init_done_notifier(&o->machine_done);
> +}
> +
> +static void remote_object_finalize(Object *obj)
> +{
> +    RemoteObjectClass *k = REMOTE_OBJECT_GET_CLASS(obj);
> +    RemoteObject *o = REMOTE_OBJECT(obj);
> +
> +    if (o->ioc) {
> +        qio_channel_shutdown(o->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
> +        qio_channel_close(o->ioc, NULL);
> +    }
> +
> +    object_unref(OBJECT(o->ioc));
> +
> +    k->nr_devs--;
> +    g_free(o->devid);
> +}
> +
> +static void remote_object_class_init(ObjectClass *klass, void *data)
> +{
> +    RemoteObjectClass *k = REMOTE_OBJECT_CLASS(klass);
> +
> +    k->max_devs = 1;
>

Could you explain that limitation, in a code comment and/or commit message?

+    k->nr_devs = 0;
> +}
> +
> +static const TypeInfo remote_object_info = {
> +    .name = TYPE_REMOTE_OBJECT,
> +    .parent = TYPE_OBJECT,
> +    .instance_size = sizeof(RemoteObject),
> +    .instance_init = remote_object_init,
> +    .instance_finalize = remote_object_finalize,
> +    .class_size = sizeof(RemoteObjectClass),
> +    .class_init = remote_object_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&remote_object_info);
> +}
> +
> +type_init(register_types);
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b64e4b8..aedfc27 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3144,6 +3144,8 @@ F: include/hw/remote/machine.h
>  F: hw/remote/mpqemu-link.c
>  F: include/hw/remote/mpqemu-link.h
>  F: hw/remote/message.c
> +F: include/hw/remote/remote-obj.h
> +F: hw/remote/remote-obj.c
>
>  Build and test automation
>  -------------------------
> diff --git a/hw/remote/meson.build b/hw/remote/meson.build
> index 9f5c57f..71d0a56 100644
> --- a/hw/remote/meson.build
> +++ b/hw/remote/meson.build
> @@ -3,5 +3,6 @@ remote_ss = ss.source_set()
>  remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('machine.c'))
>  remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true:
> files('mpqemu-link.c'))
>  remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('message.c'))
> +remote_ss.add(when: 'CONFIG_MULTIPROCESS', if_true: files('remote-obj.c'))
>
>  softmmu_ss.add_all(when: 'CONFIG_MULTIPROCESS', if_true: remote_ss)
> --
> 1.8.3.1
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 12629 bytes --]