[PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[John Berg]

From: John Berg <jhnberg@amazon.com>

The MQES field in the CAP register describes the Maximum Queue Entries
Supported for the IO queues of an NVMe controller. Adding a +1 to the
value in this field results in the total queue size. A full queue is
when a queue of size N contains N - 1 entries, and the minimum queue
size is 2. Thus the lowest MQES value is 1.

This patch adds the new mqes property to the NVMe emulation which allows
a user to specify the maximum queue size by setting this property. This
is useful as it enables testing of NVMe controller where the MQES is
relatively small. The smallest NVMe queue size supported in NVMe is 2
submission and completion entries, which means that the smallest legal
mqes value is 1.

The following example shows how the mqes can be set for a the NVMe
emulation:

-drive id=nvme0,if=none,file=nvme.img,format=raw
-device nvme,drive=nvme0,serial=foo,mqes=1

If the mqes property is not provided then the default mqes will still be
0x7ff (the queue size is 2048 entries).

Signed-off-by: John Berg <jhnberg@amazon.co.uk>
---
 hw/nvme/ctrl.c | 9 ++++++++-
 hw/nvme/nvme.h | 1 +
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 127c3d2383..86cda9bc73 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7805,6 +7805,12 @@ static bool nvme_check_params(NvmeCtrl *n, Error **errp)
         return false;
     }
 
+    if (params->mqes < 1)
+    {
+        error_setg(errp, "mqes property cannot be less than 1");
+        return false;
+    }
+
     if (n->pmr.dev) {
         if (params->msix_exclusive_bar) {
             error_setg(errp, "not enough BARs available to enable PMR");
@@ -8281,7 +8287,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 
     id->ctratt = cpu_to_le32(ctratt);
 
-    NVME_CAP_SET_MQES(cap, 0x7ff);
+    NVME_CAP_SET_MQES(cap, n->params.mqes);
     NVME_CAP_SET_CQR(cap, 1);
     NVME_CAP_SET_TO(cap, 0xf);
     NVME_CAP_SET_CSS(cap, NVME_CAP_CSS_NVM);
@@ -8451,6 +8457,7 @@ static Property nvme_props[] = {
                       params.sriov_max_vq_per_vf, 0),
     DEFINE_PROP_BOOL("msix-exclusive-bar", NvmeCtrl, params.msix_exclusive_bar,
                      false),
+    DEFINE_PROP_UINT16("mqes", NvmeCtrl, params.mqes, 0x7ff),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index bed8191bd5..2e7d31c0ae 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -521,6 +521,7 @@ typedef struct NvmeParams {
     uint32_t num_queues; /* deprecated since 5.1 */
     uint32_t max_ioqpairs;
     uint16_t msix_qsize;
+    uint16_t mqes;
     uint32_t cmb_size_mb;
     uint8_t  aerl;
     uint32_t aer_max_queued;
-- 
2.34.1

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Klaus Jensen]

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

On Apr  4 13:04, John Berg wrote:
> From: John Berg <jhnberg@amazon.com>
> 
> The MQES field in the CAP register describes the Maximum Queue Entries
> Supported for the IO queues of an NVMe controller. Adding a +1 to the
> value in this field results in the total queue size. A full queue is
> when a queue of size N contains N - 1 entries, and the minimum queue
> size is 2. Thus the lowest MQES value is 1.
> 
> This patch adds the new mqes property to the NVMe emulation which allows
> a user to specify the maximum queue size by setting this property. This
> is useful as it enables testing of NVMe controller where the MQES is
> relatively small. The smallest NVMe queue size supported in NVMe is 2
> submission and completion entries, which means that the smallest legal
> mqes value is 1.
> 
> The following example shows how the mqes can be set for a the NVMe
> emulation:
> 
> -drive id=nvme0,if=none,file=nvme.img,format=raw
> -device nvme,drive=nvme0,serial=foo,mqes=1
> 
> If the mqes property is not provided then the default mqes will still be
> 0x7ff (the queue size is 2048 entries).
> 
> Signed-off-by: John Berg <jhnberg@amazon.co.uk>

LGTM,

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

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

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Klaus Jensen]

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

On Apr  4 13:04, John Berg wrote:
> From: John Berg <jhnberg@amazon.com>
> 
> The MQES field in the CAP register describes the Maximum Queue Entries
> Supported for the IO queues of an NVMe controller. Adding a +1 to the
> value in this field results in the total queue size. A full queue is
> when a queue of size N contains N - 1 entries, and the minimum queue
> size is 2. Thus the lowest MQES value is 1.
> 
> This patch adds the new mqes property to the NVMe emulation which allows
> a user to specify the maximum queue size by setting this property. This
> is useful as it enables testing of NVMe controller where the MQES is
> relatively small. The smallest NVMe queue size supported in NVMe is 2
> submission and completion entries, which means that the smallest legal
> mqes value is 1.
> 
> The following example shows how the mqes can be set for a the NVMe
> emulation:
> 
> -drive id=nvme0,if=none,file=nvme.img,format=raw
> -device nvme,drive=nvme0,serial=foo,mqes=1
> 
> If the mqes property is not provided then the default mqes will still be
> 0x7ff (the queue size is 2048 entries).
> 
> Signed-off-by: John Berg <jhnberg@amazon.co.uk>
> ---
>  hw/nvme/ctrl.c | 9 ++++++++-
>  hw/nvme/nvme.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 127c3d2383..86cda9bc73 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7805,6 +7805,12 @@ static bool nvme_check_params(NvmeCtrl *n, Error **errp)
>          return false;
>      }
>  
> +    if (params->mqes < 1)
> +    {

Please keep the `{` on the same line as the `if`. I think checkpatch.pl
should catch this.

No need to send a v2, I'll fix it up when I apply it to nvme-next :)

Thanks!

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

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Keith Busch]

On Thu, Apr 04, 2024 at 01:04:18PM +0100, John Berg wrote:
> The MQES field in the CAP register describes the Maximum Queue Entries
> Supported for the IO queues of an NVMe controller. Adding a +1 to the
> value in this field results in the total queue size. A full queue is
> when a queue of size N contains N - 1 entries, and the minimum queue
> size is 2. Thus the lowest MQES value is 1.
> 
> This patch adds the new mqes property to the NVMe emulation which allows
> a user to specify the maximum queue size by setting this property. This
> is useful as it enables testing of NVMe controller where the MQES is
> relatively small. The smallest NVMe queue size supported in NVMe is 2
> submission and completion entries, which means that the smallest legal
> mqes value is 1.
> 
> The following example shows how the mqes can be set for a the NVMe
> emulation:
> 
> -drive id=nvme0,if=none,file=nvme.img,format=raw
> -device nvme,drive=nvme0,serial=foo,mqes=1
> 
> If the mqes property is not provided then the default mqes will still be
> 0x7ff (the queue size is 2048 entries).

Looks good. I had to double check where nvme_create_sq() was getting its
limit from when processing the host command, and sure enough it's
directly from the register field.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Berg, John]

On Thu, 2024-04-04 at 15:01 +0200, Klaus Jensen wrote:
> On Apr  4 13:04, John Berg wrote:
> > From: John Berg <jhnberg@amazon.com>
> > 
> > The MQES field in the CAP register describes the Maximum Queue
> > Entries
> > Supported for the IO queues of an NVMe controller. Adding a +1 to
> > the
> > value in this field results in the total queue size. A full queue
> > is
> > when a queue of size N contains N - 1 entries, and the minimum
> > queue
> > size is 2. Thus the lowest MQES value is 1.
> > 
> > This patch adds the new mqes property to the NVMe emulation which
> > allows
> > a user to specify the maximum queue size by setting this property.
> > This
> > is useful as it enables testing of NVMe controller where the MQES
> > is
> > relatively small. The smallest NVMe queue size supported in NVMe is
> > 2
> > submission and completion entries, which means that the smallest
> > legal
> > mqes value is 1.
> > 
> > The following example shows how the mqes can be set for a the NVMe
> > emulation:
> > 
> > -drive id=nvme0,if=none,file=nvme.img,format=raw
> > -device nvme,drive=nvme0,serial=foo,mqes=1
> > 
> > If the mqes property is not provided then the default mqes will
> > still be
> > 0x7ff (the queue size is 2048 entries).
> > 
> > Signed-off-by: John Berg <jhnberg@amazon.co.uk>
> > ---
> >  hw/nvme/ctrl.c | 9 ++++++++-
> >  hw/nvme/nvme.h | 1 +
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 127c3d2383..86cda9bc73 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -7805,6 +7805,12 @@ static bool nvme_check_params(NvmeCtrl *n,
> > Error **errp)
> >          return false;
> >      }
> >  
> > +    if (params->mqes < 1)
> > +    {
> 
> Please keep the `{` on the same line as the `if`. I think
> checkpatch.pl
> should catch this.
> 
> No need to send a v2, I'll fix it up when I apply it to nvme-next :)
> 
> Thanks!


Hello

Sorry for chasing. I was just wondering when this patch will be
applied. I can send a second revision if that helps.

Kind regards,
John

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Klaus Jensen]

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

On May  1 12:27, Berg, John wrote:
> On Thu, 2024-04-04 at 15:01 +0200, Klaus Jensen wrote:
> > On Apr  4 13:04, John Berg wrote:
> > > From: John Berg <jhnberg@amazon.com>
> > > 
> > > The MQES field in the CAP register describes the Maximum Queue
> > > Entries
> > > Supported for the IO queues of an NVMe controller. Adding a +1 to
> > > the
> > > value in this field results in the total queue size. A full queue
> > > is
> > > when a queue of size N contains N - 1 entries, and the minimum
> > > queue
> > > size is 2. Thus the lowest MQES value is 1.
> > > 
> > > This patch adds the new mqes property to the NVMe emulation which
> > > allows
> > > a user to specify the maximum queue size by setting this property.
> > > This
> > > is useful as it enables testing of NVMe controller where the MQES
> > > is
> > > relatively small. The smallest NVMe queue size supported in NVMe is
> > > 2
> > > submission and completion entries, which means that the smallest
> > > legal
> > > mqes value is 1.
> > > 
> > > The following example shows how the mqes can be set for a the NVMe
> > > emulation:
> > > 
> > > -drive id=nvme0,if=none,file=nvme.img,format=raw
> > > -device nvme,drive=nvme0,serial=foo,mqes=1
> > > 
> > > If the mqes property is not provided then the default mqes will
> > > still be
> > > 0x7ff (the queue size is 2048 entries).
> > > 
> > > Signed-off-by: John Berg <jhnberg@amazon.co.uk>
> > > ---
> > >  hw/nvme/ctrl.c | 9 ++++++++-
> > >  hw/nvme/nvme.h | 1 +
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > index 127c3d2383..86cda9bc73 100644
> > > --- a/hw/nvme/ctrl.c
> > > +++ b/hw/nvme/ctrl.c
> > > @@ -7805,6 +7805,12 @@ static bool nvme_check_params(NvmeCtrl *n,
> > > Error **errp)
> > >          return false;
> > >      }
> > >  
> > > +    if (params->mqes < 1)
> > > +    {
> > 
> > Please keep the `{` on the same line as the `if`. I think
> > checkpatch.pl
> > should catch this.
> > 
> > No need to send a v2, I'll fix it up when I apply it to nvme-next :)
> > 
> > Thanks!
> 
> 
> Hello
> 
> Sorry for chasing. I was just wondering when this patch will be
> applied. I can send a second revision if that helps.
> 

No need for the sorry. My apologies. It feel off my radar, so thanks for
the bump.

I've queued this on nvme-next, will send a pull for master ASAP.

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

Re: [PATCH] hw/nvme: Add support for setting the MQES for the NVMe emulation

[Berg, John]

On Wed, 2024-05-01 at 14:35 +0200, Klaus Jensen wrote:
> On May  1 12:27, Berg, John wrote:
> > On Thu, 2024-04-04 at 15:01 +0200, Klaus Jensen wrote:
> > > On Apr  4 13:04, John Berg wrote:
> > > > From: John Berg <jhnberg@amazon.com>
> > > > 
> > > > The MQES field in the CAP register describes the Maximum Queue
> > > > Entries
> > > > Supported for the IO queues of an NVMe controller. Adding a +1
> > > > to
> > > > the
> > > > value in this field results in the total queue size. A full
> > > > queue
> > > > is
> > > > when a queue of size N contains N - 1 entries, and the minimum
> > > > queue
> > > > size is 2. Thus the lowest MQES value is 1.
> > > > 
> > > > This patch adds the new mqes property to the NVMe emulation
> > > > which
> > > > allows
> > > > a user to specify the maximum queue size by setting this
> > > > property.
> > > > This
> > > > is useful as it enables testing of NVMe controller where the
> > > > MQES
> > > > is
> > > > relatively small. The smallest NVMe queue size supported in
> > > > NVMe is
> > > > 2
> > > > submission and completion entries, which means that the
> > > > smallest
> > > > legal
> > > > mqes value is 1.
> > > > 
> > > > The following example shows how the mqes can be set for a the
> > > > NVMe
> > > > emulation:
> > > > 
> > > > -drive id=nvme0,if=none,file=nvme.img,format=raw
> > > > -device nvme,drive=nvme0,serial=foo,mqes=1
> > > > 
> > > > If the mqes property is not provided then the default mqes will
> > > > still be
> > > > 0x7ff (the queue size is 2048 entries).
> > > > 
> > > > Signed-off-by: John Berg <jhnberg@amazon.co.uk>
> > > > ---
> > > >  hw/nvme/ctrl.c | 9 ++++++++-
> > > >  hw/nvme/nvme.h | 1 +
> > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > > index 127c3d2383..86cda9bc73 100644
> > > > --- a/hw/nvme/ctrl.c
> > > > +++ b/hw/nvme/ctrl.c
> > > > @@ -7805,6 +7805,12 @@ static bool nvme_check_params(NvmeCtrl
> > > > *n,
> > > > Error **errp)
> > > >          return false;
> > > >      }
> > > >  
> > > > +    if (params->mqes < 1)
> > > > +    {
> > > 
> > > Please keep the `{` on the same line as the `if`. I think
> > > checkpatch.pl
> > > should catch this.
> > > 
> > > No need to send a v2, I'll fix it up when I apply it to nvme-next
> > > :)
> > > 
> > > Thanks!
> > 
> > 
> > Hello
> > 
> > Sorry for chasing. I was just wondering when this patch will be
> > applied. I can send a second revision if that helps.
> > 
> 
> No need for the sorry. My apologies. It feel off my radar, so thanks
> for
> the bump.
> 
> I've queued this on nvme-next, will send a pull for master ASAP.

Hello, just checking in again. Will this be merged into master soon?