[PATCH 0/3] hw/nvme: misc fixes

[PATCH 0/3] hw/nvme: misc fixes

[Klaus Jensen]

Fix some bugs in the backlog.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
Klaus Jensen (3):
      hw/nvme: reject fdp.runs smaller than the lba data size
      hw/nvme: fix fdp reclaim unit accounting
      hw/nvme: fix broadcast fdp events handling

 hw/nvme/ctrl.c | 5 ++++-
 hw/nvme/ns.c   | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)
---
base-commit: 2a2590a7efb01f1f11ab34d180273f02b94e9427
change-id: 20261112-nvme-for-rc-a81f0c2dcf4c

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

[PATCH 1/3] hw/nvme: reject fdp.runs smaller than the lba data size

[Klaus Jensen]

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

The reclaim unit accounting in nvme_do_write_fdp spins forever if RUNS
is configured to be smaller than LBADS.

Reject such an invalid configuration upfront.

Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ns.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 2ae26a9acbd0..37d45bb4d533 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -485,6 +485,12 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp)
     char *r, *p;
     uint16_t *ph;
 
+    if (endgrp->fdp.runs < nvme_l2b(ns, 1)) {
+        error_setg(errp, "fdp.runs must be greater than or equal to the "
+                   "lba data size of the namespace");
+        return false;
+    }
+
     if (!ns->params.fdp.ruhs) {
         ns->fdp.nphs = 1;
         ph = ns->fdp.phs = g_new(uint16_t, 1);

-- 
2.53.0

Re: [PATCH 1/3] hw/nvme: reject fdp.runs smaller than the lba data size

[Tommaso Califano]

On Thu Nov 12, 2026 at 10:53 AM CET, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The reclaim unit accounting in nvme_do_write_fdp spins forever if RUNS
> is configured to be smaller than LBADS.
>
> Reject such an invalid configuration upfront.
>
> Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520

/s/Link/Resolves

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

Maybe Fixes tag and QEMU stable in cc?

> ---
>  hw/nvme/ns.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> index 2ae26a9acbd0..37d45bb4d533 100644
> --- a/hw/nvme/ns.c
> +++ b/hw/nvme/ns.c
> @@ -485,6 +485,12 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp)
>      char *r, *p;
>      uint16_t *ph;
>  
> +    if (endgrp->fdp.runs < nvme_l2b(ns, 1)) {
> +        error_setg(errp, "fdp.runs must be greater than or equal to the "
> +                   "lba data size of the namespace");
> +        return false;
> +    }
> +
>      if (!ns->params.fdp.ruhs) {
>          ns->fdp.nphs = 1;
>          ph = ns->fdp.phs = g_new(uint16_t, 1);

Thanks,
Tommaso

Re: [PATCH 1/3] hw/nvme: reject fdp.runs smaller than the lba data size

[Klaus Jensen]

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

On Nov 12 14:13, Tommaso Califano wrote:
> On Thu Nov 12, 2026 at 10:53 AM CET, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The reclaim unit accounting in nvme_do_write_fdp spins forever if RUNS
> > is configured to be smaller than LBADS.
> >
> > Reject such an invalid configuration upfront.
> >
> > Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
> 
> /s/Link/Resolves
> 

Issue #4520 is a bulk report that unfortunately contains more than just
one issue. I didn't want automation to close the issue. That's why I
used Link instead of Resolves.

> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> Maybe Fixes tag and QEMU stable in cc?

Doh. Yes.

Thanks!

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

Re: [PATCH 1/3] hw/nvme: reject fdp.runs smaller than the lba data size

[Tommaso Califano]

On Thu Nov 12, 2026 at 7:04 PM CET, Klaus Jensen wrote:
> On Nov 12 14:13, Tommaso Califano wrote:
>> On Thu Nov 12, 2026 at 10:53 AM CET, Klaus Jensen wrote:
>> > From: Klaus Jensen <k.jensen@samsung.com>
>> >
>> > The reclaim unit accounting in nvme_do_write_fdp spins forever if RUNS
>> > is configured to be smaller than LBADS.
>> >
>> > Reject such an invalid configuration upfront.
>> >
>> > Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
>> 
>> /s/Link/Resolves
>> 
>
> Issue #4520 is a bulk report that unfortunately contains more than just
> one issue. I didn't want automation to close the issue. That's why I
> used Link instead of Resolves.
>

Makes sense!

>> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> 
>> Maybe Fixes tag and QEMU stable in cc?
>
> Doh. Yes.
>
> Thanks!

Regards,
Tommaso

Re: [PATCH 1/3] hw/nvme: reject fdp.runs smaller than the lba data size

[Jesper Wendel Devantier]

On Thu, 12 Nov 2026 10:53:28 +0100, Klaus Jensen <its@irrelevant.dk> wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The reclaim unit accounting in nvme_do_write_fdp spins forever if RUNS
> is configured to be smaller than LBADS.
>
> Reject such an invalid configuration upfront.
>
> Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ns.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> index 2ae26a9acbd0..37d45bb4d533 100644
> --- a/hw/nvme/ns.c
> +++ b/hw/nvme/ns.c
> @@ -485,6 +485,12 @@ static bool nvme_ns_init_fdp(NvmeNamespace *ns, Error **errp)
>      char *r, *p;
>      uint16_t *ph;
>  
> +    if (endgrp->fdp.runs < nvme_l2b(ns, 1)) {
> +        error_setg(errp, "fdp.runs must be greater than or equal to the "
> +                   "lba data size of the namespace");
> +        return false;
> +    }
> +
>      if (!ns->params.fdp.ruhs) {
>          ns->fdp.nphs = 1;
>          ph = ns->fdp.phs = g_new(uint16_t, 1);
>
> -- 
> 2.53.0
>
>
>

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

[PATCH 2/3] hw/nvme: fix fdp reclaim unit accounting

[Klaus Jensen]

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

nvme_do_write_fdp falls back to placement handle zero for a write that
has no Data Placement directive or where the placement identifier is
invalid. However, it then passes the invalid placement identifier to
nvme_update_ruh which results in no update to RUAMW for placement handle
zero.

Reset the placement identifier along with the handle in the fallback.

Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 685cbc2f340c..a0d22dacedfa 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -3754,6 +3754,7 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba,
         !nvme_parse_pid(ns, pid, &ph, &rg)) {
         ph = 0;
         rg = 0;
+        pid = 0;
     }
 
     ruhid = ns->fdp.phs[ph];

-- 
2.53.0

Re: [PATCH 2/3] hw/nvme: fix fdp reclaim unit accounting

[Tommaso Califano]

On Thu Nov 12, 2026 at 10:53 AM CET, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> nvme_do_write_fdp falls back to placement handle zero for a write that
> has no Data Placement directive or where the placement identifier is
> invalid. However, it then passes the invalid placement identifier to
> nvme_update_ruh which results in no update to RUAMW for placement handle
> zero.
>
> Reset the placement identifier along with the handle in the fallback.
>
> Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520

/s/Link/Resolves

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

Fixes tag and QEMU stable

> ---
>  hw/nvme/ctrl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 685cbc2f340c..a0d22dacedfa 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -3754,6 +3754,7 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba,
>          !nvme_parse_pid(ns, pid, &ph, &rg)) {
>          ph = 0;
>          rg = 0;
> +        pid = 0;
>      }
>  
>      ruhid = ns->fdp.phs[ph];

Thanks,
Tommaso

Re: [PATCH 2/3] hw/nvme: fix fdp reclaim unit accounting

[Jesper Wendel Devantier]

On Thu, 12 Nov 2026 10:53:29 +0100, Klaus Jensen <its@irrelevant.dk> wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> nvme_do_write_fdp falls back to placement handle zero for a write that
> has no Data Placement directive or where the placement identifier is
> invalid. However, it then passes the invalid placement identifier to
> nvme_update_ruh which results in no update to RUAMW for placement handle
> zero.
>
> Reset the placement identifier along with the handle in the fallback.
>
> Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 685cbc2f340c..a0d22dacedfa 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -3754,6 +3754,7 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba,
>          !nvme_parse_pid(ns, pid, &ph, &rg)) {
>          ph = 0;
>          rg = 0;
> +        pid = 0;
>      }
>  
>      ruhid = ns->fdp.phs[ph];
>
> -- 
> 2.53.0
>
>
>

Correct.
Had to re-familiarize myself, but pid (placement identifier) is essentially
a 2-tuple of (rg, ph)  (reclaim group, placement handle).

NVMe Base Specification 2.4, §8.1.12.1 states that if the data placement directive
is unset/invalid that the write must then use the placement handle 0, but that
the reclaim group may be defined by the controller.

Setting `pid=0` is correct, but only so long as we also set rg to 0.
Perhaps a clearer way to specify this would be:

```
pid = 0;
nvme_parse_pid(ns, pid, &ph, &rg);
```

.. it more clearly captures the relationship between PID and RG/PH.
But the change is correct.

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

Re: [PATCH 2/3] hw/nvme: fix fdp reclaim unit accounting

[Klaus Jensen]

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

On Nov 12 17:08, Jesper Wendel Devantier wrote:
> 
> 
> On Thu, 12 Nov 2026 10:53:29 +0100, Klaus Jensen <its@irrelevant.dk> wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > nvme_do_write_fdp falls back to placement handle zero for a write that
> > has no Data Placement directive or where the placement identifier is
> > invalid. However, it then passes the invalid placement identifier to
> > nvme_update_ruh which results in no update to RUAMW for placement handle
> > zero.
> >
> > Reset the placement identifier along with the handle in the fallback.
> >
> > Link: https://gitlab.com/qemu-project/qemu/-/work_items/4520
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 685cbc2f340c..a0d22dacedfa 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -3754,6 +3754,7 @@ static void nvme_do_write_fdp(NvmeCtrl *n, NvmeRequest *req, uint64_t slba,
> >          !nvme_parse_pid(ns, pid, &ph, &rg)) {
> >          ph = 0;
> >          rg = 0;
> > +        pid = 0;
> >      }
> >  
> >      ruhid = ns->fdp.phs[ph];
> >
> > -- 
> > 2.53.0
> >
> >
> >
> 
> Correct.
> Had to re-familiarize myself, but pid (placement identifier) is essentially
> a 2-tuple of (rg, ph)  (reclaim group, placement handle).
> 
> NVMe Base Specification 2.4, §8.1.12.1 states that if the data placement directive
> is unset/invalid that the write must then use the placement handle 0, but that
> the reclaim group may be defined by the controller.
> 
> Setting `pid=0` is correct, but only so long as we also set rg to 0.
> Perhaps a clearer way to specify this would be:
> 
> ```
> pid = 0;
> nvme_parse_pid(ns, pid, &ph, &rg);
> ```
> 
> .. it more clearly captures the relationship between PID and RG/PH.
> But the change is correct.
> 

I like that, thanks!

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

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

[PATCH 3/3] hw/nvme: fix broadcast fdp events handling

[Klaus Jensen]

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

The Set Features handler for the FDP Events feature asserts that a
namespace is passed in the command because the feature is
namespace-specific. An invalid namespace is rejected prior to reaching
the handler, but the "broadcast nsid" (0xFFFFFFFF) is considered valid
and causes the assert to abort QEMU.

Reject it with Invalid Field in Command instead as per the
specification.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4792
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a0d22dacedfa..71c24dc02b67 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6636,7 +6636,9 @@ static uint16_t nvme_set_feature_fdp_events(NvmeCtrl *n, NvmeNamespace *ns,
     g_autofree uint8_t *events = NULL;
     NvmeRuHandle *ruh = NULL;
 
-    assert(ns);
+    if (!ns) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
 
     if (!n->subsys || !n->subsys->endgrp.fdp.enabled) {
         return NVME_FDP_DISABLED | NVME_DNR;

-- 
2.53.0

Re: [PATCH 3/3] hw/nvme: fix broadcast fdp events handling

[Tommaso Califano]

On Thu Nov 12, 2026 at 10:53 AM CET, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The Set Features handler for the FDP Events feature asserts that a
> namespace is passed in the command because the feature is
> namespace-specific. An invalid namespace is rejected prior to reaching
> the handler, but the "broadcast nsid" (0xFFFFFFFF) is considered valid
> and causes the assert to abort QEMU.
>
> Reject it with Invalid Field in Command instead as per the
> specification.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4792
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Fixes tag and QEMU stable in cc.

> ---
>  hw/nvme/ctrl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index a0d22dacedfa..71c24dc02b67 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6636,7 +6636,9 @@ static uint16_t nvme_set_feature_fdp_events(NvmeCtrl *n, NvmeNamespace *ns,
>      g_autofree uint8_t *events = NULL;
>      NvmeRuHandle *ruh = NULL;
>  
> -    assert(ns);
> +    if (!ns) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
>  
>      if (!n->subsys || !n->subsys->endgrp.fdp.enabled) {
>          return NVME_FDP_DISABLED | NVME_DNR;

Thanks,
Tommaso

Re: [PATCH 3/3] hw/nvme: fix broadcast fdp events handling

[Jesper Wendel Devantier]

On Thu, 12 Nov 2026 10:53:30 +0100, Klaus Jensen <its@irrelevant.dk> wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The Set Features handler for the FDP Events feature asserts that a
> namespace is passed in the command because the feature is
> namespace-specific. An invalid namespace is rejected prior to reaching
> the handler, but the "broadcast nsid" (0xFFFFFFFF) is considered valid
> and causes the assert to abort QEMU.
>
> Reject it with Invalid Field in Command instead as per the
> specification.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4792
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index a0d22dacedfa..71c24dc02b67 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6636,7 +6636,9 @@ static uint16_t nvme_set_feature_fdp_events(NvmeCtrl *n, NvmeNamespace *ns,
>      g_autofree uint8_t *events = NULL;
>      NvmeRuHandle *ruh = NULL;
>  
> -    assert(ns);
> +    if (!ns) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
>  
>      if (!n->subsys || !n->subsys->endgrp.fdp.enabled) {
>          return NVME_FDP_DISABLED | NVME_DNR;
>
> -- 
> 2.53.0
>
>
>

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