| Message ID | 20210416072234.25732-2-anaidu.gollu@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] hw/block/nvme: consider metadata read aio return value in compare | expand |
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. > >- 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 > >
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 >> >> >
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; - 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];
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(-)