[PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare

[PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare

[Gollu Appalanaidu]

Currently in compare command metadata aio read blk_aio_preadv return
value ignored, consider it and complete the block accounting.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
---
 hw/block/nvme.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 624a1431d0..c2727540f1 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
     uint32_t reftag = le32_to_cpu(rw->reftag);
     struct nvme_compare_ctx *ctx = req->opaque;
     g_autofree uint8_t *buf = NULL;
+    BlockBackend *blk = ns->blkconf.blk;
+    BlockAcctCookie *acct = &req->acct;
+    BlockAcctStats *stats = blk_get_stats(blk);
     uint16_t status = NVME_SUCCESS;
 
     trace_pci_nvme_compare_mdata_cb(nvme_cid(req));
 
+    if (ret) {
+        block_acct_failed(stats, acct);
+        nvme_aio_err(req, ret);
+        goto out;
+    }
+
     buf = g_malloc(ctx->mdata.iov.size);
 
     status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size,
@@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
         goto out;
     }
 
+    block_acct_done(stats, acct);
+
 out:
     qemu_iovec_destroy(&ctx->data.iov);
     g_free(ctx->data.bounce);
-- 
2.17.1

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

[Gollu Appalanaidu]

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

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

[Klaus Jensen]

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

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

[Gollu Appalanaidu]

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

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



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

Re: [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare

[Klaus Jensen]

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

On Apr 16 12:52, Gollu Appalanaidu wrote:
>Currently in compare command metadata aio read blk_aio_preadv return
>value ignored, consider it and complete the block accounting.
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
> hw/block/nvme.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 624a1431d0..c2727540f1 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
>     uint32_t reftag = le32_to_cpu(rw->reftag);
>     struct nvme_compare_ctx *ctx = req->opaque;
>     g_autofree uint8_t *buf = NULL;
>+    BlockBackend *blk = ns->blkconf.blk;
>+    BlockAcctCookie *acct = &req->acct;
>+    BlockAcctStats *stats = blk_get_stats(blk);
>     uint16_t status = NVME_SUCCESS;
>
>     trace_pci_nvme_compare_mdata_cb(nvme_cid(req));
>
>+    if (ret) {
>+        block_acct_failed(stats, acct);
>+        nvme_aio_err(req, ret);
>+        goto out;
>+    }
>+
>     buf = g_malloc(ctx->mdata.iov.size);
>
>     status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size,
>@@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
>         goto out;
>     }
>
>+    block_acct_done(stats, acct);
>+
> out:
>     qemu_iovec_destroy(&ctx->data.iov);
>     g_free(ctx->data.bounce);
>-- 
>2.17.1
>
>

Good fix, thanks! Since there is no crash, data corruption or other 
"bad" behavior, this isn't critical for v6.0.

Might consider it for a potential stable release though, so I'll add a 
Fixes: tag and queue it up.

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

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

Re: [PATCH 1/2] hw/block/nvme: consider metadata read aio return value in compare

[Klaus Jensen]

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

On Apr 16 12:52, Gollu Appalanaidu wrote:
>Currently in compare command metadata aio read blk_aio_preadv return
>value ignored, consider it and complete the block accounting.
>
>Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>---
> hw/block/nvme.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 624a1431d0..c2727540f1 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -2369,10 +2369,19 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
>     uint32_t reftag = le32_to_cpu(rw->reftag);
>     struct nvme_compare_ctx *ctx = req->opaque;
>     g_autofree uint8_t *buf = NULL;
>+    BlockBackend *blk = ns->blkconf.blk;
>+    BlockAcctCookie *acct = &req->acct;
>+    BlockAcctStats *stats = blk_get_stats(blk);
>     uint16_t status = NVME_SUCCESS;
>
>     trace_pci_nvme_compare_mdata_cb(nvme_cid(req));
>
>+    if (ret) {
>+        block_acct_failed(stats, acct);
>+        nvme_aio_err(req, ret);
>+        goto out;
>+    }
>+
>     buf = g_malloc(ctx->mdata.iov.size);
>
>     status = nvme_bounce_mdata(n, buf, ctx->mdata.iov.size,
>@@ -2421,6 +2430,8 @@ static void nvme_compare_mdata_cb(void *opaque, int ret)
>         goto out;
>     }
>
>+    block_acct_done(stats, acct);
>+
> out:
>     qemu_iovec_destroy(&ctx->data.iov);
>     g_free(ctx->data.bounce);
>-- 
>2.17.1
>

Applied to nvme-next, thanks!

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