diff mbox series

[09/12] hw/xen: prevent duplicate device registrations

Message ID 20231016151909.22133-10-dwmw2@infradead.org (mailing list archive)
State New, archived
Headers show
Series Get Xen PV shim running in qemu | expand

Commit Message

David Woodhouse Oct. 16, 2023, 3:19 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

Ensure that we have a XenBackendInstance for every device regardless
of whether it was "discovered" in XenStore or created directly in QEMU.

This allows the backend_list to be a source of truth about whether a
given backend exists, and allows us to reject duplicates.

This also cleans up the fact that backend drivers were calling
xen_backend_set_device() with a XenDevice immediately after calling
qdev_realize_and_unref() on it, when it wasn't theirs to play with any
more.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/block/xen-block.c         |  1 -
 hw/char/xen_console.c        |  2 +-
 hw/xen/xen-backend.c         | 78 ++++++++++++++++++++++++++----------
 hw/xen/xen-bus.c             |  8 ++++
 include/hw/xen/xen-backend.h |  3 ++
 5 files changed, 69 insertions(+), 23 deletions(-)

Comments

Paul Durrant Oct. 24, 2023, 2:10 p.m. UTC | #1 
On 16/10/2023 16:19, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Ensure that we have a XenBackendInstance for every device regardless
> of whether it was "discovered" in XenStore or created directly in QEMU.
> 
> This allows the backend_list to be a source of truth about whether a
> given backend exists, and allows us to reject duplicates.
> 
> This also cleans up the fact that backend drivers were calling
> xen_backend_set_device() with a XenDevice immediately after calling
> qdev_realize_and_unref() on it, when it wasn't theirs to play with any
> more.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/block/xen-block.c         |  1 -
>   hw/char/xen_console.c        |  2 +-
>   hw/xen/xen-backend.c         | 78 ++++++++++++++++++++++++++----------
>   hw/xen/xen-bus.c             |  8 ++++
>   include/hw/xen/xen-backend.h |  3 ++
>   5 files changed, 69 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a07cd7eb5d..9262338535 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -975,7 +975,6 @@ static void xen_block_device_create(XenBackendInstance *backend,
>           goto fail;
>       }
>   
> -    xen_backend_set_device(backend, xendev);
>       return;
>   
>   fail:
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index bd20be116c..2825b8c511 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
>       Chardev *cd = NULL;
>       struct qemu_xs_handle *xsh = xenbus->xsh;
>   
> -    if (qemu_strtoul(name, NULL, 10, &number)) {
> +    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
>           error_setg(errp, "failed to parse name '%s'", name);
>           goto fail;
>       }
I don't think this hunk belongs here, does it? Seems like it should be 
in patch 7.

> diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
> index b9bf70a9f5..dcb4329258 100644
> --- a/hw/xen/xen-backend.c
> +++ b/hw/xen/xen-backend.c
> @@ -101,22 +101,28 @@ static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
>       return NULL;
>   }
>   
> -bool xen_backend_exists(const char *type, const char *name)
> +static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl, const char *name)

This name is a little close to xen_backend_table_lookup()... perhaps 
that one should be renamed xen_backend_impl_lookup() for clarity.

>   {
> -    const XenBackendImpl *impl = xen_backend_table_lookup(type);
>       XenBackendInstance *backend;
>   
> -    if (!impl) {
> -        return false;
> -    }
> -
>       QLIST_FOREACH(backend, &backend_list, entry) {
>           if (backend->impl == impl && !strcmp(backend->name, name)) {
> -            return true;
> +            return backend;
>           }
>       }
>   
> -    return false;
> +    return NULL;
> +}
> +
> +bool xen_backend_exists(const char *type, const char *name)
> +{
> +    const XenBackendImpl *impl = xen_backend_table_lookup(type);
> +
> +    if (!impl) {
> +        return false;
> +    }
> +
> +    return !!xen_backend_lookup(impl, name);
>   }
>   
>   static void xen_backend_list_remove(XenBackendInstance *backend)
> @@ -138,11 +144,10 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
>       backend = g_new0(XenBackendInstance, 1);
>       backend->xenbus = xenbus;
>       backend->name = g_strdup(name);
> -
> -    impl->create(backend, opts, errp);
> -
>       backend->impl = impl;
>       xen_backend_list_add(backend);
> +
> +    impl->create(backend, opts, errp);
>   }
>   
>   XenBus *xen_backend_get_bus(XenBackendInstance *backend)
> @@ -155,13 +160,6 @@ const char *xen_backend_get_name(XenBackendInstance *backend)
>       return backend->name;
>   }
>   
> -void xen_backend_set_device(XenBackendInstance *backend,
> -                            XenDevice *xendev)
> -{
> -    g_assert(!backend->xendev);
> -    backend->xendev = xendev;
> -}
> -

The declaration also needs removing from xen-backend.h presumably.

>   XenDevice *xen_backend_get_device(XenBackendInstance *backend)
>   {
>       return backend->xendev;
> @@ -178,9 +176,7 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
>       }
>   
>       impl = backend->impl;
> -    if (backend->xendev) {
> -        impl->destroy(backend, errp);
> -    }
> +    impl->destroy(backend, errp);
>   
>       xen_backend_list_remove(backend);
>       g_free(backend->name);
> @@ -188,3 +184,43 @@ bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
>   
>       return true;
>   }
> +
> +bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
> +{
> +    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
> +    const char *type = xendev_class->backend ? : object_get_typename(OBJECT(xendev));
> +    const XenBackendImpl *impl = xen_backend_table_lookup(type);
> +    XenBackendInstance *backend;
> +
> +    if (!impl) {
> +        return false;
> +    }
> +
> +    backend = xen_backend_lookup(impl, xendev->name);
> +    if (backend) {
> +        if (backend->xendev && backend->xendev != xendev) {
> +            error_setg(errp, "device %s/%s already exists", type, xendev->name);
> +            return false;
> +        }
> +        backend->xendev = xendev;
> +        return true;
> +    }
> +
> +    backend = g_new0(XenBackendInstance, 1);
> +    backend->xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> +    backend->xendev = xendev;
> +    backend->name = g_strdup(xendev->name);
> +    backend->impl = impl;
> +
> +    xen_backend_list_add(backend);
> +    return true;
> +}

Not sure I'm getting my head around this. How does this play with the 
legacy backend code? The point about having the impl list was so that 
the newer xenbus code didn't conflict with the legacy backend code, 
which thinks it has carte blanche ownership of a class.

   Paul

> +
> +void xen_backend_device_unrealized(XenDevice *xendev)
> +{
> +    XenBackendInstance *backend = xen_backend_list_find(xendev);
> +
> +    if (backend) {
> +        backend->xendev = NULL;
> +    }
> +}
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 0da2aa219a..0b232d1f94 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -359,6 +359,8 @@ static void xen_bus_realize(BusState *bus, Error **errp)
>   
>       g_free(type);
>       g_free(key);
> +
> +    xen_bus_enumerate(xenbus);
>       return;
>   
>   fail:
> @@ -958,6 +960,8 @@ static void xen_device_unrealize(DeviceState *dev)
>   
>       trace_xen_device_unrealize(type, xendev->name);
>   
> +    xen_backend_device_unrealized(xendev);
> +
>       if (xendev->exit.notify) {
>           qemu_remove_exit_notifier(&xendev->exit);
>           xendev->exit.notify = NULL;
> @@ -1024,6 +1028,10 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>           goto unrealize;
>       }
>   
> +    if (!xen_backend_device_realized(xendev, errp)) {
> +        goto unrealize;
> +    }
> +
>       trace_xen_device_realize(type, xendev->name);
>   
>       xendev->xsh = qemu_xen_xs_open();
> diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
> index 0f01631ae7..3f1e764c51 100644
> --- a/include/hw/xen/xen-backend.h
> +++ b/include/hw/xen/xen-backend.h
> @@ -38,4 +38,7 @@ void xen_backend_device_create(XenBus *xenbus, const char *type,
>                                  const char *name, QDict *opts, Error **errp);
>   bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
>   
> +bool xen_backend_device_realized(XenDevice *xendev, Error **errp);
> +void xen_backend_device_unrealized(XenDevice *xendev);
> +
>   #endif /* HW_XEN_BACKEND_H */
David Woodhouse Oct. 24, 2023, 2:38 p.m. UTC | #2 
On Tue, 2023-10-24 at 15:10 +0100, Paul Durrant wrote:
> On 16/10/2023 16:19, David Woodhouse wrote:
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -468,7 +468,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >       Chardev *cd = NULL;
> >       struct qemu_xs_handle *xsh = xenbus->xsh;
> >   
> > -    if (qemu_strtoul(name, NULL, 10, &number)) {
> > +    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
> >           error_setg(errp, "failed to parse name '%s'", name);
> >           goto fail;
> >       }
> I don't think this hunk belongs here, does it? Seems like it should be 
> in patch 7.

Well, console#4294967295 *did* actually work before this patch started
using -1 to mean something different. But yes, I've already moved that
into the previous patch.

In fact I've just completely dropped this patch now, as the
dedeuplication needs to happen on the *frontend* nodes, since a given
frontend can be powered by a backend of different types, or in
different driver domains.
diff mbox series

Patch

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..9262338535 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -975,7 +975,6 @@  static void xen_block_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
-    xen_backend_set_device(backend, xendev);
     return;
 
 fail:
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bd20be116c..2825b8c511 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -468,7 +468,7 @@  static void xen_console_device_create(XenBackendInstance *backend,
     Chardev *cd = NULL;
     struct qemu_xs_handle *xsh = xenbus->xsh;
 
-    if (qemu_strtoul(name, NULL, 10, &number)) {
+    if (qemu_strtoul(name, NULL, 10, &number) || number >= INT_MAX) {
         error_setg(errp, "failed to parse name '%s'", name);
         goto fail;
     }
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..dcb4329258 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,22 +101,28 @@  static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
     return NULL;
 }
 
-bool xen_backend_exists(const char *type, const char *name)
+static XenBackendInstance *xen_backend_lookup(const XenBackendImpl *impl, const char *name)
 {
-    const XenBackendImpl *impl = xen_backend_table_lookup(type);
     XenBackendInstance *backend;
 
-    if (!impl) {
-        return false;
-    }
-
     QLIST_FOREACH(backend, &backend_list, entry) {
         if (backend->impl == impl && !strcmp(backend->name, name)) {
-            return true;
+            return backend;
         }
     }
 
-    return false;
+    return NULL;
+}
+
+bool xen_backend_exists(const char *type, const char *name)
+{
+    const XenBackendImpl *impl = xen_backend_table_lookup(type);
+
+    if (!impl) {
+        return false;
+    }
+
+    return !!xen_backend_lookup(impl, name);
 }
 
 static void xen_backend_list_remove(XenBackendInstance *backend)
@@ -138,11 +144,10 @@  void xen_backend_device_create(XenBus *xenbus, const char *type,
     backend = g_new0(XenBackendInstance, 1);
     backend->xenbus = xenbus;
     backend->name = g_strdup(name);
-
-    impl->create(backend, opts, errp);
-
     backend->impl = impl;
     xen_backend_list_add(backend);
+
+    impl->create(backend, opts, errp);
 }
 
 XenBus *xen_backend_get_bus(XenBackendInstance *backend)
@@ -155,13 +160,6 @@  const char *xen_backend_get_name(XenBackendInstance *backend)
     return backend->name;
 }
 
-void xen_backend_set_device(XenBackendInstance *backend,
-                            XenDevice *xendev)
-{
-    g_assert(!backend->xendev);
-    backend->xendev = xendev;
-}
-
 XenDevice *xen_backend_get_device(XenBackendInstance *backend)
 {
     return backend->xendev;
@@ -178,9 +176,7 @@  bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
     }
 
     impl = backend->impl;
-    if (backend->xendev) {
-        impl->destroy(backend, errp);
-    }
+    impl->destroy(backend, errp);
 
     xen_backend_list_remove(backend);
     g_free(backend->name);
@@ -188,3 +184,43 @@  bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
 
     return true;
 }
+
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp)
+{
+    XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
+    const char *type = xendev_class->backend ? : object_get_typename(OBJECT(xendev));
+    const XenBackendImpl *impl = xen_backend_table_lookup(type);
+    XenBackendInstance *backend;
+
+    if (!impl) {
+        return false;
+    }
+
+    backend = xen_backend_lookup(impl, xendev->name);
+    if (backend) {
+        if (backend->xendev && backend->xendev != xendev) {
+            error_setg(errp, "device %s/%s already exists", type, xendev->name);
+            return false;
+        }
+        backend->xendev = xendev;
+        return true;
+    }
+
+    backend = g_new0(XenBackendInstance, 1);
+    backend->xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+    backend->xendev = xendev;
+    backend->name = g_strdup(xendev->name);
+    backend->impl = impl;
+
+    xen_backend_list_add(backend);
+    return true;
+}
+
+void xen_backend_device_unrealized(XenDevice *xendev)
+{
+    XenBackendInstance *backend = xen_backend_list_find(xendev);
+
+    if (backend) {
+        backend->xendev = NULL;
+    }
+}
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 0da2aa219a..0b232d1f94 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -359,6 +359,8 @@  static void xen_bus_realize(BusState *bus, Error **errp)
 
     g_free(type);
     g_free(key);
+
+    xen_bus_enumerate(xenbus);
     return;
 
 fail:
@@ -958,6 +960,8 @@  static void xen_device_unrealize(DeviceState *dev)
 
     trace_xen_device_unrealize(type, xendev->name);
 
+    xen_backend_device_unrealized(xendev);
+
     if (xendev->exit.notify) {
         qemu_remove_exit_notifier(&xendev->exit);
         xendev->exit.notify = NULL;
@@ -1024,6 +1028,10 @@  static void xen_device_realize(DeviceState *dev, Error **errp)
         goto unrealize;
     }
 
+    if (!xen_backend_device_realized(xendev, errp)) {
+        goto unrealize;
+    }
+
     trace_xen_device_realize(type, xendev->name);
 
     xendev->xsh = qemu_xen_xs_open();
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index 0f01631ae7..3f1e764c51 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -38,4 +38,7 @@  void xen_backend_device_create(XenBus *xenbus, const char *type,
                                const char *name, QDict *opts, Error **errp);
 bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
 
+bool xen_backend_device_realized(XenDevice *xendev, Error **errp);
+void xen_backend_device_unrealized(XenDevice *xendev);
+
 #endif /* HW_XEN_BACKEND_H */