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

Klaus Jensen posted 2 patches 5 months, 2 weeks ago
Maintainers: Keith Busch <kbusch@kernel.org>, Klaus Jensen <its@irrelevant.dk>, Jesper Devantier <foss@defmacro.it>
[PATCH 1/2] hw/nvme: fix namespace attachment
Posted by Klaus Jensen 5 months, 2 weeks ago
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
Re: [PATCH 1/2] hw/nvme: fix namespace attachment
Posted by Jesper Devantier 5 months ago
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>