Re: [PATCH 4/4] hw/nvme: fix controller hot unplugging

[Klaus Jensen <its@irrelevant.dk>]

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

On Jul  6 11:33, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Prior to this patch the nvme-ns devices are always children of the
>NvmeBus owned by the NvmeCtrl. This causes the namespaces to be
>unrealized when the parent device is removed. However, when subsystems
>are involved, this is not what we want since the namespaces may be
>attached to other controllers as well.
>
>This patch adds an additional NvmeBus on the subsystem device. When
>nvme-ns devices are realized, if the parent controller device is linked
>to a subsystem, the parent bus is set to the subsystem one instead. This
>makes sure that namespaces are kept alive and not unrealized.
>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/nvme/nvme.h   | 18 ++++++++++--------
> hw/nvme/ctrl.c   |  8 +++++---
> hw/nvme/ns.c     | 32 +++++++++++++++++++++++++-------
> hw/nvme/subsys.c |  4 ++++
> 4 files changed, 44 insertions(+), 18 deletions(-)
>
>diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>index c4065467d877..9401e212f9f7 100644
>--- a/hw/nvme/nvme.h
>+++ b/hw/nvme/nvme.h
>@@ -33,12 +33,21 @@ QEMU_BUILD_BUG_ON(NVME_MAX_NAMESPACES > NVME_NSID_BROADCAST - 1);
> typedef struct NvmeCtrl NvmeCtrl;
> typedef struct NvmeNamespace NvmeNamespace;
>
>+#define TYPE_NVME_BUS "nvme-bus"
>+OBJECT_DECLARE_SIMPLE_TYPE(NvmeBus, NVME_BUS)
>+
>+typedef struct NvmeBus {
>+    BusState parent_bus;
>+    bool     is_subsys;
>+} NvmeBus;
>+
> #define TYPE_NVME_SUBSYS "nvme-subsys"
> #define NVME_SUBSYS(obj) \
>     OBJECT_CHECK(NvmeSubsystem, (obj), TYPE_NVME_SUBSYS)
>
> typedef struct NvmeSubsystem {
>     DeviceState parent_obj;
>+    NvmeBus     bus;
>     uint8_t     subnqn[256];
>
>     NvmeCtrl      *ctrls[NVME_MAX_CONTROLLERS];
>@@ -365,13 +374,6 @@ typedef struct NvmeCQueue {
>     QTAILQ_HEAD(, NvmeRequest) req_list;
> } NvmeCQueue;
>
>-#define TYPE_NVME_BUS "nvme-bus"
>-#define NVME_BUS(obj) OBJECT_CHECK(NvmeBus, (obj), TYPE_NVME_BUS)
>-
>-typedef struct NvmeBus {
>-    BusState parent_bus;
>-} NvmeBus;
>-
> #define TYPE_NVME "nvme"
> #define NVME(obj) \
>         OBJECT_CHECK(NvmeCtrl, (obj), TYPE_NVME)
>@@ -463,7 +465,7 @@ typedef struct NvmeCtrl {
>
> static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid)
> {
>-    if (!nsid || nsid > NVME_MAX_NAMESPACES) {
>+    if (!n || !nsid || nsid > NVME_MAX_NAMESPACES) {
>         return NULL;
>     }
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 90e3ee2b70ee..7c8fca36d9a5 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -6516,11 +6516,13 @@ static void nvme_exit(PCIDevice *pci_dev)
>
>     for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
>         ns = nvme_ns(n, i);
>-        if (!ns) {
>-            continue;
>+        if (ns) {
>+            ns->attached--;
>         }
>+    }
>
>-        nvme_ns_cleanup(ns);
>+    if (n->subsys) {
>+        nvme_subsys_unregister_ctrl(n->subsys, n);
>     }
>
>     if (n->subsys) {
>diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>index 3c4f5b8c714a..612a2786d75d 100644
>--- a/hw/nvme/ns.c
>+++ b/hw/nvme/ns.c
>@@ -444,13 +444,29 @@ void nvme_ns_cleanup(NvmeNamespace *ns)
> static void nvme_ns_realize(DeviceState *dev, Error **errp)
> {
>     NvmeNamespace *ns = NVME_NS(dev);
>-    BusState *s = qdev_get_parent_bus(dev);
>-    NvmeCtrl *n = NVME(s->parent);
>-    NvmeSubsystem *subsys = n->subsys;
>+    BusState *qbus = qdev_get_parent_bus(dev);
>+    NvmeBus *bus = NVME_BUS(qbus);
>+    NvmeCtrl *n = NULL;
>+    NvmeSubsystem *subsys = NULL;
>     uint32_t nsid = ns->params.nsid;
>     int i;
>
>-    if (!n->subsys) {
>+    if (bus->is_subsys) {
>+        subsys = NVME_SUBSYS(qbus->parent);
>+    } else {
>+        n = NVME(qbus->parent);
>+        subsys = n->subsys;
>+    }

So, I realized that this if is not needed, since the device will always 
attach to the bus from the nvme device, never the 'fake' one from the 
subsystem.

>+
>+    if (subsys) {
>+        /*
>+         * If this namespace belongs to a subsystem (through a link on the
>+         * controller device), reparent the device.
>+         */
>+        if (!qdev_set_parent_bus(dev, &subsys->bus.parent_bus, errp)) {
>+            return;
>+        }
>+    } else {
>         if (ns->params.detached) {
>             error_setg(errp, "detached requires that the nvme device is "
>                        "linked to an nvme-subsys device");
>@@ -470,7 +486,7 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
>
>     if (!nsid) {
>         for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
>-            if (nvme_ns(n, i) || nvme_subsys_ns(subsys, i)) {
>+            if (nvme_subsys_ns(subsys, i) || nvme_ns(n, i)) {
>                 continue;
>             }
>
>@@ -483,7 +499,7 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
>             return;
>         }
>     } else {
>-        if (nvme_ns(n, nsid) || nvme_subsys_ns(subsys, nsid)) {
>+        if (nvme_subsys_ns(subsys, nsid) || nvme_ns(n, nsid)) {
>             error_setg(errp, "namespace id '%d' already allocated", nsid);
>             return;
>         }
>@@ -509,7 +525,9 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
>         }
>     }
>
>-    nvme_attach_ns(n, ns);
>+    if (n) {
>+        nvme_attach_ns(n, ns);
>+    }
> }
>
> static Property nvme_ns_props[] = {
>diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
>index 92caa604a280..fb7c3a7c55fc 100644
>--- a/hw/nvme/subsys.c
>+++ b/hw/nvme/subsys.c
>@@ -50,6 +50,10 @@ static void nvme_subsys_realize(DeviceState *dev, Error **errp)
> {
>     NvmeSubsystem *subsys = NVME_SUBSYS(dev);
>
>+    qbus_create_inplace(&subsys->bus, sizeof(NvmeBus), TYPE_NVME_BUS, dev,
>+                        dev->id);
>+    subsys->bus.is_subsys = true;
>+
>     nvme_subsys_setup(subsys);
> }
>
>-- 
>2.32.0
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]