hw/nvme: fix attachment of private namespaces

Commit Message

Klaus Jensen April 8, 2025, 10:20 a.m. UTC

From: Klaus Jensen <[e-mail]>

Fix regression when attaching private namespaces that gets attached to
the wrong controller.

Keep track of the original controller "owner" of private namespaces, and
only attach if this matches on controller enablement.

Fixes: 6ccca4b6bb9f ("hw/nvme: rework csi handling")
Reported-by: Alan Adamson <[e-mail]>
Suggested-by: Alan Adamson <[e-mail]>
Signed-off-by: Klaus Jensen <[e-mail]>
---
 hw/nvme/ctrl.c   | 7 ++++++-
 hw/nvme/ns.c     | 4 ++++
 hw/nvme/nvme.h   | 3 +++
 hw/nvme/subsys.c | 9 +--------
 4 files changed, 14 insertions(+), 9 deletions(-)


---
base-commit: dfaecc04c46d298e9ee81bd0ca96d8754f1c27ed
change-id: 20250408-fix-private-ns-19b2bdf62696

Best regards,

Comments

Keith Busch April 8, 2025, 10:23 a.m. UTC | #1

On Tue, Apr 08, 2025 at 12:20:46PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <[e-mail]>
> 
> Fix regression when attaching private namespaces that gets attached to
> the wrong controller.
> 
> Keep track of the original controller "owner" of private namespaces, and
> only attach if this matches on controller enablement.

Looks good.

Reviewed-by: Keith Busch <[e-mail]>

Alan Adamson April 8, 2025, 4:41 p.m. UTC | #2

On 4/8/25 3:20 AM, Klaus Jensen wrote:
> From: Klaus Jensen <[e-mail]>
>
> Fix regression when attaching private namespaces that gets attached to
> the wrong controller.
>
> Keep track of the original controller "owner" of private namespaces, and
> only attach if this matches on controller enablement.

Tested-by: Alan Adamson <[e-mail]>

Reviewed-by: Alan Adamson <[e-mail]>

Philippe Mathieu-Daudé April 8, 2025, 6:56 p.m. UTC | #3

On 8/4/25 12:20, Klaus Jensen wrote:
> From: Klaus Jensen <[e-mail]>
> 
> Fix regression when attaching private namespaces that gets attached to
> the wrong controller.
> 
> Keep track of the original controller "owner" of private namespaces, and
> only attach if this matches on controller enablement.
> 
> Fixes: 6ccca4b6bb9f ("hw/nvme: rework csi handling")
> Reported-by: Alan Adamson <[e-mail]>
> Suggested-by: Alan Adamson <[e-mail]>
> Signed-off-by: Klaus Jensen <[e-mail]>
> ---
>   hw/nvme/ctrl.c   | 7 ++++++-
>   hw/nvme/ns.c     | 4 ++++
>   hw/nvme/nvme.h   | 3 +++
>   hw/nvme/subsys.c | 9 +--------
>   4 files changed, 14 insertions(+), 9 deletions(-)


Patch queued, thanks!

Klaus Jensen April 9, 2025, 6:36 a.m. UTC | #4

On Apr  8 20:56, Philippe Mathieu-Daudé wrote:
> On 8/4/25 12:20, Klaus Jensen wrote:
> > From: Klaus Jensen <[e-mail]>
> > 
> > Fix regression when attaching private namespaces that gets attached to
> > the wrong controller.
> > 
> > Keep track of the original controller "owner" of private namespaces, and
> > only attach if this matches on controller enablement.
> > 
> > Fixes: 6ccca4b6bb9f ("hw/nvme: rework csi handling")
> > Reported-by: Alan Adamson <[e-mail]>
> > Suggested-by: Alan Adamson <[e-mail]>
> > Signed-off-by: Klaus Jensen <[e-mail]>
> > ---
> >   hw/nvme/ctrl.c   | 7 ++++++-
> >   hw/nvme/ns.c     | 4 ++++
> >   hw/nvme/nvme.h   | 3 +++
> >   hw/nvme/subsys.c | 9 +--------
> >   4 files changed, 14 insertions(+), 9 deletions(-)
> 
> 
> Patch queued, thanks!

Hi Philippe,

Thanks for picking this up!

Patch

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 518d02dc66706e2d2e86f1705db52188a97a67fc..d6b77d4fbc9def4639d53074c93f35ca882c4a02 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7755,7 +7755,11 @@  static int nvme_start_ctrl(NvmeCtrl *n)
     for (int i = 1; i <= NVME_MAX_NAMESPACES; i++) {
         NvmeNamespace *ns = nvme_subsys_ns(n->subsys, i);
 
-        if (ns && nvme_csi_supported(n, ns->csi) && !ns->params.detached) {
+        if (!ns || (!ns->params.shared && ns->ctrl != n)) {
+            continue;
+        }
+
+        if (nvme_csi_supported(n, ns->csi) && !ns->params.detached) {
             if (!ns->attached || ns->params.shared) {
                 nvme_attach_ns(n, ns);
             }
@@ -8988,6 +8992,7 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     if (n->namespace.blkconf.blk) {
         ns = &n->namespace;
         ns->params.nsid = 1;
+        ns->ctrl = n;
 
         if (nvme_ns_setup(ns, errp)) {
             return;
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 98c1e75a5d29627351f1aa741da3625c984a2d40..4ab8ba74f51b346a50419869b6f4a7f4b2d0e9c2 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -763,6 +763,10 @@  static void nvme_ns_realize(DeviceState *dev, Error **errp)
 
     ns->id_ns.endgid = cpu_to_le16(0x1);
     ns->id_ns_ind.endgrpid = cpu_to_le16(0x1);
+
+    if (!ns->params.shared) {
+        ns->ctrl = n;
+    }
 }
 
 static const Property nvme_ns_props[] = {
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 6f782ba18826d3ff8db7198d3a29c7654262bb7b..b5c9378ea4e524abacced613fbc4ce5a404350c0 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -268,6 +268,9 @@  typedef struct NvmeNamespace {
     NvmeSubsystem *subsys;
     NvmeEnduranceGroup *endgrp;
 
+    /* NULL for shared namespaces; set to specific controller if private */
+    NvmeCtrl *ctrl;
+
     struct {
         uint32_t err_rec;
     } features;
diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 2ae56f12a596198e93a118428579301f8c8275d8..b617ac3892a32efebcaedca837eff59104dcc751 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -56,7 +56,7 @@  int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 {
     NvmeSubsystem *subsys = n->subsys;
     NvmeSecCtrlEntry *sctrl = nvme_sctrl(n);
-    int cntlid, nsid, num_rsvd, num_vfs = n->params.sriov_max_vfs;
+    int cntlid, num_rsvd, num_vfs = n->params.sriov_max_vfs;
 
     if (pci_is_vf(&n->parent_obj)) {
         cntlid = le16_to_cpu(sctrl->scid);
@@ -92,13 +92,6 @@  int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
 
     subsys->ctrls[cntlid] = n;
 
-    for (nsid = 1; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {
-        NvmeNamespace *ns = subsys->namespaces[nsid];
-        if (ns && ns->params.shared && !ns->params.detached) {
-            nvme_attach_ns(n, ns);
-        }
-    }
-
     return cntlid;
 }