QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] hw/block/nvme: fix lbaf formats initialization
       [not found] <CGME20210416120324epcas5p354e2f196cc68680aba45341f385b59ed@epcas5p3.samsung.com>
@ 2021-04-16 11:59 ` Gollu Appalanaidu
  2021-04-20 19:47   ` Klaus Jensen
  2021-04-21 10:30   ` Philippe Mathieu-Daudé
  0 siblings, 2 replies; 4+ messages in thread
From: Gollu Appalanaidu @ 2021-04-16 11:59 UTC (permalink / raw)
  To: qemu-devel
  Cc: fam, kwolf, qemu-block, Gollu Appalanaidu, mreitz, its, stefanha, kbusch

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];
@@ -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 (ns->params.nsid > NVME_MAX_NAMESPACES) {
-- 
2.17.1



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] hw/block/nvme: fix lbaf formats initialization
  2021-04-16 11:59 ` [PATCH v3] hw/block/nvme: fix lbaf formats initialization Gollu Appalanaidu
@ 2021-04-20 19:47   ` Klaus Jensen
  2021-04-21 10:03     ` Gollu Appalanaidu
  2021-04-21 10:30   ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 4+ messages in thread
From: Klaus Jensen @ 2021-04-20 19:47 UTC (permalink / raw)
  To: Gollu Appalanaidu
  Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch


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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] hw/block/nvme: fix lbaf formats initialization
  2021-04-20 19:47   ` Klaus Jensen
@ 2021-04-21 10:03     ` Gollu Appalanaidu
  0 siblings, 0 replies; 4+ messages in thread
From: Gollu Appalanaidu @ 2021-04-21 10:03 UTC (permalink / raw)
  To: Klaus Jensen; +Cc: fam, kwolf, qemu-block, qemu-devel, mreitz, stefanha, kbusch


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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] hw/block/nvme: fix lbaf formats initialization
  2021-04-16 11:59 ` [PATCH v3] hw/block/nvme: fix lbaf formats initialization Gollu Appalanaidu
  2021-04-20 19:47   ` Klaus Jensen
@ 2021-04-21 10:30   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-21 10:30 UTC (permalink / raw)
  To: Gollu Appalanaidu, qemu-devel
  Cc: fam, kwolf, qemu-block, mreitz, kbusch, stefanha, its

On 4/16/21 1:59 PM, 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(-)

> +    NvmeLBAF lbaf[16] = {

Unrelated to your change, but better to use a read-only array:

       static const 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;



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210416120324epcas5p354e2f196cc68680aba45341f385b59ed@epcas5p3.samsung.com>
2021-04-16 11:59 ` [PATCH v3] hw/block/nvme: fix lbaf formats initialization Gollu Appalanaidu
2021-04-20 19:47   ` Klaus Jensen
2021-04-21 10:03     ` Gollu Appalanaidu
2021-04-21 10:30   ` Philippe Mathieu-Daudé

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git