[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 05/11] hw/block/nvme: Support Zoned Namespace Command Set
From: |
Klaus Jensen |
Subject: |
Re: [PATCH v7 05/11] hw/block/nvme: Support Zoned Namespace Command Set |
Date: |
Tue, 20 Oct 2020 13:08:57 +0200 |
On Oct 19 11:17, Dmitry Fomichev wrote:
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 974aea33f7..fedfad595c 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -133,6 +320,12 @@ static Property nvme_ns_props[] = {
> DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
> DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
> DEFINE_PROP_BOOL("attached", NvmeNamespace, params.attached, true),
> + DEFINE_PROP_BOOL("zoned", NvmeNamespace, params.zoned, false),
Instead of using a 'zoned' property here, can we add an 'iocs' or 'csi'
property in the namespace types patch? Then, in the future if we add
additional command sets we won't need another property (like 'kv').
> + DEFINE_PROP_SIZE("zone_size", NvmeNamespace, params.zone_size_bs,
> + NVME_DEFAULT_ZONE_SIZE),
> + DEFINE_PROP_SIZE("zone_capacity", NvmeNamespace, params.zone_cap_bs, 0),
I would like that the zone_size and zone_capacity were named zoned.zsze
and zoned.zcap and were in terms of logical blocks, like in the spec.
Putting them in a pseudo-namespace makes it clear that the options
affect the zoned command set and reduces the risk of anything clashing
with the addition of other command sets (like 'kv') in the future.
> + DEFINE_PROP_BOOL("cross_zone_read", NvmeNamespace,
> + params.cross_zone_read, false),
Instead of cluttering the parameters with a bunch of these when others
zone operational characteristics are added, can we use a 'zoned.zoc'
parameter that matches the spec?
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 93728e51b3..34d0d0250d 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3079,6 +4001,9 @@ static Property nvme_props[] = {
> DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued,
> 64),
> DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
> DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
> + DEFINE_PROP_UINT8("fill_pattern", NvmeCtrl, params.fill_pattern, 0),
> + DEFINE_PROP_SIZE32("zone_append_size_limit", NvmeCtrl, params.zasl_bs,
> + NVME_DEFAULT_MAX_ZA_SIZE),
Similar to my reasoning above, I would like this to be zoned.zasl and in
terms of logical blocks like the spec. Also, I think '0' is a better
default since zero values typically identify a default value in the spec
as well.
I know this might sound like bikeshedding, but I wanna make sure that we
get the parameters right since we cannot get rid of them once they are
there. Following the definitions of the spec makes it very clear what
their meaning are and should be. 'mdts' is currently the only other
parameter like this, but that is also specified as in the spec, and not
as an absolute value.
My preference also applies to subsequent patches, like using `zoned.mor`
and `zoned.mar` for the resource limits.
signature.asc
Description: PGP signature
- Re: [PATCH v7 03/11] hw/block/nvme: Add support for Namespace Types, (continued)
- [PATCH v7 04/11] hw/block/nvme: Support allocated CNS command variants, Dmitry Fomichev, 2020/10/18
- [PATCH v7 05/11] hw/block/nvme: Support Zoned Namespace Command Set, Dmitry Fomichev, 2020/10/18
- [PATCH v7 07/11] hw/block/nvme: Support Zone Descriptor Extensions, Dmitry Fomichev, 2020/10/18
- [PATCH v7 08/11] hw/block/nvme: Add injection of Offline/Read-Only zones, Dmitry Fomichev, 2020/10/18
- [PATCH v7 06/11] hw/block/nvme: Introduce max active and open zone limits, Dmitry Fomichev, 2020/10/18
- [PATCH v7 09/11] hw/block/nvme: Document zoned parameters in usage text, Dmitry Fomichev, 2020/10/18
- [PATCH v7 10/11] hw/block/nvme: Separate read and write handlers, Dmitry Fomichev, 2020/10/18