From: Gollu Appalanaidu <anaidu.gollu@samsung.com>
To: Klaus Jensen <its@irrelevant.dk>
Cc: fam@euphon.net, kwolf@redhat.com, qemu-block@nongnu.org,
qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@redhat.com,
kbusch@kernel.org
Subject: Re: [PATCH v3] hw/block/nvme: fix lbaf formats initialization
Date: Wed, 21 Apr 2021 15:33:04 +0530 [thread overview]
Message-ID: <20210421100303.GA3854@2030045822> (raw)
In-Reply-To: <YH8vtPtI2OkOAkOw@apples.localdomain>
[-- 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 --]
next prev parent reply other threads:[~2021-04-21 10:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 [this message]
2021-04-21 10:30 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210421100303.GA3854@2030045822 \
--to=anaidu.gollu@samsung.com \
--cc=fam@euphon.net \
--cc=its@irrelevant.dk \
--cc=kbusch@kernel.org \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).