[PATCH 0/2] hw/nvme: stable fixes

[PATCH 0/2] hw/nvme: stable fixes

[Klaus Jensen]

Two fixes for stable. See commits.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
Klaus Jensen (2):
      hw/nvme: fix namespace attachment
      hw/nvme: revert CMIC behavior

 hw/nvme/ctrl.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)
---
base-commit: 6322b753f798337835e205b6d805356bea582c86
change-id: 20250603-nvme-fixes-d3f93f5a9139

Best regards,
-- 
Klaus Jensen <k.jensen@samsung.com>

[PATCH 1/2] hw/nvme: fix namespace attachment

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Commit 6ccca4b6bb9f ("hw/nvme: rework csi handling") introduced a bug in
Namespace Attachment, causing it to

  a) not allow a controller to attach namespaces to other controllers
  b) assert if a valid non-attached namespace is detached

This fixes both issues.

Fixes: 6ccca4b6bb9f ("hw/nvme: rework csi handling")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2976
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fd935507bc0280c1f49822f9e3cb035df596ae47..8de900ef8aca9b510b072892f9f82c01acee4f7d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6816,7 +6816,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
 
         switch (sel) {
         case NVME_NS_ATTACHMENT_ATTACH:
-            if (nvme_ns(n, nsid)) {
+            if (nvme_ns(ctrl, nsid)) {
                 return NVME_NS_ALREADY_ATTACHED | NVME_DNR;
             }
 
@@ -6824,7 +6824,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
                 return NVME_NS_PRIVATE | NVME_DNR;
             }
 
-            if (!nvme_csi_supported(n, ns->csi)) {
+            if (!nvme_csi_supported(ctrl, ns->csi)) {
                 return NVME_IOCS_NOT_SUPPORTED | NVME_DNR;
             }
 
@@ -6834,6 +6834,10 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
             break;
 
         case NVME_NS_ATTACHMENT_DETACH:
+            if (!nvme_ns(ctrl, nsid)) {
+                return NVME_NS_NOT_ATTACHED | NVME_DNR;
+            }
+
             nvme_detach_ns(ctrl, ns);
             nvme_update_dsm_limits(ctrl, NULL);
 

-- 
2.47.2

[PATCH 2/2] hw/nvme: revert CMIC behavior

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Commit cd59f50ab017 ("hw/nvme: always initialize a subsystem") causes
the controller to always set the CMIC.MCTRS ("Multiple Controllers")
bit. While spec-compliant, this is a deviation from the previous
behavior where this was only set if an nvme-subsys device was explicitly
created (to configure a subsystem with multiple controllers/namespaces).

Revert the behavior to only set CMIC.MCTRS if an nvme-subsys device is
created explicitly.

Reported-by: Alan Adamson <alan.adamson@oracle.com>
Fixes: cd59f50ab017 ("hw/nvme: always initialize a subsystem")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8de900ef8aca9b510b072892f9f82c01acee4f7d..0637e29ec9fdcfe65a97b8bdcff7851091096d44 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -8780,7 +8780,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     uint8_t *pci_conf = pci_dev->config;
     uint64_t cap = ldq_le_p(&n->bar.cap);
     NvmeSecCtrlEntry *sctrl = nvme_sctrl(n);
-    uint32_t ctratt;
+    uint32_t ctratt = le32_to_cpu(id->ctratt);
     uint16_t oacs;
 
     memcpy(n->cse.acs, nvme_cse_acs_default, sizeof(n->cse.acs));
@@ -8798,10 +8798,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 
     id->oaes = cpu_to_le32(NVME_OAES_NS_ATTR);
 
-    ctratt = NVME_CTRATT_ELBAS;
+    ctratt |= NVME_CTRATT_ELBAS;
     if (n->params.ctratt.mem) {
         ctratt |= NVME_CTRATT_MEM;
     }
+    id->ctratt = cpu_to_le32(ctratt);
 
     id->rab = 6;
 
@@ -8884,17 +8885,6 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
     id->psd[0].enlat = cpu_to_le32(0x10);
     id->psd[0].exlat = cpu_to_le32(0x4);
 
-    id->cmic |= NVME_CMIC_MULTI_CTRL;
-    ctratt |= NVME_CTRATT_ENDGRPS;
-
-    id->endgidmax = cpu_to_le16(0x1);
-
-    if (n->subsys->endgrp.fdp.enabled) {
-        ctratt |= NVME_CTRATT_FDPS;
-    }
-
-    id->ctratt = cpu_to_le32(ctratt);
-
     NVME_CAP_SET_MQES(cap, n->params.mqes);
     NVME_CAP_SET_CQR(cap, 1);
     NVME_CAP_SET_TO(cap, 0xf);
@@ -8927,6 +8917,20 @@ static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
         }
 
         n->subsys = NVME_SUBSYS(dev);
+    } else {
+        NvmeIdCtrl *id = &n->id_ctrl;
+        uint32_t ctratt = le32_to_cpu(id->ctratt);
+
+        id->cmic |= NVME_CMIC_MULTI_CTRL;
+        ctratt |= NVME_CTRATT_ENDGRPS;
+
+        id->endgidmax = cpu_to_le16(0x1);
+
+        if (n->subsys->endgrp.fdp.enabled) {
+            ctratt |= NVME_CTRATT_FDPS;
+        }
+
+        id->ctratt = cpu_to_le32(ctratt);
     }
 
     cntlid = nvme_subsys_register_ctrl(n, errp);

-- 
2.47.2

Re: [PATCH 2/2] hw/nvme: revert CMIC behavior

[alan.adamson]

On 6/3/25 5:59 AM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Commit cd59f50ab017 ("hw/nvme: always initialize a subsystem") causes
> the controller to always set the CMIC.MCTRS ("Multiple Controllers")
> bit. While spec-compliant, this is a deviation from the previous
> behavior where this was only set if an nvme-subsys device was explicitly
> created (to configure a subsystem with multiple controllers/namespaces).
>
> Revert the behavior to only set CMIC.MCTRS if an nvme-subsys device is
> created explicitly.
>
> Reported-by: Alan Adamson <alan.adamson@oracle.com>
> Fixes: cd59f50ab017 ("hw/nvme: always initialize a subsystem")
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---

Reviewed-by: Alan Adamson <alan.adamson@oracle.com>

Tested-by: Alan Adamson <alan.adamson@oracle.com>


                                 Before 
Fix                                      With Klaus's Fix
subsys with 1 controller        # nvme id-ctrl /dev/nvme0n1 | grep 
cmic         # nvme id-ctrl /dev/nvme0n1 | grep cmic
                                 cmic      : 
0x2                                 cmic      : 0x2

subsys with 2 controllers       # nvme id-ctrl /dev/nvme0n1 | grep 
cmic         # nvme id-ctrl /dev/nvme0n1 | grep cmic
                                 cmic      : 
0x2                                 cmic      : 0x2
                                 # nvme id-ctrl /dev/nvme0n2 | grep 
cmic         # nvme id-ctrl /dev/nvme0n2 | grep cmic
                                 cmic      : 
0x2                                 cmic      : 0x2

no subsys with 2 controllers    # nvme id-ctrl /dev/nvme0n1 | grep 
cmic         # nvme id-ctrl /dev/nvme0n1 | grep cmic
                                 cmic      : 
0x2                                 cmic      : 0x0
                                 # nvme id-ctrl /dev/nvme1n1 | grep 
cmic         # nvme id-ctrl /dev/nvme1n1 | grep cmic
                                 cmic      : 
0x2                                 cmic      : 0x0


A bug in the Linux Atomic Write feature resulted in the 
atomic_write_max_bytes being set
to zero when cmic=0x2.

# uname -a
Linux localhost.localdomain 6.15.0-rc3+ #1 SMP PREEMPT_DYNAMIC Tue Apr 
22 14:34:09 PDT 2025 x86_64 x86_64 x86_64 GNU/Linux
# nvme id-ctrl /dev/nvme0n1 | grep cmic
cmic      : 0
# cat /sys/block/nvme0n1/queue/atomic_write_max_bytes
0

With this fix:

# nvme id-ctrl /dev/nvme0n1
cmic      : 0
# cat /sys/block/nvme0n1/queue/atomic_write_max_bytes
8192

Re: [PATCH 0/2] hw/nvme: stable fixes

[Michael Tokarev]

On 03.06.2025 15:59, Klaus Jensen wrote:
> Two fixes for stable. See commits.

What do you mean "for stable"?
Are these not for master but for stable *only*?

Usually changes for qemu-stable are picked up *from*
master branch, unless there are major changes in
stable already.

Thanks,

/mjt

Re: [PATCH 0/2] hw/nvme: stable fixes

[Klaus Jensen]

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

On Jun  4 10:21, Michael Tokarev wrote:
> On 03.06.2025 15:59, Klaus Jensen wrote:
> > Two fixes for stable. See commits.
> 
> What do you mean "for stable"?
> Are these not for master but for stable *only*?
> 
> Usually changes for qemu-stable are picked up *from*
> master branch, unless there are major changes in
> stable already.
> 
> Thanks,
> 
> /mjt

Hi Michael,

I was intending to CC to qemu-stable when I do the PULL to master after
I get reviews :) The intention was not for you to pick it up
immediately.

My apologies if the word "stable" in the subject caught your radar
unintentionally! :)

Thanks,
Klaus

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

Re: [PATCH 1/2] hw/nvme: fix namespace attachment

[Jesper Devantier]

On Tue Jun 3, 2025 at 2:59 PM CEST, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Commit 6ccca4b6bb9f ("hw/nvme: rework csi handling") introduced a bug in
> Namespace Attachment, causing it to
>
>   a) not allow a controller to attach namespaces to other controllers
>   b) assert if a valid non-attached namespace is detached
>
> This fixes both issues.
>
> Fixes: 6ccca4b6bb9f ("hw/nvme: rework csi handling")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2976
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index fd935507bc0280c1f49822f9e3cb035df596ae47..8de900ef8aca9b510b072892f9f82c01acee4f7d 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6816,7 +6816,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>  
>          switch (sel) {
>          case NVME_NS_ATTACHMENT_ATTACH:
> -            if (nvme_ns(n, nsid)) {
> +            if (nvme_ns(ctrl, nsid)) {
>                  return NVME_NS_ALREADY_ATTACHED | NVME_DNR;
>              }
>  
> @@ -6824,7 +6824,7 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>                  return NVME_NS_PRIVATE | NVME_DNR;
>              }
>  
> -            if (!nvme_csi_supported(n, ns->csi)) {
> +            if (!nvme_csi_supported(ctrl, ns->csi)) {
>                  return NVME_IOCS_NOT_SUPPORTED | NVME_DNR;
>              }
>  
> @@ -6834,6 +6834,10 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
>              break;
>  
>          case NVME_NS_ATTACHMENT_DETACH:
> +            if (!nvme_ns(ctrl, nsid)) {
> +                return NVME_NS_NOT_ATTACHED | NVME_DNR;
> +            }
> +
>              nvme_detach_ns(ctrl, ns);
>              nvme_update_dsm_limits(ctrl, NULL);
>  

Applied, built, read, and it seems in line with the specification :)

Reviewed-by: Jesper Wendel Devantier <foss@defmacro.it>