[15/18] xen: add a mechanism to automatically create XenDevice-s...

...that maintains compatibility with existing Xen toolstacks.

Xen toolstacks instantiate PV backends by simply writing information into
xenstore and expecting a backend implementation to be watching for this.

This patch adds a new 'xen-backend' module to allow individual XenDevice
implementations to register a creator function to be called when a tool-
stack instantiates a new backend in this way.

To support this it is also necessary to add new watchers into the XenBus
implementation to handle enumeration of new backends and also destruction
of XenDevice-s when the toolstack sets the backend 'online' key to 0.

NOTE: This patch only adds the framework. A subsequent patch will add a
      creator function for xen-qdisk.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 hw/xen/Makefile.objs         |   2 +-
 hw/xen/trace-events          |   5 +
 hw/xen/xen-backend.c         |  67 +++++++++++++
 hw/xen/xen-bus.c             | 220 +++++++++++++++++++++++++++++++++++++++----
 include/hw/xen/xen-backend.h |  24 +++++
 include/hw/xen/xen-bus.h     |   5 +-
 include/qemu/module.h        |   3 +
 7 files changed, 305 insertions(+), 21 deletions(-)
 create mode 100644 hw/xen/xen-backend.c
 create mode 100644 include/hw/xen/xen-backend.h

On Wed, Nov 21, 2018 at 03:12:08PM +0000, Paul Durrant wrote:
> +    xen_backend_device_create(BUS(xenbus), type, name, opts, &local_err);
> +    qobject_unref(opts);
> +
> +    if (local_err) {
> +        const char *msg = error_get_pretty(local_err);
> +
> +        error_report("failed to create '%s' device '%s': %s", type, name,
> +                     msg);
> +        error_free(local_err);

You can use error_reportf_err() instead of those three calls. I may have
only suggest error_report_err in a previous patch, but error_reportf_err
does the error_prepend as well.

> +    }
> +}
> +
> +static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
> +{
> +    char *domain_path = g_strdup_printf("backend/%s/%u", type, xen_domid);
> +    char **backend;
> +    unsigned int i, n;
> +
> +    trace_xen_bus_type_enumerate(type);
> +
> +    backend = xs_directory(xenbus->xsh, XBT_NULL, domain_path, &n);
> +    if (!backend) {

domain_path isn't free here, you probably want a `goto out` which would
free everything.

> +        return;
> +    }
> +
> @@ -193,6 +302,17 @@ static void xen_bus_realize(BusState *bus, Error **errp)
>      notifier_list_init(&xenbus->watch_notifiers);
>      qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
>                          xenbus);
> +
> +    module_call_init(MODULE_INIT_XEN_BACKEND);
> +
> +    xenbus->backend_watch =
> +        xen_bus_add_watch(xenbus, "", /* domain root node */
> +                          "backend", xen_bus_enumerate, xenbus, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "failed to set up enumeration watch: ");

You should use error_propagate_prepend instead
error_propagate;error_prepend. And it looks like there is the same
mistake in other patches that I haven't notice.

Also you probably want goto fail here.


> +static void xen_device_backend_changed(void *opaque)
> +{
> +    XenDevice *xendev = opaque;
> +    const char *type = object_get_typename(OBJECT(xendev));
> +    enum xenbus_state state;
> +    unsigned int online;
> +
> +    trace_xen_device_backend_changed(type, xendev->name);
> +
> +    if (xen_device_backend_scanf(xendev, "state", "%u", &state) != 1) {
> +        state = XenbusStateUnknown;
> +    }
> +
> +    xen_device_backend_set_state(xendev, state);

It's kind of weird to set the internal state base on the external one
that something else may have modified. Shouldn't we check that it is
fine for something else to modify the state and that it is a correct
transition?

Also aren't we going in a loop by having QEMU set the state, then the
watch fires again? (Not really a loop since the function _set_state
check for changes.

Also maybe we should watch for the state changes only when something
else like libxl creates (ask for) the backend, and ignore changes when
QEMU did it itself.

> +
> +    if (xen_device_backend_scanf(xendev, "online", "%u", &online) != 1) {
> +        online = 0;
> +    }
> +
> +    xen_device_backend_set_online(xendev, !!online);
> +
> +    /*
> +     * If a backend is still 'online' then its state should be cycled
> +     * back round to InitWait in order for a new frontend instance to
> +     * connect. This may happen when, for example, a frontend driver is
> +     * re-installed or updated.
> +     * If a backend id not 'online' then the device should be destroyed.

s/id/is/

> +     */
> +    if (xendev->backend_online &&
> +        xendev->backend_state == XenbusStateClosed) {
> +        xen_device_backend_set_state(xendev, XenbusStateInitWait);
> +    } else if (!xendev->backend_online &&
> +               (xendev->backend_state == XenbusStateClosed ||
> +                xendev->backend_state == XenbusStateInitialising ||
> +                xendev->backend_state == XenbusStateInitWait ||
> +                xendev->backend_state == XenbusStateUnknown)) {
> +        object_unparent(OBJECT(xendev));
> +    }
> +}
> +
>  static void xen_device_backend_create(XenDevice *xendev, Error **errp)
>  {
>      XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> @@ -289,12 +463,38 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
>          error_propagate(errp, local_err);
>          error_prepend(errp, "failed to create backend: ");

It looks like there is a missing return here.

>      }
> +
> +    xendev->backend_state_watch =
> +        xen_bus_add_watch(xenbus, xendev->backend_path,
> +                          "state", xen_device_backend_changed,
> +                          xendev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "failed to watch backend state: ");

You should return here, as local_err mustn't be reused.

> +    }
> +
> +    xendev->backend_online_watch =
> +        xen_bus_add_watch(xenbus, xendev->backend_path,
> +                          "online", xen_device_backend_changed,
> +                          xendev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "failed to watch backend online: ");

You probably want a return here, in case there is more code added after.

> +    }

Other instances of error_propagate;error_prepend to be replaced by
error_propagate_prepend.

>  }
>  

Thanks,

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 04 December 2018 15:35
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 15/18] xen: add a mechanism to automatically create
> XenDevice-s...
> 
> On Wed, Nov 21, 2018 at 03:12:08PM +0000, Paul Durrant wrote:
> > +    xen_backend_device_create(BUS(xenbus), type, name, opts,
> &local_err);
> > +    qobject_unref(opts);
> > +
> > +    if (local_err) {
> > +        const char *msg = error_get_pretty(local_err);
> > +
> > +        error_report("failed to create '%s' device '%s': %s", type,
> name,
> > +                     msg);
> > +        error_free(local_err);
> 
> You can use error_reportf_err() instead of those three calls. I may have
> only suggest error_report_err in a previous patch, but error_reportf_err
> does the error_prepend as well.
> 

Ah. I'll go back over the patches and use that where necessary.

> > +    }
> > +}
> > +
> > +static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
> > +{
> > +    char *domain_path = g_strdup_printf("backend/%s/%u", type,
> xen_domid);
> > +    char **backend;
> > +    unsigned int i, n;
> > +
> > +    trace_xen_bus_type_enumerate(type);
> > +
> > +    backend = xs_directory(xenbus->xsh, XBT_NULL, domain_path, &n);
> > +    if (!backend) {
> 
> domain_path isn't free here, you probably want a `goto out` which would
> free everything.

Ok.

> 
> > +        return;
> > +    }
> > +
> > @@ -193,6 +302,17 @@ static void xen_bus_realize(BusState *bus, Error
> **errp)
> >      notifier_list_init(&xenbus->watch_notifiers);
> >      qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
> >                          xenbus);
> > +
> > +    module_call_init(MODULE_INIT_XEN_BACKEND);
> > +
> > +    xenbus->backend_watch =
> > +        xen_bus_add_watch(xenbus, "", /* domain root node */
> > +                          "backend", xen_bus_enumerate, xenbus,
> &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to set up enumeration watch: ");
> 
> You should use error_propagate_prepend instead
> error_propagate;error_prepend. And it looks like there is the same
> mistake in other patches that I haven't notice.
> 

Oh, I didn't know about that one either... I've only seen the separate calls used elsewhere.

> Also you probably want goto fail here.
> 

Not sure about that. Whilst the bus scan won't happen, it doesn't mean devices can't be added via QMP.

> 
> > +static void xen_device_backend_changed(void *opaque)
> > +{
> > +    XenDevice *xendev = opaque;
> > +    const char *type = object_get_typename(OBJECT(xendev));
> > +    enum xenbus_state state;
> > +    unsigned int online;
> > +
> > +    trace_xen_device_backend_changed(type, xendev->name);
> > +
> > +    if (xen_device_backend_scanf(xendev, "state", "%u", &state) != 1) {
> > +        state = XenbusStateUnknown;
> > +    }
> > +
> > +    xen_device_backend_set_state(xendev, state);
> 
> It's kind of weird to set the internal state base on the external one
> that something else may have modified. Shouldn't we check that it is
> fine for something else to modify the state and that it is a correct
> transition?

The only thing (apart from this code) that's going to have perms to write the backend state is the toolstack... which is, of course, be definition trusted.

> 
> Also aren't we going in a loop by having QEMU set the state, then the
> watch fires again? (Not really a loop since the function _set_state
> check for changes.

No. It's de-bounced inside the set_state function.

> 
> Also maybe we should watch for the state changes only when something
> else like libxl creates (ask for) the backend, and ignore changes when
> QEMU did it itself.

I don't think it's necessary to add that complexity.

> 
> > +
> > +    if (xen_device_backend_scanf(xendev, "online", "%u", &online) != 1)
> {
> > +        online = 0;
> > +    }
> > +
> > +    xen_device_backend_set_online(xendev, !!online);
> > +
> > +    /*
> > +     * If a backend is still 'online' then its state should be cycled
> > +     * back round to InitWait in order for a new frontend instance to
> > +     * connect. This may happen when, for example, a frontend driver is
> > +     * re-installed or updated.
> > +     * If a backend id not 'online' then the device should be
> destroyed.
> 
> s/id/is/

Ok.

> 
> > +     */
> > +    if (xendev->backend_online &&
> > +        xendev->backend_state == XenbusStateClosed) {
> > +        xen_device_backend_set_state(xendev, XenbusStateInitWait);
> > +    } else if (!xendev->backend_online &&
> > +               (xendev->backend_state == XenbusStateClosed ||
> > +                xendev->backend_state == XenbusStateInitialising ||
> > +                xendev->backend_state == XenbusStateInitWait ||
> > +                xendev->backend_state == XenbusStateUnknown)) {
> > +        object_unparent(OBJECT(xendev));
> > +    }
> > +}
> > +
> >  static void xen_device_backend_create(XenDevice *xendev, Error **errp)
> >  {
> >      XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > @@ -289,12 +463,38 @@ static void xen_device_backend_create(XenDevice
> *xendev, Error **errp)
> >          error_propagate(errp, local_err);
> >          error_prepend(errp, "failed to create backend: ");
> 
> It looks like there is a missing return here.
> 
> >      }
> > +
> > +    xendev->backend_state_watch =
> > +        xen_bus_add_watch(xenbus, xendev->backend_path,
> > +                          "state", xen_device_backend_changed,
> > +                          xendev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to watch backend state: ");
> 
> You should return here, as local_err mustn't be reused.
> 
> > +    }
> > +
> > +    xendev->backend_online_watch =
> > +        xen_bus_add_watch(xenbus, xendev->backend_path,
> > +                          "online", xen_device_backend_changed,
> > +                          xendev, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "failed to watch backend online: ");
> 
> You probably want a return here, in case there is more code added after.

Yes, there should be returns in all three cases above.

> 
> > +    }
> 
> Other instances of error_propagate;error_prepend to be replaced by
> error_propagate_prepend.

Yes, will do.

  Paul

> 
> >  }
> >
> 
> Thanks,
> 
> --
> Anthony PERARD

On Thu, Dec 06, 2018 at 12:36:52PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 04 December 2018 15:35
> > 
> > On Wed, Nov 21, 2018 at 03:12:08PM +0000, Paul Durrant wrote:
> > > +    xenbus->backend_watch =
> > > +        xen_bus_add_watch(xenbus, "", /* domain root node */
> > > +                          "backend", xen_bus_enumerate, xenbus,
> > &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        error_prepend(errp, "failed to set up enumeration watch: ");
> > 
> > You should use error_propagate_prepend instead
> > error_propagate;error_prepend. And it looks like there is the same
> > mistake in other patches that I haven't notice.
> > 
> 
> Oh, I didn't know about that one either... I've only seen the separate calls used elsewhere.

That information is all in "include/qapi/error.h", if you which to know
more on how to use Error.

> > Also you probably want goto fail here.
> > 
> 
> Not sure about that. Whilst the bus scan won't happen, it doesn't mean devices can't be added via QMP.

In that case, don't modify errp, and use error_reportf_err instead, or
warn_reportf_err (then local_err = NULL, in case it is reused in a
future modification of the function).

Setting errp (with error_propagate) means that the function failed, and
QEMU is going to exit(1), because of qdev_init_nofail call in
xen_bus_init.

> > > +static void xen_device_backend_changed(void *opaque)
> > > +{
> > > +    XenDevice *xendev = opaque;
> > > +    const char *type = object_get_typename(OBJECT(xendev));
> > > +    enum xenbus_state state;
> > > +    unsigned int online;
> > > +
> > > +    trace_xen_device_backend_changed(type, xendev->name);
> > > +
> > > +    if (xen_device_backend_scanf(xendev, "state", "%u", &state) != 1) {
> > > +        state = XenbusStateUnknown;
> > > +    }
> > > +
> > > +    xen_device_backend_set_state(xendev, state);
> > 
> > It's kind of weird to set the internal state base on the external one
> > that something else may have modified. Shouldn't we check that it is
> > fine for something else to modify the state and that it is a correct
> > transition?
> 
> The only thing (apart from this code) that's going to have perms to write the backend state is the toolstack... which is, of course, be definition trusted.

"trusted" doesn't mean that there isn't a bug somewhere else :-). But I
guess it's good enough for now.

> > Also aren't we going in a loop by having QEMU set the state, then the
> > watch fires again? (Not really a loop since the function _set_state
> > check for changes.
> 
> No. It's de-bounced inside the set_state function.
> 
> > 
> > Also maybe we should watch for the state changes only when something
> > else like libxl creates (ask for) the backend, and ignore changes when
> > QEMU did it itself.
> 
> I don't think it's necessary to add that complexity.

Ok.

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 06 December 2018 15:24
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-block@nongnu.org; qemu-devel@nongnu.org; xen-
> devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 15/18] xen: add a mechanism to automatically create
> XenDevice-s...
> 
> On Thu, Dec 06, 2018 at 12:36:52PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > Sent: 04 December 2018 15:35
> > >
> > > On Wed, Nov 21, 2018 at 03:12:08PM +0000, Paul Durrant wrote:
> > > > +    xenbus->backend_watch =
> > > > +        xen_bus_add_watch(xenbus, "", /* domain root node */
> > > > +                          "backend", xen_bus_enumerate, xenbus,
> > > &local_err);
> > > > +    if (local_err) {
> > > > +        error_propagate(errp, local_err);
> > > > +        error_prepend(errp, "failed to set up enumeration watch:
> ");
> > >
> > > You should use error_propagate_prepend instead
> > > error_propagate;error_prepend. And it looks like there is the same
> > > mistake in other patches that I haven't notice.
> > >
> >
> > Oh, I didn't know about that one either... I've only seen the separate
> calls used elsewhere.
> 
> That information is all in "include/qapi/error.h", if you which to know
> more on how to use Error.
> 

Thanks.

> > > Also you probably want goto fail here.
> > >
> >
> > Not sure about that. Whilst the bus scan won't happen, it doesn't mean
> devices can't be added via QMP.
> 
> In that case, don't modify errp, and use error_reportf_err instead, or
> warn_reportf_err (then local_err = NULL, in case it is reused in a
> future modification of the function).
> 
> Setting errp (with error_propagate) means that the function failed, and
> QEMU is going to exit(1), because of qdev_init_nofail call in
> xen_bus_init.

Ah, good point. I'll wait for more feedback on v2 and then fix in v3.

  Paul