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

[Gollu Appalanaidu <anaidu.gollu@samsung.com>]

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

On Tue, Apr 20, 2021 at 09:47:00PM +0200, Klaus Jensen wrote:
>On Apr 16 17:29, 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>
>>---
>>-v3: Remove "mset" constraint  check if ms < 8, "mset" can be
>>set even when ms < 8 and non-zero.
>>
>>-v2: Addressing review comments (Klaus)
>>Change the current "pi" and "ms" constraint check such that it
>>will throw the error if ms < 8 and if namespace protection info,
>>location and metadata settings are set.
>>Splitting this from compare fix patch series.
>>
>>hw/block/nvme-ns.c | 58 ++++++++++++++++++++--------------------------
>>1 file changed, 25 insertions(+), 33 deletions(-)
>>
>>diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
>>index 7bb618f182..594b0003cf 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];
>
>This part LGTM.
>
>>@@ -395,10 +385,12 @@ static int nvme_ns_check_constraints(NvmeCtrl *n, NvmeNamespace *ns,
>>        return -1;
>>    }
>>
>>-    if (ns->params.pi && ns->params.ms < 8) {
>>-        error_setg(errp, "at least 8 bytes of metadata required to enable "
>>-                   "protection information");
>>-        return -1;
>>+    if (ns->params.ms < 8) {
>>+        if (ns->params.pi || ns->params.pil) {
>>+            error_setg(errp, "at least 8 bytes of metadata required to enable "
>>+                    "protection information, protection information location");
>>+            return -1;
>>+        }
>>    }
>>
>
>If you do this additional check, then you should maybe also check that 
>pil is only set if pi is. But if pi is not enabled, then the value of 
>pil is irrelevant (even though it ends up in FLBAS). In other words, 
>if you want to validate all possible parameter configurations, then we 
>have a lot more checking to do!
>
>Currently, the approach taken by the parameter validation code is to 
>error out on *invalid* configurations that causes invariants to not 
>hold, and I'd prefer that we stick with that to keep the check logic 
>as simple as possible.
>
>So, (without this unnecessary check):
>

Sure, will remove this check and send v4

>Reviewed-by: Klaus Jensen <k.jensen@samsung.com>



[-- Attachment #2: Type: text/plain, Size: 0 bytes --]