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