[PATCH v3 0/1] hw/nvme: CMIC.MCTRS should be set automatically for multi-controller subsystems or by parameter

[PATCH v3 0/1] hw/nvme: CMIC.MCTRS should be set automatically for multi-controller subsystems or by parameter

[Alan Adamson]

v3: - Sync up with v10.0

v2: - Change the parameter name from "cmic" to "cmic-mctrs".
    - If there is more than 1 controller in a subsystem, set CMIC.MCTRS
      for each controller whether or not the cmic-mctrs parameter is set.

While testing Linux atomic writes with qemu-nvme v10.0.0-rc1, Linux was
incorrectly displaying atomic_write_max_bytes
# cat /sys/block/nvme0n1/queue/atomic_write_max_bytes
0
# nvme id-ctrl /dev/nvme0n1 | grep awupf
awupf     : 15
#
Since AWUPF was set to 15, it was expected atomic_write_max_bytes would
be set to 8192.

The commit cd59f50ab017 ("hw/nvme: always initialize a subsystem")
introduced this behavior. The commit hardcodes the subsystem cmic bit
to ON which caused the Linux NVMe driver to treat the namespace as
multi-pathed which uncovered a bug with how Atomic Write Queue Limits
were being inherited.  This Linux issue is being addressed, but the
question was asked of why the subsystem CMIC.MCTRS bit was hardcoded to ON.
Most NVMe devices today don't set CMIC.MCTRS  to ON. Shouldn't the setting
of this bit be a settable parameter?


Proposal:

- The default setting of the CMIC.MCTRS bit will be OFF.

- If there is more than 1 controller detected in a subsystem, the CMIC.MCTRS
  bit will be set to ON for each controller in the subsystem.

- Create a subsystem specific parameter (cmic-mctrs) to specify CMIC.MCTRS
  in one controller subsystems.  This parameter does not affect
  multi-controller subsystems.

  <subsystem>,cmic-mctrs=BOOLEAN (default: off)

  Example:
    -device nvme-subsys,id=subsys0,cmic-mctrs=on \
    -device nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 \
    -drive id=ns1,file=/dev/nullb0,if=none \
    -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,shared=false


Alan Adamson (1):
  hw/nvme: CMIC.MCTRS should be set automatically for multi-controller
    subsystems or by parameter

 hw/nvme/ctrl.c   | 15 ++++++++++++++-
 hw/nvme/nvme.h   |  2 ++
 hw/nvme/subsys.c |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.43.5

[PATCH v3 1/1] hw/nvme: CMIC.MCTRS should be set automatically for multi-controller subsystems or by parameter

[Alan Adamson]

If there are multiple controllers in a subsystem, CMIC.MCTRS should be set to on
for all controllers. For single controller subsystems, CMIC.MCTRS will be off by
default. A new subsystem specific parameter will allow setting CMIC.MCTRS for
single controller subsystems.

New NVMe Subsystem QEMU Parameter (See NVMe Specification for details):
    <subsystem>,cmic-mctrs=BOOLEAN (default: off)

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 hw/nvme/ctrl.c   | 15 ++++++++++++++-
 hw/nvme/nvme.h   |  2 ++
 hw/nvme/subsys.c |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index fd935507bc02..72e45f3a7f78 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -8880,7 +8880,20 @@ 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;
+    n->subsys->total_ctrls++;
+
+    /* Check if there are more than 2 controllers or cmic.mctrs is enabled */
+    if (n->subsys->params.cmic_mctrs || (n->subsys->total_ctrls > 2)) {
+        id->cmic |= NVME_CMIC_MULTI_CTRL;
+    } else if (n->subsys->total_ctrls == 2) {
+        /*
+         * When the 2nd controller on this subsys is inited, CMIC.MCTRS
+         * needs to be set. Also need to go back and set CMIC.MCTRS
+         * on the first controller.
+         */
+        id->cmic |= NVME_CMIC_MULTI_CTRL;
+        n->subsys->ctrls[0]->id_ctrl.cmic |= NVME_CMIC_MULTI_CTRL;
+    }
     ctratt |= NVME_CTRATT_ENDGRPS;
 
     id->endgidmax = cpu_to_le16(0x1);
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index b5c9378ea4e5..061e7046550b 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -116,7 +116,9 @@ typedef struct NvmeSubsystem {
             uint16_t nruh;
             uint32_t nrg;
         } fdp;
+        bool         cmic_mctrs;
     } params;
+    uint8_t          total_ctrls;
 } NvmeSubsystem;
 
 int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 38271d78c8bd..c644fdf0be5e 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -216,6 +216,7 @@ static const Property nvme_subsystem_props[] = {
                      NVME_DEFAULT_RU_SIZE),
     DEFINE_PROP_UINT32("fdp.nrg", NvmeSubsystem, params.fdp.nrg, 1),
     DEFINE_PROP_UINT16("fdp.nruh", NvmeSubsystem, params.fdp.nruh, 0),
+    DEFINE_PROP_BOOL("cmic.mctrs", NvmeSubsystem, params.cmic_mctrs, false),
 };
 
 static void nvme_subsys_class_init(ObjectClass *oc, const void *data)
-- 
2.43.5

Re: [PATCH v3 1/1] hw/nvme: CMIC.MCTRS should be set automatically for multi-controller subsystems or by parameter

[alan.adamson]

Reposting.


Alan

On 5/1/25 11:45 AM, Alan Adamson wrote:
> If there are multiple controllers in a subsystem, CMIC.MCTRS should be set to on
> for all controllers. For single controller subsystems, CMIC.MCTRS will be off by
> default. A new subsystem specific parameter will allow setting CMIC.MCTRS for
> single controller subsystems.
>
> New NVMe Subsystem QEMU Parameter (See NVMe Specification for details):
>      <subsystem>,cmic-mctrs=BOOLEAN (default: off)
>
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---
>   hw/nvme/ctrl.c   | 15 ++++++++++++++-
>   hw/nvme/nvme.h   |  2 ++
>   hw/nvme/subsys.c |  1 +
>   3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index fd935507bc02..72e45f3a7f78 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -8880,7 +8880,20 @@ 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;
> +    n->subsys->total_ctrls++;
> +
> +    /* Check if there are more than 2 controllers or cmic.mctrs is enabled */
> +    if (n->subsys->params.cmic_mctrs || (n->subsys->total_ctrls > 2)) {
> +        id->cmic |= NVME_CMIC_MULTI_CTRL;
> +    } else if (n->subsys->total_ctrls == 2) {
> +        /*
> +         * When the 2nd controller on this subsys is inited, CMIC.MCTRS
> +         * needs to be set. Also need to go back and set CMIC.MCTRS
> +         * on the first controller.
> +         */
> +        id->cmic |= NVME_CMIC_MULTI_CTRL;
> +        n->subsys->ctrls[0]->id_ctrl.cmic |= NVME_CMIC_MULTI_CTRL;
> +    }
>       ctratt |= NVME_CTRATT_ENDGRPS;
>   
>       id->endgidmax = cpu_to_le16(0x1);
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index b5c9378ea4e5..061e7046550b 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -116,7 +116,9 @@ typedef struct NvmeSubsystem {
>               uint16_t nruh;
>               uint32_t nrg;
>           } fdp;
> +        bool         cmic_mctrs;
>       } params;
> +    uint8_t          total_ctrls;
>   } NvmeSubsystem;
>   
>   int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp);
> diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
> index 38271d78c8bd..c644fdf0be5e 100644
> --- a/hw/nvme/subsys.c
> +++ b/hw/nvme/subsys.c
> @@ -216,6 +216,7 @@ static const Property nvme_subsystem_props[] = {
>                        NVME_DEFAULT_RU_SIZE),
>       DEFINE_PROP_UINT32("fdp.nrg", NvmeSubsystem, params.fdp.nrg, 1),
>       DEFINE_PROP_UINT16("fdp.nruh", NvmeSubsystem, params.fdp.nruh, 0),
> +    DEFINE_PROP_BOOL("cmic.mctrs", NvmeSubsystem, params.cmic_mctrs, false),
>   };
>   
>   static void nvme_subsys_class_init(ObjectClass *oc, const void *data)