Re: [PATCH 2/2] hw/block/nvme: fix lbaf formats initialization

[Gollu Appalanaidu]

On Fri, Apr 16, 2021 at 10:48:16AM +0200, Klaus Jensen wrote:
> On Apr 16 12:52, Gollu Appalanaidu wrote:
> > Currently LBAF formats are being intialized based on metadata
> > size if and only if nvme-ns "ms" parameter is non-zero value.
> > Since FormatNVM command being supported device parameter "ms"
> > may not be the criteria to initialize the supported LBAFs.
> >
> > Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > ---
> > hw/block/nvme-ns.c | 48 ++++++++++++++++++----------------------------
> > 1 file changed, 19 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 7bb618f182..573dbb5a9d 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -85,38 +85,28 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
> >    ds = 31 - clz32(ns->blkconf.logical_block_size);
> >    ms = ns->params.ms;
> >
> > -    if (ns->params.ms) {
> > -        id_ns->mc = 0x3;
> > +    id_ns->mc = 0x3;
> >
> > -        if (ns->params.mset) {
> > -            id_ns->flbas |= 0x10;
> > -        }
> > +    if (ms && ns->params.mset) {
> > +        id_ns->flbas |= 0x10;
> > +    }
> >
> > -        id_ns->dpc = 0x1f;
> > -        id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi;
> > -
> > -        NvmeLBAF lbaf[16] = {
> > -            [0] = { .ds =  9           },
> > -            [1] = { .ds =  9, .ms =  8 },
> > -            [2] = { .ds =  9, .ms = 16 },
> > -            [3] = { .ds =  9, .ms = 64 },
> > -            [4] = { .ds = 12           },
> > -            [5] = { .ds = 12, .ms =  8 },
> > -            [6] = { .ds = 12, .ms = 16 },
> > -            [7] = { .ds = 12, .ms = 64 },
> > -        };
> > -
> > -        memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf));
> > -        id_ns->nlbaf = 7;
> > -    } else {
> > -        NvmeLBAF lbaf[16] = {
> > -            [0] = { .ds =  9 },
> > -            [1] = { .ds = 12 },
> > -        };
> > +    id_ns->dpc = 0x1f;
> > +    id_ns->dps = ((ns->params.pil & 0x1) << 3) | ns->params.pi;
>
> While nvme_ns_check_constraints() will error out if pi is set and the 
> metadata bytes are insufficient, I don't think this should set bit 3 
> unless both metadata and pi is enabled. It's not against the spec, but 
> it's just slightly weird.

okay, will modify current "ms" and "pi" constraint check like this:
if (ns->params.ms < 8) {
       if (ns->params.pi || ns->params.pil || ns->params.mset) {
           error_setg(errp, "at least 8 bytes of metadata required to enable "
                   "protection information, protection information location and 
"
                   "metadata settings");
           return -1;
       }
}
and "mset" is set will set directly without checing "ms" in nvme_ns_init()
> > -        memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf));
> > -        id_ns->nlbaf = 1;
> > -    }
> > +    NvmeLBAF lbaf[16] = {
> > +        [0] = { .ds =  9           },
> > +        [1] = { .ds =  9, .ms =  8 },
> > +        [2] = { .ds =  9, .ms = 16 },
> > +        [3] = { .ds =  9, .ms = 64 },
> > +        [4] = { .ds = 12           },
> > +        [5] = { .ds = 12, .ms =  8 },
> > +        [6] = { .ds = 12, .ms = 16 },
> > +        [7] = { .ds = 12, .ms = 64 },
> > +    };
> > +
> > +    memcpy(&id_ns->lbaf, &lbaf, sizeof(lbaf));
> > +    id_ns->nlbaf = 7;
> >
> >    for (i = 0; i <= id_ns->nlbaf; i++) {
> >        NvmeLBAF *lbaf = &id_ns->lbaf[i];
> > --
> > 2.17.1