hw/nvme/ctrl.c | 9 ++++++++- hw/nvme/ns.c | 2 ++ hw/nvme/nvme.h | 1 + 3 files changed, 11 insertions(+), 1 deletion(-)
From: Niklas Cassel <niklas.cassel@wdc.com>
In the Zoned Namespace Command Set Specification, chapter
2.5.1 Managing resources
"The controller may transition zones in the ZSIO:Implicitly Opened state
to the ZSC:Closed state for resource management purposes."
The word may in this sentence means that automatically transitioning
an implicitly opened zone to closed is completely optional.
Add a new parameter so that the user can control if this automatic
transitioning should be performed or not.
Being able to control this can help with verifying that e.g. a user-space
program behaves properly even without this optional ZNS feature.
The default value is set to true, in order to not change the existing
behavior.
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
hw/nvme/ctrl.c | 9 ++++++++-
hw/nvme/ns.c | 2 ++
hw/nvme/nvme.h | 1 +
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 40a7efcea9..d00f0297a5 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -141,6 +141,11 @@
*
* zoned.cross_read=<enable RAZB, default: false>
* Setting this property to true enables Read Across Zone Boundaries.
+ *
+ * zoned.auto_transition=<enable auto resource management, default: true>
+ * Indicates if zones in zone state implicitly opened can be
+ * automatically transitioned to zone state closed for resource
+ * management purposes.
*/
#include "qemu/osdep.h"
@@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
/* fallthrough */
case NVME_ZONE_STATE_CLOSED:
- nvme_zrm_auto_transition_zone(ns);
+ if (ns->params.auto_transition_zones) {
+ nvme_zrm_auto_transition_zone(ns);
+ }
status = nvme_aor_check(ns, act, 1);
if (status) {
return status;
diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
index 3fec9c6273..31dee43d30 100644
--- a/hw/nvme/ns.c
+++ b/hw/nvme/ns.c
@@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
params.max_open_zones, 0),
DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
params.zd_extension_size, 0),
+ DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
+ params.auto_transition_zones, true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda14..bd86054db2 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
uint32_t max_active_zones;
uint32_t max_open_zones;
uint32_t zd_extension_size;
+ bool auto_transition_zones;
} NvmeNamespaceParams;
typedef struct NvmeNamespace {
--
2.31.1
On May 28 11:05, Niklas Cassel wrote:
>From: Niklas Cassel <niklas.cassel@wdc.com>
>
>In the Zoned Namespace Command Set Specification, chapter
>2.5.1 Managing resources
>
>"The controller may transition zones in the ZSIO:Implicitly Opened state
>to the ZSC:Closed state for resource management purposes."
>
>The word may in this sentence means that automatically transitioning
>an implicitly opened zone to closed is completely optional.
>
>Add a new parameter so that the user can control if this automatic
>transitioning should be performed or not.
>
>Being able to control this can help with verifying that e.g. a user-space
>program behaves properly even without this optional ZNS feature.
>
>The default value is set to true, in order to not change the existing
>behavior.
>
>Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>---
> hw/nvme/ctrl.c | 9 ++++++++-
> hw/nvme/ns.c | 2 ++
> hw/nvme/nvme.h | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 40a7efcea9..d00f0297a5 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -141,6 +141,11 @@
> *
> * zoned.cross_read=<enable RAZB, default: false>
> * Setting this property to true enables Read Across Zone Boundaries.
>+ *
>+ * zoned.auto_transition=<enable auto resource management, default: true>
>+ * Indicates if zones in zone state implicitly opened can be
>+ * automatically transitioned to zone state closed for resource
>+ * management purposes.
> */
>
> #include "qemu/osdep.h"
>@@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> /* fallthrough */
>
> case NVME_ZONE_STATE_CLOSED:
>- nvme_zrm_auto_transition_zone(ns);
>+ if (ns->params.auto_transition_zones) {
>+ nvme_zrm_auto_transition_zone(ns);
>+ }
> status = nvme_aor_check(ns, act, 1);
> if (status) {
> return status;
>diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>index 3fec9c6273..31dee43d30 100644
>--- a/hw/nvme/ns.c
>+++ b/hw/nvme/ns.c
>@@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
> params.max_open_zones, 0),
> DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
> params.zd_extension_size, 0),
>+ DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>+ params.auto_transition_zones, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
>diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>index 81a35cda14..bd86054db2 100644
>--- a/hw/nvme/nvme.h
>+++ b/hw/nvme/nvme.h
>@@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> uint32_t max_active_zones;
> uint32_t max_open_zones;
> uint32_t zd_extension_size;
>+ bool auto_transition_zones;
> } NvmeNamespaceParams;
>
> typedef struct NvmeNamespace {
>--
>2.31.1
>
Looks good Niklas!
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
> On May 28 11:05, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > In the Zoned Namespace Command Set Specification, chapter
> > 2.5.1 Managing resources
> >
> > "The controller may transition zones in the ZSIO:Implicitly Opened state
> > to the ZSC:Closed state for resource management purposes."
> >
> > The word may in this sentence means that automatically transitioning
> > an implicitly opened zone to closed is completely optional.
> >
> > Add a new parameter so that the user can control if this automatic
> > transitioning should be performed or not.
> >
> > Being able to control this can help with verifying that e.g. a user-space
> > program behaves properly even without this optional ZNS feature.
> >
> > The default value is set to true, in order to not change the existing
> > behavior.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> > hw/nvme/ctrl.c | 9 ++++++++-
> > hw/nvme/ns.c | 2 ++
> > hw/nvme/nvme.h | 1 +
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 40a7efcea9..d00f0297a5 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -141,6 +141,11 @@
> > *
> > * zoned.cross_read=<enable RAZB, default: false>
> > * Setting this property to true enables Read Across Zone Boundaries.
> > + *
> > + * zoned.auto_transition=<enable auto resource management, default: true>
> > + * Indicates if zones in zone state implicitly opened can be
> > + * automatically transitioned to zone state closed for resource
> > + * management purposes.
> > */
> >
> > #include "qemu/osdep.h"
> > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > /* fallthrough */
> >
> > case NVME_ZONE_STATE_CLOSED:
> > - nvme_zrm_auto_transition_zone(ns);
> > + if (ns->params.auto_transition_zones) {
> > + nvme_zrm_auto_transition_zone(ns);
> > + }
> > status = nvme_aor_check(ns, act, 1);
> > if (status) {
> > return status;
> > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> > index 3fec9c6273..31dee43d30 100644
> > --- a/hw/nvme/ns.c
> > +++ b/hw/nvme/ns.c
> > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
> > params.max_open_zones, 0),
> > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
> > params.zd_extension_size, 0),
> > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
> > + params.auto_transition_zones, true),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> > index 81a35cda14..bd86054db2 100644
> > --- a/hw/nvme/nvme.h
> > +++ b/hw/nvme/nvme.h
> > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> > uint32_t max_active_zones;
> > uint32_t max_open_zones;
> > uint32_t zd_extension_size;
> > + bool auto_transition_zones;
> > } NvmeNamespaceParams;
> >
> > typedef struct NvmeNamespace {
> > --
> > 2.31.1
> >
>
> Looks good Niklas!
>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
In reality, it is the controller that does the auto transitioning.
In theory, one namespace could be attached to two different controllers,
and I guess, in that case, it depends on if the controller that we used
when doing the write supports auto transitioning or not, that determines
if a zone will be auto transitioned or not.
If we were to change this to be a parameter of the controller instead
of a parameter of the namespace, we would require to refactor a lot of
code in the regular write path. As we currently don't have any NvmeRequest
object in nvme_zrm_open_flags().
Thoughts?
Kind regards,
Niklas
On May 31 15:42, Niklas Cassel wrote:
>On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
>> On May 28 11:05, Niklas Cassel wrote:
>> > From: Niklas Cassel <niklas.cassel@wdc.com>
>> >
>> > In the Zoned Namespace Command Set Specification, chapter
>> > 2.5.1 Managing resources
>> >
>> > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > to the ZSC:Closed state for resource management purposes."
>> >
>> > The word may in this sentence means that automatically transitioning
>> > an implicitly opened zone to closed is completely optional.
>> >
>> > Add a new parameter so that the user can control if this automatic
>> > transitioning should be performed or not.
>> >
>> > Being able to control this can help with verifying that e.g. a user-space
>> > program behaves properly even without this optional ZNS feature.
>> >
>> > The default value is set to true, in order to not change the existing
>> > behavior.
>> >
>> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > ---
>> > hw/nvme/ctrl.c | 9 ++++++++-
>> > hw/nvme/ns.c | 2 ++
>> > hw/nvme/nvme.h | 1 +
>> > 3 files changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > index 40a7efcea9..d00f0297a5 100644
>> > --- a/hw/nvme/ctrl.c
>> > +++ b/hw/nvme/ctrl.c
>> > @@ -141,6 +141,11 @@
>> > *
>> > * zoned.cross_read=<enable RAZB, default: false>
>> > * Setting this property to true enables Read Across Zone Boundaries.
>> > + *
>> > + * zoned.auto_transition=<enable auto resource management, default: true>
>> > + * Indicates if zones in zone state implicitly opened can be
>> > + * automatically transitioned to zone state closed for resource
>> > + * management purposes.
>> > */
>> >
>> > #include "qemu/osdep.h"
>> > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > /* fallthrough */
>> >
>> > case NVME_ZONE_STATE_CLOSED:
>> > - nvme_zrm_auto_transition_zone(ns);
>> > + if (ns->params.auto_transition_zones) {
>> > + nvme_zrm_auto_transition_zone(ns);
>> > + }
>> > status = nvme_aor_check(ns, act, 1);
>> > if (status) {
>> > return status;
>> > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>> > index 3fec9c6273..31dee43d30 100644
>> > --- a/hw/nvme/ns.c
>> > +++ b/hw/nvme/ns.c
>> > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>> > params.max_open_zones, 0),
>> > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>> > params.zd_extension_size, 0),
>> > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>> > + params.auto_transition_zones, true),
>> > DEFINE_PROP_END_OF_LIST(),
>> > };
>> >
>> > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > index 81a35cda14..bd86054db2 100644
>> > --- a/hw/nvme/nvme.h
>> > +++ b/hw/nvme/nvme.h
>> > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
>> > uint32_t max_active_zones;
>> > uint32_t max_open_zones;
>> > uint32_t zd_extension_size;
>> > + bool auto_transition_zones;
>> > } NvmeNamespaceParams;
>> >
>> > typedef struct NvmeNamespace {
>> > --
>> > 2.31.1
>> >
>>
>> Looks good Niklas!
>>
>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>
>In reality, it is the controller that does the auto transitioning.
>
>In theory, one namespace could be attached to two different controllers,
>and I guess, in that case, it depends on if the controller that we used
>when doing the write supports auto transitioning or not, that determines
>if a zone will be auto transitioned or not.
>
>If we were to change this to be a parameter of the controller instead
>of a parameter of the namespace, we would require to refactor a lot of
>code in the regular write path. As we currently don't have any NvmeRequest
>object in nvme_zrm_open_flags().
>
>Thoughts?
>
I think you are right. This should be controller-specific behavior. I
took the liberty of moving the parameter; the refactor is minimal.
From: Niklas Cassel <niklas.cassel@wdc.com>
In the Zoned Namespace Command Set Specification, chapter
2.5.1 Managing resources
"The controller may transition zones in the ZSIO:Implicitly Opened state
to the ZSC:Closed state for resource management purposes."
The word may in this sentence means that automatically transitioning
an implicitly opened zone to closed is completely optional.
Add a new parameter so that the user can control if this automatic
transitioning should be performed or not.
Being able to control this can help with verifying that e.g. a user-space
program behaves properly even without this optional ZNS feature.
The default value is set to true, in order to not change the existing
behavior.
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
[k.jensen: moved parameter to controller]
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/nvme/nvme.h | 1 +
hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
2 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 81a35cda142b..93a7e0e5380e 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -382,6 +382,7 @@ typedef struct NvmeParams {
uint8_t vsl;
bool use_intel_id;
uint8_t zasl;
+ bool auto_transition_zones;
bool legacy_cmb;
} NvmeParams;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 40a7efcea914..8dd9cb2ccbf3 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -34,6 +34,7 @@
* aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
* mdts=<N[optional]>,vsl=<N[optional]>, \
* zoned.zasl=<N[optional]>, \
+ * zoned.auto_transition=<on|off[optional]>, \
* subsys=<subsys_id>
* -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
* zoned=<true|false[optional]>, \
@@ -100,6 +101,11 @@
* the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
* defaulting to the value of `mdts`).
*
+ * - `zoned.auto_transition`
+ * Indicates if zones in zone state implicitly opened can be automatically
+ * transitioned to zone state closed for resource management purposes.
+ * Defaults to 'on'.
+ *
* nvme namespace device parameters
* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* - `shared`
@@ -1686,8 +1692,8 @@ enum {
NVME_ZRM_AUTO = 1 << 0,
};
-static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
- int flags)
+static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
+ NvmeZone *zone, int flags)
{
int act = 0;
uint16_t status;
@@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
/* fallthrough */
case NVME_ZONE_STATE_CLOSED:
- nvme_zrm_auto_transition_zone(ns);
+ if (n->params.auto_transition_zones) {
+ nvme_zrm_auto_transition_zone(ns);
+ }
status = nvme_aor_check(ns, act, 1);
if (status) {
return status;
@@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
}
}
-static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
+static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
+ NvmeZone *zone)
{
- return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
+ return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
}
-static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
+static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
+ NvmeZone *zone)
{
- return nvme_zrm_open_flags(ns, zone, 0);
+ return nvme_zrm_open_flags(n, ns, zone, 0);
}
static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
@@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
goto invalid;
}
- status = nvme_zrm_auto(ns, zone);
+ status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
if (status) {
goto invalid;
}
@@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
goto invalid;
}
- status = nvme_zrm_auto(ns, zone);
+ status = nvme_zrm_auto(n, ns, zone);
if (status) {
goto invalid;
}
@@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
NvmeZoneState state, NvmeRequest *req)
{
- return nvme_zrm_open(ns, zone);
+ return nvme_zrm_open(nvme_ctrl(req), ns, zone);
}
static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
@@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
+ DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
+ params.auto_transition_zones, true),
DEFINE_PROP_END_OF_LIST(),
};
--
2.31.1
On Mon, May 31, 2021 at 09:39:20PM +0200, Klaus Jensen wrote:
> On May 31 15:42, Niklas Cassel wrote:
> > On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
> > > On May 28 11:05, Niklas Cassel wrote:
> > > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > >
> > > > In the Zoned Namespace Command Set Specification, chapter
> > > > 2.5.1 Managing resources
> > > >
> > > > "The controller may transition zones in the ZSIO:Implicitly Opened state
> > > > to the ZSC:Closed state for resource management purposes."
> > > >
> > > > The word may in this sentence means that automatically transitioning
> > > > an implicitly opened zone to closed is completely optional.
> > > >
> > > > Add a new parameter so that the user can control if this automatic
> > > > transitioning should be performed or not.
> > > >
> > > > Being able to control this can help with verifying that e.g. a user-space
> > > > program behaves properly even without this optional ZNS feature.
> > > >
> > > > The default value is set to true, in order to not change the existing
> > > > behavior.
> > > >
> > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > > ---
> > > > hw/nvme/ctrl.c | 9 ++++++++-
> > > > hw/nvme/ns.c | 2 ++
> > > > hw/nvme/nvme.h | 1 +
> > > > 3 files changed, 11 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > > index 40a7efcea9..d00f0297a5 100644
> > > > --- a/hw/nvme/ctrl.c
> > > > +++ b/hw/nvme/ctrl.c
> > > > @@ -141,6 +141,11 @@
> > > > *
> > > > * zoned.cross_read=<enable RAZB, default: false>
> > > > * Setting this property to true enables Read Across Zone Boundaries.
> > > > + *
> > > > + * zoned.auto_transition=<enable auto resource management, default: true>
> > > > + * Indicates if zones in zone state implicitly opened can be
> > > > + * automatically transitioned to zone state closed for resource
> > > > + * management purposes.
> > > > */
> > > >
> > > > #include "qemu/osdep.h"
> > > > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > > > /* fallthrough */
> > > >
> > > > case NVME_ZONE_STATE_CLOSED:
> > > > - nvme_zrm_auto_transition_zone(ns);
> > > > + if (ns->params.auto_transition_zones) {
> > > > + nvme_zrm_auto_transition_zone(ns);
> > > > + }
> > > > status = nvme_aor_check(ns, act, 1);
> > > > if (status) {
> > > > return status;
> > > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> > > > index 3fec9c6273..31dee43d30 100644
> > > > --- a/hw/nvme/ns.c
> > > > +++ b/hw/nvme/ns.c
> > > > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
> > > > params.max_open_zones, 0),
> > > > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
> > > > params.zd_extension_size, 0),
> > > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
> > > > + params.auto_transition_zones, true),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > >
> > > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> > > > index 81a35cda14..bd86054db2 100644
> > > > --- a/hw/nvme/nvme.h
> > > > +++ b/hw/nvme/nvme.h
> > > > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> > > > uint32_t max_active_zones;
> > > > uint32_t max_open_zones;
> > > > uint32_t zd_extension_size;
> > > > + bool auto_transition_zones;
> > > > } NvmeNamespaceParams;
> > > >
> > > > typedef struct NvmeNamespace {
> > > > --
> > > > 2.31.1
> > > >
> > >
> > > Looks good Niklas!
> > >
> > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> >
> > In reality, it is the controller that does the auto transitioning.
> >
> > In theory, one namespace could be attached to two different controllers,
> > and I guess, in that case, it depends on if the controller that we used
> > when doing the write supports auto transitioning or not, that determines
> > if a zone will be auto transitioned or not.
> >
> > If we were to change this to be a parameter of the controller instead
> > of a parameter of the namespace, we would require to refactor a lot of
> > code in the regular write path. As we currently don't have any NvmeRequest
> > object in nvme_zrm_open_flags().
> >
> > Thoughts?
> >
>
> I think you are right. This should be controller-specific behavior. I took
> the liberty of moving the parameter; the refactor is minimal.
>
>
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> In the Zoned Namespace Command Set Specification, chapter
> 2.5.1 Managing resources
>
> "The controller may transition zones in the ZSIO:Implicitly Opened state
> to the ZSC:Closed state for resource management purposes."
>
> The word may in this sentence means that automatically transitioning
> an implicitly opened zone to closed is completely optional.
>
> Add a new parameter so that the user can control if this automatic
> transitioning should be performed or not.
>
> Being able to control this can help with verifying that e.g. a user-space
> program behaves properly even without this optional ZNS feature.
>
> The default value is set to true, in order to not change the existing
> behavior.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> [k.jensen: moved parameter to controller]
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> hw/nvme/nvme.h | 1 +
> hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
> 2 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 81a35cda142b..93a7e0e5380e 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -382,6 +382,7 @@ typedef struct NvmeParams {
> uint8_t vsl;
> bool use_intel_id;
> uint8_t zasl;
> + bool auto_transition_zones;
> bool legacy_cmb;
> } NvmeParams;
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 40a7efcea914..8dd9cb2ccbf3 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -34,6 +34,7 @@
> * aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
> * mdts=<N[optional]>,vsl=<N[optional]>, \
> * zoned.zasl=<N[optional]>, \
> + * zoned.auto_transition=<on|off[optional]>, \
> * subsys=<subsys_id>
> * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
> * zoned=<true|false[optional]>, \
> @@ -100,6 +101,11 @@
> * the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
> * defaulting to the value of `mdts`).
> *
> + * - `zoned.auto_transition`
> + * Indicates if zones in zone state implicitly opened can be automatically
> + * transitioned to zone state closed for resource management purposes.
> + * Defaults to 'on'.
> + *
> * nvme namespace device parameters
> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> * - `shared`
> @@ -1686,8 +1692,8 @@ enum {
> NVME_ZRM_AUTO = 1 << 0,
> };
> -static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> - int flags)
> +static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
> + NvmeZone *zone, int flags)
> {
> int act = 0;
> uint16_t status;
> @@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> /* fallthrough */
> case NVME_ZONE_STATE_CLOSED:
> - nvme_zrm_auto_transition_zone(ns);
> + if (n->params.auto_transition_zones) {
> + nvme_zrm_auto_transition_zone(ns);
> + }
> status = nvme_aor_check(ns, act, 1);
> if (status) {
> return status;
> @@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> }
> }
> -static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
> +static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
> + NvmeZone *zone)
> {
> - return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
> + return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
> }
> -static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
> +static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
> + NvmeZone *zone)
> {
> - return nvme_zrm_open_flags(ns, zone, 0);
> + return nvme_zrm_open_flags(n, ns, zone, 0);
> }
> static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
> @@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
> goto invalid;
> }
> - status = nvme_zrm_auto(ns, zone);
> + status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
> if (status) {
> goto invalid;
> }
> @@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
> goto invalid;
> }
> - status = nvme_zrm_auto(ns, zone);
> + status = nvme_zrm_auto(n, ns, zone);
> if (status) {
> goto invalid;
> }
> @@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
> static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
> NvmeZoneState state, NvmeRequest *req)
> {
> - return nvme_zrm_open(ns, zone);
> + return nvme_zrm_open(nvme_ctrl(req), ns, zone);
> }
> static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
> @@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
> DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
> DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
> DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
> + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
> + params.auto_transition_zones, true),
> DEFINE_PROP_END_OF_LIST(),
> };
> --
> 2.31.1
>
Thanks a lot Klaus! I really appreciate it.
My initial thought was to add a new flag in the enum where NVME_ZRM_AUTO is.
But I think that would just make the code harder to read.
You simply check the parameter directly, which is more obvious to the reader,
so I think patch looks good!
Kind regards,
Niklas
On Jun 1 07:30, Niklas Cassel wrote:
>On Mon, May 31, 2021 at 09:39:20PM +0200, Klaus Jensen wrote:
>> On May 31 15:42, Niklas Cassel wrote:
>> > On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
>> > > On May 28 11:05, Niklas Cassel wrote:
>> > > > From: Niklas Cassel <niklas.cassel@wdc.com>
>> > > >
>> > > > In the Zoned Namespace Command Set Specification, chapter
>> > > > 2.5.1 Managing resources
>> > > >
>> > > > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > > > to the ZSC:Closed state for resource management purposes."
>> > > >
>> > > > The word may in this sentence means that automatically transitioning
>> > > > an implicitly opened zone to closed is completely optional.
>> > > >
>> > > > Add a new parameter so that the user can control if this automatic
>> > > > transitioning should be performed or not.
>> > > >
>> > > > Being able to control this can help with verifying that e.g. a user-space
>> > > > program behaves properly even without this optional ZNS feature.
>> > > >
>> > > > The default value is set to true, in order to not change the existing
>> > > > behavior.
>> > > >
>> > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > > > ---
>> > > > hw/nvme/ctrl.c | 9 ++++++++-
>> > > > hw/nvme/ns.c | 2 ++
>> > > > hw/nvme/nvme.h | 1 +
>> > > > 3 files changed, 11 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > > index 40a7efcea9..d00f0297a5 100644
>> > > > --- a/hw/nvme/ctrl.c
>> > > > +++ b/hw/nvme/ctrl.c
>> > > > @@ -141,6 +141,11 @@
>> > > > *
>> > > > * zoned.cross_read=<enable RAZB, default: false>
>> > > > * Setting this property to true enables Read Across Zone Boundaries.
>> > > > + *
>> > > > + * zoned.auto_transition=<enable auto resource management, default: true>
>> > > > + * Indicates if zones in zone state implicitly opened can be
>> > > > + * automatically transitioned to zone state closed for resource
>> > > > + * management purposes.
>> > > > */
>> > > >
>> > > > #include "qemu/osdep.h"
>> > > > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > > /* fallthrough */
>> > > >
>> > > > case NVME_ZONE_STATE_CLOSED:
>> > > > - nvme_zrm_auto_transition_zone(ns);
>> > > > + if (ns->params.auto_transition_zones) {
>> > > > + nvme_zrm_auto_transition_zone(ns);
>> > > > + }
>> > > > status = nvme_aor_check(ns, act, 1);
>> > > > if (status) {
>> > > > return status;
>> > > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>> > > > index 3fec9c6273..31dee43d30 100644
>> > > > --- a/hw/nvme/ns.c
>> > > > +++ b/hw/nvme/ns.c
>> > > > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>> > > > params.max_open_zones, 0),
>> > > > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>> > > > params.zd_extension_size, 0),
>> > > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>> > > > + params.auto_transition_zones, true),
>> > > > DEFINE_PROP_END_OF_LIST(),
>> > > > };
>> > > >
>> > > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > > > index 81a35cda14..bd86054db2 100644
>> > > > --- a/hw/nvme/nvme.h
>> > > > +++ b/hw/nvme/nvme.h
>> > > > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
>> > > > uint32_t max_active_zones;
>> > > > uint32_t max_open_zones;
>> > > > uint32_t zd_extension_size;
>> > > > + bool auto_transition_zones;
>> > > > } NvmeNamespaceParams;
>> > > >
>> > > > typedef struct NvmeNamespace {
>> > > > --
>> > > > 2.31.1
>> > > >
>> > >
>> > > Looks good Niklas!
>> > >
>> > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> >
>> > In reality, it is the controller that does the auto transitioning.
>> >
>> > In theory, one namespace could be attached to two different controllers,
>> > and I guess, in that case, it depends on if the controller that we used
>> > when doing the write supports auto transitioning or not, that determines
>> > if a zone will be auto transitioned or not.
>> >
>> > If we were to change this to be a parameter of the controller instead
>> > of a parameter of the namespace, we would require to refactor a lot of
>> > code in the regular write path. As we currently don't have any NvmeRequest
>> > object in nvme_zrm_open_flags().
>> >
>> > Thoughts?
>> >
>>
>> I think you are right. This should be controller-specific behavior. I took
>> the liberty of moving the parameter; the refactor is minimal.
>>
>>
>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>
>> In the Zoned Namespace Command Set Specification, chapter
>> 2.5.1 Managing resources
>>
>> "The controller may transition zones in the ZSIO:Implicitly Opened state
>> to the ZSC:Closed state for resource management purposes."
>>
>> The word may in this sentence means that automatically transitioning
>> an implicitly opened zone to closed is completely optional.
>>
>> Add a new parameter so that the user can control if this automatic
>> transitioning should be performed or not.
>>
>> Being able to control this can help with verifying that e.g. a user-space
>> program behaves properly even without this optional ZNS feature.
>>
>> The default value is set to true, in order to not change the existing
>> behavior.
>>
>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> [k.jensen: moved parameter to controller]
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>> hw/nvme/nvme.h | 1 +
>> hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
>> 2 files changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> index 81a35cda142b..93a7e0e5380e 100644
>> --- a/hw/nvme/nvme.h
>> +++ b/hw/nvme/nvme.h
>> @@ -382,6 +382,7 @@ typedef struct NvmeParams {
>> uint8_t vsl;
>> bool use_intel_id;
>> uint8_t zasl;
>> + bool auto_transition_zones;
>> bool legacy_cmb;
>> } NvmeParams;
>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> index 40a7efcea914..8dd9cb2ccbf3 100644
>> --- a/hw/nvme/ctrl.c
>> +++ b/hw/nvme/ctrl.c
>> @@ -34,6 +34,7 @@
>> * aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
>> * mdts=<N[optional]>,vsl=<N[optional]>, \
>> * zoned.zasl=<N[optional]>, \
>> + * zoned.auto_transition=<on|off[optional]>, \
>> * subsys=<subsys_id>
>> * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
>> * zoned=<true|false[optional]>, \
>> @@ -100,6 +101,11 @@
>> * the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
>> * defaulting to the value of `mdts`).
>> *
>> + * - `zoned.auto_transition`
>> + * Indicates if zones in zone state implicitly opened can be automatically
>> + * transitioned to zone state closed for resource management purposes.
>> + * Defaults to 'on'.
>> + *
>> * nvme namespace device parameters
>> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> * - `shared`
>> @@ -1686,8 +1692,8 @@ enum {
>> NVME_ZRM_AUTO = 1 << 0,
>> };
>> -static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> - int flags)
>> +static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
>> + NvmeZone *zone, int flags)
>> {
>> int act = 0;
>> uint16_t status;
>> @@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> /* fallthrough */
>> case NVME_ZONE_STATE_CLOSED:
>> - nvme_zrm_auto_transition_zone(ns);
>> + if (n->params.auto_transition_zones) {
>> + nvme_zrm_auto_transition_zone(ns);
>> + }
>> status = nvme_aor_check(ns, act, 1);
>> if (status) {
>> return status;
>> @@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> }
>> }
>> -static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
>> +static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
>> + NvmeZone *zone)
>> {
>> - return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
>> + return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
>> }
>> -static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
>> +static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
>> + NvmeZone *zone)
>> {
>> - return nvme_zrm_open_flags(ns, zone, 0);
>> + return nvme_zrm_open_flags(n, ns, zone, 0);
>> }
>> static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
>> @@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
>> goto invalid;
>> }
>> - status = nvme_zrm_auto(ns, zone);
>> + status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
>> if (status) {
>> goto invalid;
>> }
>> @@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
>> goto invalid;
>> }
>> - status = nvme_zrm_auto(ns, zone);
>> + status = nvme_zrm_auto(n, ns, zone);
>> if (status) {
>> goto invalid;
>> }
>> @@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
>> static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
>> NvmeZoneState state, NvmeRequest *req)
>> {
>> - return nvme_zrm_open(ns, zone);
>> + return nvme_zrm_open(nvme_ctrl(req), ns, zone);
>> }
>> static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
>> @@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
>> DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
>> DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
>> DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
>> + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
>> + params.auto_transition_zones, true),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>> --
>> 2.31.1
>>
>
>Thanks a lot Klaus! I really appreciate it.
>
>My initial thought was to add a new flag in the enum where NVME_ZRM_AUTO is.
>But I think that would just make the code harder to read.
>You simply check the parameter directly, which is more obvious to the reader,
>so I think patch looks good!
>
Can I add your reviewed-by on this? :)
On Mon, Jun 07, 2021 at 11:54:02AM +0200, Klaus Jensen wrote:
> On Jun 1 07:30, Niklas Cassel wrote:
> > On Mon, May 31, 2021 at 09:39:20PM +0200, Klaus Jensen wrote:
> > > On May 31 15:42, Niklas Cassel wrote:
> > > > On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
> > > > > On May 28 11:05, Niklas Cassel wrote:
> > > > > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > > > >
> > > > > > In the Zoned Namespace Command Set Specification, chapter
> > > > > > 2.5.1 Managing resources
> > > > > >
> > > > > > "The controller may transition zones in the ZSIO:Implicitly Opened state
> > > > > > to the ZSC:Closed state for resource management purposes."
> > > > > >
> > > > > > The word may in this sentence means that automatically transitioning
> > > > > > an implicitly opened zone to closed is completely optional.
> > > > > >
> > > > > > Add a new parameter so that the user can control if this automatic
> > > > > > transitioning should be performed or not.
> > > > > >
> > > > > > Being able to control this can help with verifying that e.g. a user-space
> > > > > > program behaves properly even without this optional ZNS feature.
> > > > > >
> > > > > > The default value is set to true, in order to not change the existing
> > > > > > behavior.
> > > > > >
> > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > > > > ---
> > > > > > hw/nvme/ctrl.c | 9 ++++++++-
> > > > > > hw/nvme/ns.c | 2 ++
> > > > > > hw/nvme/nvme.h | 1 +
> > > > > > 3 files changed, 11 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > > > > index 40a7efcea9..d00f0297a5 100644
> > > > > > --- a/hw/nvme/ctrl.c
> > > > > > +++ b/hw/nvme/ctrl.c
> > > > > > @@ -141,6 +141,11 @@
> > > > > > *
> > > > > > * zoned.cross_read=<enable RAZB, default: false>
> > > > > > * Setting this property to true enables Read Across Zone Boundaries.
> > > > > > + *
> > > > > > + * zoned.auto_transition=<enable auto resource management, default: true>
> > > > > > + * Indicates if zones in zone state implicitly opened can be
> > > > > > + * automatically transitioned to zone state closed for resource
> > > > > > + * management purposes.
> > > > > > */
> > > > > >
> > > > > > #include "qemu/osdep.h"
> > > > > > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > > > > > /* fallthrough */
> > > > > >
> > > > > > case NVME_ZONE_STATE_CLOSED:
> > > > > > - nvme_zrm_auto_transition_zone(ns);
> > > > > > + if (ns->params.auto_transition_zones) {
> > > > > > + nvme_zrm_auto_transition_zone(ns);
> > > > > > + }
> > > > > > status = nvme_aor_check(ns, act, 1);
> > > > > > if (status) {
> > > > > > return status;
> > > > > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> > > > > > index 3fec9c6273..31dee43d30 100644
> > > > > > --- a/hw/nvme/ns.c
> > > > > > +++ b/hw/nvme/ns.c
> > > > > > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
> > > > > > params.max_open_zones, 0),
> > > > > > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
> > > > > > params.zd_extension_size, 0),
> > > > > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
> > > > > > + params.auto_transition_zones, true),
> > > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > > };
> > > > > >
> > > > > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> > > > > > index 81a35cda14..bd86054db2 100644
> > > > > > --- a/hw/nvme/nvme.h
> > > > > > +++ b/hw/nvme/nvme.h
> > > > > > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
> > > > > > uint32_t max_active_zones;
> > > > > > uint32_t max_open_zones;
> > > > > > uint32_t zd_extension_size;
> > > > > > + bool auto_transition_zones;
> > > > > > } NvmeNamespaceParams;
> > > > > >
> > > > > > typedef struct NvmeNamespace {
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > > > > Looks good Niklas!
> > > > >
> > > > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> > > >
> > > > In reality, it is the controller that does the auto transitioning.
> > > >
> > > > In theory, one namespace could be attached to two different controllers,
> > > > and I guess, in that case, it depends on if the controller that we used
> > > > when doing the write supports auto transitioning or not, that determines
> > > > if a zone will be auto transitioned or not.
> > > >
> > > > If we were to change this to be a parameter of the controller instead
> > > > of a parameter of the namespace, we would require to refactor a lot of
> > > > code in the regular write path. As we currently don't have any NvmeRequest
> > > > object in nvme_zrm_open_flags().
> > > >
> > > > Thoughts?
> > > >
> > >
> > > I think you are right. This should be controller-specific behavior. I took
> > > the liberty of moving the parameter; the refactor is minimal.
> > >
> > >
> > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > >
> > > In the Zoned Namespace Command Set Specification, chapter
> > > 2.5.1 Managing resources
> > >
> > > "The controller may transition zones in the ZSIO:Implicitly Opened state
> > > to the ZSC:Closed state for resource management purposes."
> > >
> > > The word may in this sentence means that automatically transitioning
> > > an implicitly opened zone to closed is completely optional.
> > >
> > > Add a new parameter so that the user can control if this automatic
> > > transitioning should be performed or not.
> > >
> > > Being able to control this can help with verifying that e.g. a user-space
> > > program behaves properly even without this optional ZNS feature.
> > >
> > > The default value is set to true, in order to not change the existing
> > > behavior.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > > [k.jensen: moved parameter to controller]
> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > ---
> > > hw/nvme/nvme.h | 1 +
> > > hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
> > > 2 files changed, 23 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> > > index 81a35cda142b..93a7e0e5380e 100644
> > > --- a/hw/nvme/nvme.h
> > > +++ b/hw/nvme/nvme.h
> > > @@ -382,6 +382,7 @@ typedef struct NvmeParams {
> > > uint8_t vsl;
> > > bool use_intel_id;
> > > uint8_t zasl;
> > > + bool auto_transition_zones;
> > > bool legacy_cmb;
> > > } NvmeParams;
> > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > index 40a7efcea914..8dd9cb2ccbf3 100644
> > > --- a/hw/nvme/ctrl.c
> > > +++ b/hw/nvme/ctrl.c
> > > @@ -34,6 +34,7 @@
> > > * aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
> > > * mdts=<N[optional]>,vsl=<N[optional]>, \
> > > * zoned.zasl=<N[optional]>, \
> > > + * zoned.auto_transition=<on|off[optional]>, \
> > > * subsys=<subsys_id>
> > > * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
> > > * zoned=<true|false[optional]>, \
> > > @@ -100,6 +101,11 @@
> > > * the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
> > > * defaulting to the value of `mdts`).
> > > *
> > > + * - `zoned.auto_transition`
> > > + * Indicates if zones in zone state implicitly opened can be automatically
> > > + * transitioned to zone state closed for resource management purposes.
> > > + * Defaults to 'on'.
> > > + *
> > > * nvme namespace device parameters
> > > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > * - `shared`
> > > @@ -1686,8 +1692,8 @@ enum {
> > > NVME_ZRM_AUTO = 1 << 0,
> > > };
> > > -static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > > - int flags)
> > > +static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
> > > + NvmeZone *zone, int flags)
> > > {
> > > int act = 0;
> > > uint16_t status;
> > > @@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > > /* fallthrough */
> > > case NVME_ZONE_STATE_CLOSED:
> > > - nvme_zrm_auto_transition_zone(ns);
> > > + if (n->params.auto_transition_zones) {
> > > + nvme_zrm_auto_transition_zone(ns);
> > > + }
> > > status = nvme_aor_check(ns, act, 1);
> > > if (status) {
> > > return status;
> > > @@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
> > > }
> > > }
> > > -static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
> > > +static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
> > > + NvmeZone *zone)
> > > {
> > > - return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
> > > + return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
> > > }
> > > -static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
> > > +static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
> > > + NvmeZone *zone)
> > > {
> > > - return nvme_zrm_open_flags(ns, zone, 0);
> > > + return nvme_zrm_open_flags(n, ns, zone, 0);
> > > }
> > > static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
> > > @@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
> > > goto invalid;
> > > }
> > > - status = nvme_zrm_auto(ns, zone);
> > > + status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
> > > if (status) {
> > > goto invalid;
> > > }
> > > @@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
> > > goto invalid;
> > > }
> > > - status = nvme_zrm_auto(ns, zone);
> > > + status = nvme_zrm_auto(n, ns, zone);
> > > if (status) {
> > > goto invalid;
> > > }
> > > @@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
> > > static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
> > > NvmeZoneState state, NvmeRequest *req)
> > > {
> > > - return nvme_zrm_open(ns, zone);
> > > + return nvme_zrm_open(nvme_ctrl(req), ns, zone);
> > > }
> > > static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
> > > @@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
> > > DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
> > > DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
> > > DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
> > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
> > > + params.auto_transition_zones, true),
> > > DEFINE_PROP_END_OF_LIST(),
> > > };
> > > --
> > > 2.31.1
> > >
> >
> > Thanks a lot Klaus! I really appreciate it.
> >
> > My initial thought was to add a new flag in the enum where NVME_ZRM_AUTO is.
> > But I think that would just make the code harder to read.
> > You simply check the parameter directly, which is more obvious to the reader,
> > so I think patch looks good!
> >
>
> Can I add your reviewed-by on this? :)
Yes, of course:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
However, in many projects you can't have a Reviewed-by that is the same as
the author, but perhaps that is not the case in QEMU.
Kind regards,
Niklas
On Jun 7 09:58, Niklas Cassel wrote:
>On Mon, Jun 07, 2021 at 11:54:02AM +0200, Klaus Jensen wrote:
>> On Jun 1 07:30, Niklas Cassel wrote:
>> > On Mon, May 31, 2021 at 09:39:20PM +0200, Klaus Jensen wrote:
>> > > On May 31 15:42, Niklas Cassel wrote:
>> > > > On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
>> > > > > On May 28 11:05, Niklas Cassel wrote:
>> > > > > > From: Niklas Cassel <niklas.cassel@wdc.com>
>> > > > > >
>> > > > > > In the Zoned Namespace Command Set Specification, chapter
>> > > > > > 2.5.1 Managing resources
>> > > > > >
>> > > > > > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > > > > > to the ZSC:Closed state for resource management purposes."
>> > > > > >
>> > > > > > The word may in this sentence means that automatically transitioning
>> > > > > > an implicitly opened zone to closed is completely optional.
>> > > > > >
>> > > > > > Add a new parameter so that the user can control if this automatic
>> > > > > > transitioning should be performed or not.
>> > > > > >
>> > > > > > Being able to control this can help with verifying that e.g. a user-space
>> > > > > > program behaves properly even without this optional ZNS feature.
>> > > > > >
>> > > > > > The default value is set to true, in order to not change the existing
>> > > > > > behavior.
>> > > > > >
>> > > > > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > > > > > ---
>> > > > > > hw/nvme/ctrl.c | 9 ++++++++-
>> > > > > > hw/nvme/ns.c | 2 ++
>> > > > > > hw/nvme/nvme.h | 1 +
>> > > > > > 3 files changed, 11 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > > > > index 40a7efcea9..d00f0297a5 100644
>> > > > > > --- a/hw/nvme/ctrl.c
>> > > > > > +++ b/hw/nvme/ctrl.c
>> > > > > > @@ -141,6 +141,11 @@
>> > > > > > *
>> > > > > > * zoned.cross_read=<enable RAZB, default: false>
>> > > > > > * Setting this property to true enables Read Across Zone Boundaries.
>> > > > > > + *
>> > > > > > + * zoned.auto_transition=<enable auto resource management, default: true>
>> > > > > > + * Indicates if zones in zone state implicitly opened can be
>> > > > > > + * automatically transitioned to zone state closed for resource
>> > > > > > + * management purposes.
>> > > > > > */
>> > > > > >
>> > > > > > #include "qemu/osdep.h"
>> > > > > > @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > > > > /* fallthrough */
>> > > > > >
>> > > > > > case NVME_ZONE_STATE_CLOSED:
>> > > > > > - nvme_zrm_auto_transition_zone(ns);
>> > > > > > + if (ns->params.auto_transition_zones) {
>> > > > > > + nvme_zrm_auto_transition_zone(ns);
>> > > > > > + }
>> > > > > > status = nvme_aor_check(ns, act, 1);
>> > > > > > if (status) {
>> > > > > > return status;
>> > > > > > diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>> > > > > > index 3fec9c6273..31dee43d30 100644
>> > > > > > --- a/hw/nvme/ns.c
>> > > > > > +++ b/hw/nvme/ns.c
>> > > > > > @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>> > > > > > params.max_open_zones, 0),
>> > > > > > DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>> > > > > > params.zd_extension_size, 0),
>> > > > > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>> > > > > > + params.auto_transition_zones, true),
>> > > > > > DEFINE_PROP_END_OF_LIST(),
>> > > > > > };
>> > > > > >
>> > > > > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > > > > > index 81a35cda14..bd86054db2 100644
>> > > > > > --- a/hw/nvme/nvme.h
>> > > > > > +++ b/hw/nvme/nvme.h
>> > > > > > @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
>> > > > > > uint32_t max_active_zones;
>> > > > > > uint32_t max_open_zones;
>> > > > > > uint32_t zd_extension_size;
>> > > > > > + bool auto_transition_zones;
>> > > > > > } NvmeNamespaceParams;
>> > > > > >
>> > > > > > typedef struct NvmeNamespace {
>> > > > > > --
>> > > > > > 2.31.1
>> > > > > >
>> > > > >
>> > > > > Looks good Niklas!
>> > > > >
>> > > > > Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> > > >
>> > > > In reality, it is the controller that does the auto transitioning.
>> > > >
>> > > > In theory, one namespace could be attached to two different controllers,
>> > > > and I guess, in that case, it depends on if the controller that we used
>> > > > when doing the write supports auto transitioning or not, that determines
>> > > > if a zone will be auto transitioned or not.
>> > > >
>> > > > If we were to change this to be a parameter of the controller instead
>> > > > of a parameter of the namespace, we would require to refactor a lot of
>> > > > code in the regular write path. As we currently don't have any NvmeRequest
>> > > > object in nvme_zrm_open_flags().
>> > > >
>> > > > Thoughts?
>> > > >
>> > >
>> > > I think you are right. This should be controller-specific behavior. I took
>> > > the liberty of moving the parameter; the refactor is minimal.
>> > >
>> > >
>> > > From: Niklas Cassel <niklas.cassel@wdc.com>
>> > >
>> > > In the Zoned Namespace Command Set Specification, chapter
>> > > 2.5.1 Managing resources
>> > >
>> > > "The controller may transition zones in the ZSIO:Implicitly Opened state
>> > > to the ZSC:Closed state for resource management purposes."
>> > >
>> > > The word may in this sentence means that automatically transitioning
>> > > an implicitly opened zone to closed is completely optional.
>> > >
>> > > Add a new parameter so that the user can control if this automatic
>> > > transitioning should be performed or not.
>> > >
>> > > Being able to control this can help with verifying that e.g. a user-space
>> > > program behaves properly even without this optional ZNS feature.
>> > >
>> > > The default value is set to true, in order to not change the existing
>> > > behavior.
>> > >
>> > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>> > > [k.jensen: moved parameter to controller]
>> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> > > ---
>> > > hw/nvme/nvme.h | 1 +
>> > > hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
>> > > 2 files changed, 23 insertions(+), 10 deletions(-)
>> > >
>> > > diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>> > > index 81a35cda142b..93a7e0e5380e 100644
>> > > --- a/hw/nvme/nvme.h
>> > > +++ b/hw/nvme/nvme.h
>> > > @@ -382,6 +382,7 @@ typedef struct NvmeParams {
>> > > uint8_t vsl;
>> > > bool use_intel_id;
>> > > uint8_t zasl;
>> > > + bool auto_transition_zones;
>> > > bool legacy_cmb;
>> > > } NvmeParams;
>> > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>> > > index 40a7efcea914..8dd9cb2ccbf3 100644
>> > > --- a/hw/nvme/ctrl.c
>> > > +++ b/hw/nvme/ctrl.c
>> > > @@ -34,6 +34,7 @@
>> > > * aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
>> > > * mdts=<N[optional]>,vsl=<N[optional]>, \
>> > > * zoned.zasl=<N[optional]>, \
>> > > + * zoned.auto_transition=<on|off[optional]>, \
>> > > * subsys=<subsys_id>
>> > > * -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
>> > > * zoned=<true|false[optional]>, \
>> > > @@ -100,6 +101,11 @@
>> > > * the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
>> > > * defaulting to the value of `mdts`).
>> > > *
>> > > + * - `zoned.auto_transition`
>> > > + * Indicates if zones in zone state implicitly opened can be automatically
>> > > + * transitioned to zone state closed for resource management purposes.
>> > > + * Defaults to 'on'.
>> > > + *
>> > > * nvme namespace device parameters
>> > > * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > > * - `shared`
>> > > @@ -1686,8 +1692,8 @@ enum {
>> > > NVME_ZRM_AUTO = 1 << 0,
>> > > };
>> > > -static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > - int flags)
>> > > +static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone, int flags)
>> > > {
>> > > int act = 0;
>> > > uint16_t status;
>> > > @@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > /* fallthrough */
>> > > case NVME_ZONE_STATE_CLOSED:
>> > > - nvme_zrm_auto_transition_zone(ns);
>> > > + if (n->params.auto_transition_zones) {
>> > > + nvme_zrm_auto_transition_zone(ns);
>> > > + }
>> > > status = nvme_aor_check(ns, act, 1);
>> > > if (status) {
>> > > return status;
>> > > @@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>> > > }
>> > > }
>> > > -static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
>> > > +static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone)
>> > > {
>> > > - return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
>> > > + return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
>> > > }
>> > > -static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
>> > > +static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
>> > > + NvmeZone *zone)
>> > > {
>> > > - return nvme_zrm_open_flags(ns, zone, 0);
>> > > + return nvme_zrm_open_flags(n, ns, zone, 0);
>> > > }
>> > > static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
>> > > @@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
>> > > goto invalid;
>> > > }
>> > > - status = nvme_zrm_auto(ns, zone);
>> > > + status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
>> > > if (status) {
>> > > goto invalid;
>> > > }
>> > > @@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
>> > > goto invalid;
>> > > }
>> > > - status = nvme_zrm_auto(ns, zone);
>> > > + status = nvme_zrm_auto(n, ns, zone);
>> > > if (status) {
>> > > goto invalid;
>> > > }
>> > > @@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
>> > > static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
>> > > NvmeZoneState state, NvmeRequest *req)
>> > > {
>> > > - return nvme_zrm_open(ns, zone);
>> > > + return nvme_zrm_open(nvme_ctrl(req), ns, zone);
>> > > }
>> > > static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
>> > > @@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
>> > > DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
>> > > DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
>> > > DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
>> > > + DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
>> > > + params.auto_transition_zones, true),
>> > > DEFINE_PROP_END_OF_LIST(),
>> > > };
>> > > --
>> > > 2.31.1
>> > >
>> >
>> > Thanks a lot Klaus! I really appreciate it.
>> >
>> > My initial thought was to add a new flag in the enum where NVME_ZRM_AUTO is.
>> > But I think that would just make the code harder to read.
>> > You simply check the parameter directly, which is more obvious to the reader,
>> > so I think patch looks good!
>> >
>>
>> Can I add your reviewed-by on this? :)
>
>Yes, of course:
>
>Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
>
>However, in many projects you can't have a Reviewed-by that is the same as
>the author, but perhaps that is not the case in QEMU.
>
Ah doh - of course. I'll put my Sign-off and Reviewed-by on with you as
author of course ;) Forgot that I kept you as Author!
Thanks!
© 2016 - 2024 Red Hat, Inc.