Re: [PATCH v5 09/14] hw/block/nvme: Support Zoned Namespace Command Set

[Klaus Jensen]

On Sep 30 14:50, Niklas Cassel wrote:
> On Mon, Sep 28, 2020 at 11:35:23AM +0900, Dmitry Fomichev wrote:
> > The emulation code has been changed to advertise NVM Command Set when
> > "zoned" device property is not set (default) and Zoned Namespace
> > Command Set otherwise.
> > 
> > Handlers for three new NVMe commands introduced in Zoned Namespace
> > Command Set specification are added, namely for Zone Management
> > Receive, Zone Management Send and Zone Append.
> > 
> > Device initialization code has been extended to create a proper
> > configuration for zoned operation using device properties.
> > 
> > Read/Write command handler is modified to only allow writes at the
> > write pointer if the namespace is zoned. For Zone Append command,
> > writes implicitly happen at the write pointer and the starting write
> > pointer value is returned as the result of the command. Write Zeroes
> > handler is modified to add zoned checks that are identical to those
> > done as a part of Write flow.
> > 
> > The code to support for Zone Descriptor Extensions is not included in
> > this commit and ZDES 0 is always reported. A later commit in this
> > series will add ZDE support.
> > 
> > This commit doesn't yet include checks for active and open zone
> > limits. It is assumed that there are no limits on either active or
> > open zones.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> > Signed-off-by: Ajay Joshi <ajay.joshi@wdc.com>
> > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > Signed-off-by: Matias Bjorling <matias.bjorling@wdc.com>
> > Signed-off-by: Aravind Ramesh <aravind.ramesh@wdc.com>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Signed-off-by: Adam Manzanares <adam.manzanares@wdc.com>
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  block/nvme.c         |   2 +-
> >  hw/block/nvme-ns.c   | 185 ++++++++-
> >  hw/block/nvme-ns.h   |   6 +-
> >  hw/block/nvme.c      | 872 +++++++++++++++++++++++++++++++++++++++++--
> >  include/block/nvme.h |   6 +-
> >  5 files changed, 1033 insertions(+), 38 deletions(-)
> > 
> > diff --git a/block/nvme.c b/block/nvme.c
> > index 05485fdd11..7a513c9a17 100644
> > --- a/block/nvme.c
> > +++ b/block/nvme.c
> > @@ -333,7 +333,7 @@ static inline int nvme_translate_error(const NvmeCqe *c)
> >  {
> >      uint16_t status = (le16_to_cpu(c->status) >> 1) & 0xFF;
> >      if (status) {
> > -        trace_nvme_error(le32_to_cpu(c->result),
> > +        trace_nvme_error(le32_to_cpu(c->result32),
> >                           le16_to_cpu(c->sq_head),
> >                           le16_to_cpu(c->sq_id),
> >                           le16_to_cpu(c->cid),
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 31b7f986c3..6d9dc9205b 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -33,14 +33,14 @@ static void nvme_ns_init(NvmeNamespace *ns)
> >      NvmeIdNs *id_ns = &ns->id_ns;
> >  
> >      if (blk_get_flags(ns->blkconf.blk) & BDRV_O_UNMAP) {
> > -        ns->id_ns.dlfeat = 0x9;
> > +        ns->id_ns.dlfeat = 0x8;
> 
> You seem to change something that is NVM namespace specific here, why?
> If it is indeed needed, I assume that this change should be in a separate
> patch.
> 

Stood out to me as well - and I thought it was sound enough, but now I'm
not sure sure.

DLFEAT is set to 0x8, which only signifies that Deallocate in Write
Zeroes is supported. Previously, it would also signify that returned
values would be 0x00 (DLFEAT 0x8 | 0x1). But since Dmitry added the
fill_pattern parameter...


> > +static int nvme_zoned_init_ns(NvmeCtrl *n, NvmeNamespace *ns, int 
> > lba_index,
> > +                              Error **errp)
> > +{
> > +    NvmeIdNsZoned *id_ns_z;
> > +
> > +    if (n->params.fill_pattern == 0) {
> > +        ns->id_ns.dlfeat |= 0x01;
> > +    } else if (n->params.fill_pattern == 0xff) {
> > +        ns->id_ns.dlfeat |= 0x02;
> > +    }

... then, when initialized, we look at the fill_pattern and set DLFEAT
accordingly instead.

But since fill_pattern only works for ZNS namespaces, I think dlfeat
should still be 0x9 for NVM namespaces. For NVM namespaces, since
neither DULBE or DSM is not supported, there is really only Write Zeroes
that can explicitly "deallocate" a block, and since that *will* write
zeroes no matter if DEAC is set or not, 0x00 pattern is guaranteed.

signature.asc
Description: PGP signature