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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

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
>
>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]