[PATCH 0/1] hw/nvme: create parameter to enable/disable cmic on subsystem

[PATCH 0/1] hw/nvme: create parameter to enable/disable cmic on subsystem

[Alan Adamson]

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 bit was hardcoded to ON.
Most NVMe devices today don't set cmic to ON. Shouldn't the setting of
this bit be a settable parameter? 

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

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

Alan Adamson (1):
  hw/nvme: create parameter to enable/disable cmic on subsystem

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

-- 
2.43.5

[PATCH 1/1] hw/nvme: create parameter to enable/disable cmic on subsystem

[Alan Adamson]

Allow the value of CMIC to to be set via a new subsystem specific parameter.
This removes the requirement that all subsystems must have the CMIC bit enabled.

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

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

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 518d02dc6670..ddd3ec15d131 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -8876,7 +8876,10 @@ 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;
+    if (n->subsys->params.cmic) {
+        id->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 6f782ba18826..437b3e64fcfb 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -116,6 +116,7 @@ typedef struct NvmeSubsystem {
             uint16_t nruh;
             uint32_t nrg;
         } fdp;
+        bool         cmic;
     } params;
 } NvmeSubsystem;
 
diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
index 2ae56f12a596..ea6687c7ee6c 100644
--- a/hw/nvme/subsys.c
+++ b/hw/nvme/subsys.c
@@ -223,6 +223,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", NvmeSubsystem, params.cmic, false),
 };
 
 static void nvme_subsys_class_init(ObjectClass *oc, void *data)
-- 
2.43.5

Re: [PATCH 0/1] hw/nvme: create parameter to enable/disable cmic on subsystem

[Klaus Jensen]

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

On Apr  8 15:56, Alan Adamson wrote:
> 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 bit was hardcoded to ON.
> Most NVMe devices today don't set cmic to ON. Shouldn't the setting of
> this bit be a settable parameter? 
> 
> <subsystem>,cmic=BOOLEAN (default: off)
> 
> Example:
>     -device nvme-subsys,id=subsys0,cmic=on \
>     -device nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 \
>     -drive id=ns1,file=/dev/nullb3,if=none \
>     -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,shared=false 
> 
> Alan Adamson (1):
>   hw/nvme: create parameter to enable/disable cmic on subsystem
> 
>  hw/nvme/ctrl.c   | 5 ++++-
>  hw/nvme/nvme.h   | 1 +
>  hw/nvme/subsys.c | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> -- 
> 2.43.5
> 
> 

Hi Alan,

I agree that it would be better for CMIC.MCTRS to remain zero for
single-controller subsystems, but I'd rather not add the parameter
without verifying it (it would be invalid to have a multi-controller
subsystem with CMIC.MCTRS zeroed).

An improvement would be to just set it if the subsystems contains
multiple controllers.

Prior to commit cd59f50ab017 we would also statically set CMIC.MCTRS to
1 if a subsystem was configured, regardless of the number of
controllers.

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

Re: [PATCH 0/1] hw/nvme: create parameter to enable/disable cmic on subsystem

[alan.adamson]

On 4/8/25 11:47 PM, Klaus Jensen wrote:
> On Apr  8 15:56, Alan Adamson wrote:
>> 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 bit was hardcoded to ON.
>> Most NVMe devices today don't set cmic to ON. Shouldn't the setting of
>> this bit be a settable parameter?
>>
>> <subsystem>,cmic=BOOLEAN (default: off)
>>
>> Example:
>>      -device nvme-subsys,id=subsys0,cmic=on \
>>      -device nvme,serial=deadbeef,id=nvme0,subsys=subsys0,atomic.dn=off,atomic.awun=31,atomic.awupf=15 \
>>      -drive id=ns1,file=/dev/nullb3,if=none \
>>      -device nvme-ns,drive=ns1,bus=nvme0,nsid=1,shared=false
>>
>> Alan Adamson (1):
>>    hw/nvme: create parameter to enable/disable cmic on subsystem
>>
>>   hw/nvme/ctrl.c   | 5 ++++-
>>   hw/nvme/nvme.h   | 1 +
>>   hw/nvme/subsys.c | 1 +
>>   3 files changed, 6 insertions(+), 1 deletion(-)
>>
>> -- 
>> 2.43.5
>>
>>
> Hi Alan,
>
> I agree that it would be better for CMIC.MCTRS to remain zero for
> single-controller subsystems, but I'd rather not add the parameter
> without verifying it (it would be invalid to have a multi-controller
> subsystem with CMIC.MCTRS zeroed).
>
> An improvement would be to just set it if the subsystems contains
> multiple controllers.
>
> Prior to commit cd59f50ab017 we would also statically set CMIC.MCTRS to
> 1 if a subsystem was configured, regardless of the number of
> controllers.

Klaus,

I'll send out a v2 that automatically sets CMIC.MCTRS for each 
controller of a multi-controller subsystem.  For single controller 
subsystems, CMIC.MCTRS will default to off and the cmic parameter can be 
used to change CMIC.MCTRS to on.

Thanks!

Alan