[PATCH] hw/block/nvme: re-enable NVMe PCI hotplug

Hannes Reinecke posted 1 patch 2 years, 11 months ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210511073511.32511-1-hare@suse.de
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>
capstone               |  2 +-
hw/block/nvme-ns.c     | 31 ++++++++++++++++++++++
hw/block/nvme-subsys.c | 12 +++++++++
hw/block/nvme-subsys.h |  1 +
hw/block/nvme.c        | 60 +++++++++++++++++++++++++++++++-----------
hw/block/nvme.h        |  1 +
roms/SLOF              |  2 +-
roms/openbios          |  2 +-
roms/u-boot            |  2 +-
9 files changed, 93 insertions(+), 20 deletions(-)
[PATCH] hw/block/nvme: re-enable NVMe PCI hotplug
Posted by Hannes Reinecke 2 years, 11 months ago
Ever since commit e570768566 ("hw/block/nvme: support for shared
namespace in subsystem") NVMe PCI hotplug is broken, as the PCI
hotplug infrastructure will only work for the nvme devices (which
are PCI devices), but not for any attached namespaces.
So when re-adding the NVMe PCI device via 'device_add' the NVMe
controller is added, but all namespaces are missing.
This patch adds device hotplug hooks for NVMe namespaces, such that one
can call 'device_add nvme-ns' to (re-)attach the namespaces after
the PCI NVMe device 'device_add nvme' hotplug call.

Fixes: e570768566 ("hw/block/nvme: support for shared namespace in subsystem")
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 capstone               |  2 +-
 hw/block/nvme-ns.c     | 31 ++++++++++++++++++++++
 hw/block/nvme-subsys.c | 12 +++++++++
 hw/block/nvme-subsys.h |  1 +
 hw/block/nvme.c        | 60 +++++++++++++++++++++++++++++++-----------
 hw/block/nvme.h        |  1 +
 roms/SLOF              |  2 +-
 roms/openbios          |  2 +-
 roms/u-boot            |  2 +-
 9 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/capstone b/capstone
index f8b1b83301..22ead3e0bf 160000
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit f8b1b833015a4ae47110ed068e0deb7106ced66d
+Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 7bb618f182..3a7e01f10f 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -526,6 +526,36 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
     nvme_attach_ns(n, ns);
 }
 
+static void nvme_ns_unrealize(DeviceState *dev)
+{
+    NvmeNamespace *ns = NVME_NS(dev);
+    BusState *s = qdev_get_parent_bus(dev);
+    NvmeCtrl *n = NVME(s->parent);
+    NvmeSubsystem *subsys = n->subsys;
+    uint32_t nsid = ns->params.nsid;
+    int i;
+
+    nvme_ns_drain(ns);
+    nvme_ns_shutdown(ns);
+    nvme_ns_cleanup(ns);
+
+    if (subsys) {
+        subsys->namespaces[nsid] = NULL;
+
+        if (ns->params.shared) {
+            for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
+                NvmeCtrl *ctrl = subsys->ctrls[i];
+
+                if (ctrl) {
+                    nvme_detach_ns(ctrl, ns);
+                }
+            }
+            return;
+        }
+    }
+    nvme_detach_ns(n, ns);
+}
+
 static Property nvme_ns_props[] = {
     DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
     DEFINE_PROP_BOOL("detached", NvmeNamespace, params.detached, false),
@@ -563,6 +593,7 @@ static void nvme_ns_class_init(ObjectClass *oc, void *data)
 
     dc->bus_type = TYPE_NVME_BUS;
     dc->realize = nvme_ns_realize;
+    dc->unrealize = nvme_ns_unrealize;
     device_class_set_props(dc, nvme_ns_props);
     dc->desc = "Virtual NVMe namespace";
 }
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index 9604c19117..1c00508f33 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -42,6 +42,18 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
     return cntlid;
 }
 
+void nvme_subsys_unregister_ctrl(NvmeCtrl *n)
+{
+    NvmeSubsystem *subsys = n->subsys;
+    int cntlid = n->cntlid;
+
+    if (!n->subsys)
+        return;
+    assert(cntlid < ARRAY_SIZE(subsys->ctrls));
+    subsys->ctrls[cntlid] = NULL;
+    n->cntlid = -1;
+}
+
 static void nvme_subsys_setup(NvmeSubsystem *subsys)
 {
     const char *nqn = subsys->params.nqn ?
diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index 7d7ef5f7f1..2d8a146c4f 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -32,6 +32,7 @@ typedef struct NvmeSubsystem {
 } NvmeSubsystem;
 
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
+void nvme_subsys_unregister_ctrl(NvmeCtrl *n);
 
 static inline NvmeCtrl *nvme_subsys_ctrl(NvmeSubsystem *subsys,
         uint32_t cntlid)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5fe082ec34..515678b686 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4963,26 +4963,12 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
             }
 
             nvme_attach_ns(ctrl, ns);
-            __nvme_select_ns_iocs(ctrl, ns);
         } else {
             if (!nvme_ns(ctrl, nsid)) {
                 return NVME_NS_NOT_ATTACHED | NVME_DNR;
             }
 
-            ctrl->namespaces[nsid - 1] = NULL;
-            ns->attached--;
-
-            nvme_update_dmrsl(ctrl);
-        }
-
-        /*
-         * Add namespace id to the changed namespace id list for event clearing
-         * via Get Log Page command.
-         */
-        if (!test_and_set_bit(nsid, ctrl->changed_nsids)) {
-            nvme_enqueue_event(ctrl, NVME_AER_TYPE_NOTICE,
-                               NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
-                               NVME_LOG_CHANGED_NSLIST);
+            nvme_detach_ns(ctrl, ns);
         }
     }
 
@@ -6166,6 +6152,34 @@ void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns)
 
     n->dmrsl = MIN_NON_ZERO(n->dmrsl,
                             BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+    if (NVME_CC_EN(n->bar.cc)) {
+        /* Ctrl is live */
+        __nvme_select_ns_iocs(n, ns);
+        if (!test_and_set_bit(nsid, n->changed_nsids)) {
+            nvme_enqueue_event(n, NVME_AER_TYPE_NOTICE,
+                               NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
+                               NVME_LOG_CHANGED_NSLIST);
+        }
+    }
+}
+
+void nvme_detach_ns(NvmeCtrl *n, NvmeNamespace *ns)
+{
+    uint32_t nsid = ns->params.nsid;
+
+    if (ns->attached) {
+        n->namespaces[nsid - 1] = NULL;
+        ns->attached--;
+    }
+    nvme_update_dmrsl(n);
+    if (NVME_CC_EN(n->bar.cc)) {
+        /* Ctrl is live */
+        if (!test_and_set_bit(nsid, n->changed_nsids)) {
+            nvme_enqueue_event(n, NVME_AER_TYPE_NOTICE,
+                               NVME_AER_INFO_NOTICE_NS_ATTR_CHANGED,
+                               NVME_LOG_CHANGED_NSLIST);
+        }
+    }
 }
 
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
@@ -6193,7 +6207,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
     nvme_init_ctrl(n, pci_dev);
-
+    qbus_set_bus_hotplug_handler(BUS(&n->bus));
     /* setup a namespace if the controller drive property was given */
     if (n->namespace.blkconf.blk) {
         ns = &n->namespace;
@@ -6224,6 +6238,8 @@ static void nvme_exit(PCIDevice *pci_dev)
         nvme_ns_cleanup(ns);
     }
 
+    nvme_subsys_unregister_ctrl(n);
+
     g_free(n->cq);
     g_free(n->sq);
     g_free(n->aer_reqs);
@@ -6348,10 +6364,22 @@ static const TypeInfo nvme_info = {
     },
 };
 
+static void nvme_bus_class_init(ObjectClass *klass, void *data)
+{
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
+
+    hc->unplug = qdev_simple_device_unplug_cb;
+}
+
 static const TypeInfo nvme_bus_info = {
     .name = TYPE_NVME_BUS,
     .parent = TYPE_BUS,
     .instance_size = sizeof(NvmeBus),
+    .class_init = nvme_bus_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void nvme_register_types(void)
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 5d05ec368f..4fc06f58a4 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -255,6 +255,7 @@ typedef enum NvmeTxDirection {
 } NvmeTxDirection;
 
 void nvme_attach_ns(NvmeCtrl *n, NvmeNamespace *ns);
+void nvme_detach_ns(NvmeCtrl *n, NvmeNamespace *ns);
 uint16_t nvme_bounce_data(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
                           NvmeTxDirection dir, NvmeRequest *req);
 uint16_t nvme_bounce_mdata(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
diff --git a/roms/SLOF b/roms/SLOF
index 33a7322de1..e18ddad851 160000
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit 33a7322de13e9dca4b38851a345a58d37e7a441d
+Subproject commit e18ddad8516ff2cfe36ec130200318f7251aa78c
diff --git a/roms/openbios b/roms/openbios
index 4a0041107b..7f28286f5c 160000
--- a/roms/openbios
+++ b/roms/openbios
@@ -1 +1 @@
-Subproject commit 4a0041107b8ef77e0e8337bfcb5f8078887261a7
+Subproject commit 7f28286f5cb1ca682e3ba0a8706d8884f12bc49e
diff --git a/roms/u-boot b/roms/u-boot
index b46dd116ce..d3689267f9 160000
--- a/roms/u-boot
+++ b/roms/u-boot
@@ -1 +1 @@
-Subproject commit b46dd116ce03e235f2a7d4843c6278e1da44b5e1
+Subproject commit d3689267f92c5956e09cc7d1baa4700141662bff
-- 
2.26.2


Re: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug
Posted by no-reply@patchew.org 2 years, 11 months ago
Patchew URL: https://patchew.org/QEMU/20210511073511.32511-1-hare@suse.de/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210511073511.32511-1-hare@suse.de
Subject: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   e58c7a3..e4f3ede  master     -> master
 * [new tag]         patchew/20210511073511.32511-1-hare@suse.de -> patchew/20210511073511.32511-1-hare@suse.de
Switched to a new branch 'test'
1d24622 hw/block/nvme: re-enable NVMe PCI hotplug

=== OUTPUT BEGIN ===
ERROR: braces {} are necessary for all arms of this statement
#101: FILE: hw/block/nvme-subsys.c:50:
+    if (!n->subsys)
[...]

total: 1 errors, 0 warnings, 182 lines checked

Commit 1d24622c5d19 (hw/block/nvme: re-enable NVMe PCI hotplug) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210511073511.32511-1-hare@suse.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
Cc'ing Klaus (maintainer)

On 5/11/21 9:35 AM, Hannes Reinecke wrote:
> Ever since commit e570768566 ("hw/block/nvme: support for shared
> namespace in subsystem") NVMe PCI hotplug is broken, as the PCI
> hotplug infrastructure will only work for the nvme devices (which
> are PCI devices), but not for any attached namespaces.
> So when re-adding the NVMe PCI device via 'device_add' the NVMe
> controller is added, but all namespaces are missing.
> This patch adds device hotplug hooks for NVMe namespaces, such that one
> can call 'device_add nvme-ns' to (re-)attach the namespaces after
> the PCI NVMe device 'device_add nvme' hotplug call.
> 
> Fixes: e570768566 ("hw/block/nvme: support for shared namespace in subsystem")
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  capstone               |  2 +-

>  roms/SLOF              |  2 +-
>  roms/openbios          |  2 +-
>  roms/u-boot            |  2 +-
>  9 files changed, 93 insertions(+), 20 deletions(-)
> 
> diff --git a/capstone b/capstone
> index f8b1b83301..22ead3e0bf 160000
> --- a/capstone
> +++ b/capstone
> @@ -1 +1 @@
> -Subproject commit f8b1b833015a4ae47110ed068e0deb7106ced66d
> +Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf

> index 33a7322de1..e18ddad851 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 33a7322de13e9dca4b38851a345a58d37e7a441d
> +Subproject commit e18ddad8516ff2cfe36ec130200318f7251aa78c
> diff --git a/roms/openbios b/roms/openbios
> index 4a0041107b..7f28286f5c 160000
> --- a/roms/openbios
> +++ b/roms/openbios
> @@ -1 +1 @@
> -Subproject commit 4a0041107b8ef77e0e8337bfcb5f8078887261a7
> +Subproject commit 7f28286f5cb1ca682e3ba0a8706d8884f12bc49e
> diff --git a/roms/u-boot b/roms/u-boot
> index b46dd116ce..d3689267f9 160000
> --- a/roms/u-boot
> +++ b/roms/u-boot
> @@ -1 +1 @@
> -Subproject commit b46dd116ce03e235f2a7d4843c6278e1da44b5e1
> +Subproject commit d3689267f92c5956e09cc7d1baa4700141662bff
> 

Submodule changes unlikely related.