* [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed @ 2021-05-28 11:05 Niklas Cassel 2021-05-28 11:22 ` Klaus Jensen 0 siblings, 1 reply; 9+ messages in thread From: Niklas Cassel @ 2021-05-28 11:05 UTC (permalink / raw) To: kbusch, its; +Cc: Niklas Cassel, qemu-devel, qemu-block 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-05-28 11:05 [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed Niklas Cassel @ 2021-05-28 11:22 ` Klaus Jensen 2021-05-31 15:42 ` Niklas Cassel 0 siblings, 1 reply; 9+ messages in thread From: Klaus Jensen @ 2021-05-28 11:22 UTC (permalink / raw) To: Niklas Cassel; +Cc: kbusch, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 3037 bytes --] 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> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-05-28 11:22 ` Klaus Jensen @ 2021-05-31 15:42 ` Niklas Cassel 2021-05-31 19:39 ` Klaus Jensen 0 siblings, 1 reply; 9+ messages in thread From: Niklas Cassel @ 2021-05-31 15:42 UTC (permalink / raw) To: Klaus Jensen; +Cc: kbusch, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-05-31 15:42 ` Niklas Cassel @ 2021-05-31 19:39 ` Klaus Jensen 2021-06-01 7:30 ` Niklas Cassel 2021-06-07 10:08 ` Klaus Jensen 0 siblings, 2 replies; 9+ messages in thread From: Klaus Jensen @ 2021-05-31 19:39 UTC (permalink / raw) To: Niklas Cassel; +Cc: kbusch, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 9644 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-05-31 19:39 ` Klaus Jensen @ 2021-06-01 7:30 ` Niklas Cassel 2021-06-07 9:54 ` Klaus Jensen 2021-06-07 10:08 ` Klaus Jensen 1 sibling, 1 reply; 9+ messages in thread From: Niklas Cassel @ 2021-06-01 7:30 UTC (permalink / raw) To: Klaus Jensen; +Cc: kbusch, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-06-01 7:30 ` Niklas Cassel @ 2021-06-07 9:54 ` Klaus Jensen 2021-06-07 9:58 ` Niklas Cassel 0 siblings, 1 reply; 9+ messages in thread From: Klaus Jensen @ 2021-06-07 9:54 UTC (permalink / raw) To: Niklas Cassel; +Cc: kbusch, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 10559 bytes --] 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? :) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-06-07 9:54 ` Klaus Jensen @ 2021-06-07 9:58 ` Niklas Cassel 2021-06-07 10:04 ` Klaus Jensen 0 siblings, 1 reply; 9+ messages in thread From: Niklas Cassel @ 2021-06-07 9:58 UTC (permalink / raw) To: Klaus Jensen; +Cc: kbusch, qemu-devel, qemu-block 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-06-07 9:58 ` Niklas Cassel @ 2021-06-07 10:04 ` Klaus Jensen 0 siblings, 0 replies; 9+ messages in thread From: Klaus Jensen @ 2021-06-07 10:04 UTC (permalink / raw) To: Niklas Cassel; +Cc: kbusch, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 12298 bytes --] 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! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed 2021-05-31 19:39 ` Klaus Jensen 2021-06-01 7:30 ` Niklas Cassel @ 2021-06-07 10:08 ` Klaus Jensen 1 sibling, 0 replies; 9+ messages in thread From: Klaus Jensen @ 2021-06-07 10:08 UTC (permalink / raw) To: Niklas Cassel; +Cc: kbusch, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 9787 bytes --] On May 31 21:39, 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 > Modified patch applied to nvme-next! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-06-07 10:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-28 11:05 [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed Niklas Cassel 2021-05-28 11:22 ` Klaus Jensen 2021-05-31 15:42 ` Niklas Cassel 2021-05-31 19:39 ` Klaus Jensen 2021-06-01 7:30 ` Niklas Cassel 2021-06-07 9:54 ` Klaus Jensen 2021-06-07 9:58 ` Niklas Cassel 2021-06-07 10:04 ` Klaus Jensen 2021-06-07 10:08 ` Klaus Jensen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.