[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [Qemu-devel] [PULL 06/25] xen: create xenstore areas for XenDevice-s
From: |
Paul Durrant |
Subject: |
RE: [Qemu-devel] [PULL 06/25] xen: create xenstore areas for XenDevice-s |
Date: |
Thu, 2 Apr 2020 10:49:50 +0100 |
> -----Original Message-----
> From: Philippe Mathieu-Daudé <address@hidden>
> Sent: 01 April 2020 17:14
> To: Anthony PERARD <address@hidden>; address@hidden
> Cc: address@hidden; Peter Maydell <address@hidden>; Paul Durrant
> <address@hidden>; Markus Armbruster <address@hidden>
> Subject: Re: [Qemu-devel] [PULL 06/25] xen: create xenstore areas for
> XenDevice-s
>
> Hi Anthony, Paul.
>
> Cc'ing Markus too.
>
> On 1/14/19 2:51 PM, Anthony PERARD wrote:
> > From: Paul Durrant <address@hidden>
> >
> > This patch adds a new source module, xen-bus-helper.c, which builds on
> > basic libxenstore primitives to provide functions to create (setting
> > permissions appropriately) and destroy xenstore areas, and functions to
> > 'printf' and 'scanf' nodes therein. The main xen-bus code then uses
> > these primitives [1] to initialize and destroy the frontend and backend
> > areas for a XenDevice during realize and unrealize respectively.
> >
> > The 'xen-block' implementation is extended with a 'get_name' method that
> > returns the VBD number. This number is required to 'name' the xenstore
> > areas.
> >
> > NOTE: An exit handler is also added to make sure the xenstore areas are
> > cleaned up if QEMU terminates without devices being unrealized.
> >
> > [1] The 'scanf' functions are actually not yet needed, but they will be
> > needed by code delivered in subsequent patches.
> >
> > Signed-off-by: Paul Durrant <address@hidden>
> > Reviewed-by: Anthony Perard <address@hidden>
> > Signed-off-by: Anthony PERARD <address@hidden>
> > ---
> > hw/block/xen-block.c | 9 +
> > hw/xen/Makefile.objs | 2 +-
> > hw/xen/trace-events | 12 +-
> > hw/xen/xen-bus-helper.c | 150 +++++++++++++++
> > hw/xen/xen-bus.c | 321 +++++++++++++++++++++++++++++++-
> > include/hw/xen/xen-bus-helper.h | 39 ++++
> > include/hw/xen/xen-bus.h | 12 ++
> > 7 files changed, 540 insertions(+), 5 deletions(-)
> > create mode 100644 hw/xen/xen-bus-helper.c
> > create mode 100644 include/hw/xen/xen-bus-helper.h
> >
> [...]
> > +static void xen_device_exit(Notifier *n, void *data)
> > +{
> > + XenDevice *xendev = container_of(n, XenDevice, exit);
> > +
> > + xen_device_unrealize(DEVICE(xendev), &error_abort);
> > }
> >
> > static void xen_device_realize(DeviceState *dev, Error **errp)
> > {
> > XenDevice *xendev = XEN_DEVICE(dev);
> > XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> > + XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> > const char *type = object_get_typename(OBJECT(xendev));
> > Error *local_err = NULL;
> >
> > - trace_xen_device_realize(type);
> > + if (xendev->frontend_id == DOMID_INVALID) {
> > + xendev->frontend_id = xen_domid;
> > + }
> > +
> > + if (xendev->frontend_id >= DOMID_FIRST_RESERVED) {
> > + error_setg(errp, "invalid frontend-id");
> > + goto unrealize;
> > + }
> > +
> > + if (!xendev_class->get_name) {
> > + error_setg(errp, "get_name method not implemented");
> > + goto unrealize;
> > + }
> > +
> > + xendev->name = xendev_class->get_name(xendev, &local_err);
> > + if (local_err) {
> > + error_propagate_prepend(errp, local_err,
> > + "failed to get device name: ");
> > + goto unrealize;
> > + }
> > +
> > + trace_xen_device_realize(type, xendev->name);
> > +
> > + xen_device_backend_create(xendev, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + goto unrealize;
> > + }
> > +
> > + xen_device_frontend_create(xendev, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + goto unrealize;
> > + }
> >
> > if (xendev_class->realize) {
> > xendev_class->realize(xendev, &local_err);
> > @@ -72,18 +364,43 @@ static void xen_device_realize(DeviceState *dev, Error
> > **errp)
> > }
> > }
> >
> > + xen_device_backend_printf(xendev, "frontend", "%s",
> > + xendev->frontend_path);
> > + xen_device_backend_printf(xendev, "frontend-id", "%u",
> > + xendev->frontend_id);
> > + xen_device_backend_printf(xendev, "online", "%u", 1);
> > + xen_device_backend_printf(xendev, "hotplug-status", "connected");
> > +
> > + xen_device_backend_set_state(xendev, XenbusStateInitWait);
> > +
> > + xen_device_frontend_printf(xendev, "backend", "%s",
> > + xendev->backend_path);
> > + xen_device_frontend_printf(xendev, "backend-id", "%u",
> > + xenbus->backend_id);
> > +
> > + xen_device_frontend_set_state(xendev, XenbusStateInitialising);
> > +
> > + xendev->exit.notify = xen_device_exit;
> > + qemu_add_exit_notifier(&xendev->exit);
> > return;
> >
> > unrealize:
> > xen_device_unrealize(dev, &error_abort);
>
> It seems if unrealize() fails, the error stored in &local_err is never
> reported. Not sure if this can be improved although.
In this case that's essentially a "don't care". We want to know why the realize
failed but if the unrealize fails something is probably pretty seriously
screwed (hence the error_abort).
Paul