qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Klaus Jensen
Subject: Re: [PATCH v3] hw/block/nvme: fix lbaf formats initialization
Date: Tue, 20 Apr 2021 21:47:00 +0200

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

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

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]