diff mbox series

hw/block/nvme: improve invalid zasl value reporting

Message ID 20210208082532.308435-1-its@irrelevant.dk
State New
Headers show
Series hw/block/nvme: improve invalid zasl value reporting | expand

Commit Message

Klaus Jensen Feb. 8, 2021, 8:25 a.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
improve the user experience by adding an early parameter check in
nvme_check_constraints.

When ZASL is still too small due to the host configuring the device for
an even larger page size, convert the trace point in nvme_start_ctrl to
an NVME_GUEST_ERR such that this is logged by QEMU instead of only
traced.

Reported-by: "info@dantalion.nl" <info@dantalion.nl>
Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

info@dantalion.nl Feb. 8, 2021, 9:15 a.m. UTC | #1 
On 08-02-2021 09:25, Klaus Jensen wrote:
> The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
> improve the user experience by adding an early parameter check in
> nvme_check_constraints.

I have confirmed this and it works for me, I don't think I am actually
qualified or understand QEMUs source well enough to sign this off but
just wanted to let you know.

Thanks for the quick updates.

Kind regards,
Corne
Klaus Jensen Feb. 8, 2021, 9:19 a.m. UTC | #2 
On Feb  8 10:15, info@dantalion.nl wrote:
> On 08-02-2021 09:25, Klaus Jensen wrote:
> > The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
> > improve the user experience by adding an early parameter check in
> > nvme_check_constraints.
> 
> I have confirmed this and it works for me, I don't think I am actually
> qualified or understand QEMUs source well enough to sign this off but
> just wanted to let you know.
> 
> Thanks for the quick updates.
> 

I'll add a 'Tested-by: ...' tag from you.

Thanks for reporting and following up! ;)
Dmitry Fomichev Feb. 9, 2021, 7:39 p.m. UTC | #3 
On Mon, 2021-02-08 at 09:25 +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
> improve the user experience by adding an early parameter check in
> nvme_check_constraints.
> 
> When ZASL is still too small due to the host configuring the device for
> an even larger page size, convert the trace point in nvme_start_ctrl to
> an NVME_GUEST_ERR such that this is logged by QEMU instead of only
> traced.
> 
> Reported-by: "info@dantalion.nl" <info@dantalion.nl>
> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index c2f0c88fbf39..d96888cd2333 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3983,8 +3983,10 @@ static int nvme_start_ctrl(NvmeCtrl *n)
>          n->zasl = n->params.mdts;
>      } else {
>          if (n->params.zasl_bs < n->page_size) {
> -            trace_pci_nvme_err_startfail_zasl_too_small(n->params.zasl_bs,
> -                                                        n->page_size);
> +            NVME_GUEST_ERR(pci_nvme_err_startfail_zasl_too_small,
> +                           "Zone Append Size Limit (ZASL) of %d bytes is too "
> +                           "small; must be at least %d bytes",
> +                           n->params.zasl_bs, n->page_size);
>              return -1;
>          }
>          n->zasl = 31 - clz32(n->params.zasl_bs / n->page_size);
> @@ -4503,6 +4505,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
>              error_setg(errp, "zone append size limit has to be a power of 2");
>              return;
>          }
> +
> +        if (n->params.zasl_bs < 4096) {
> +            error_setg(errp, "zone append size limit must be at least "
> +                       "4096 bytes");
> +            return;
> +        }
>      }
>  }

The guest error is less confusing than simply a trace. LGTM.
Reviewed-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

>  
> 
> 
>
Philippe Mathieu-Daudé Feb. 10, 2021, 6:09 a.m. UTC | #4 
On 2/9/21 8:39 PM, Dmitry Fomichev wrote:
> On Mon, 2021-02-08 at 09:25 +0100, Klaus Jensen wrote:
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
>> improve the user experience by adding an early parameter check in
>> nvme_check_constraints.
>>
>> When ZASL is still too small due to the host configuring the device for
>> an even larger page size, convert the trace point in nvme_start_ctrl to
>> an NVME_GUEST_ERR such that this is logged by QEMU instead of only
>> traced.
>>
>> Reported-by: "info@dantalion.nl" <info@dantalion.nl>

Apparently the reporter signed 'Corne'.

>> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>  hw/block/nvme.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index c2f0c88fbf39..d96888cd2333 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -3983,8 +3983,10 @@ static int nvme_start_ctrl(NvmeCtrl *n)
>>          n->zasl = n->params.mdts;
>>      } else {
>>          if (n->params.zasl_bs < n->page_size) {
>> -            trace_pci_nvme_err_startfail_zasl_too_small(n->params.zasl_bs,
>> -                                                        n->page_size);
>> +            NVME_GUEST_ERR(pci_nvme_err_startfail_zasl_too_small,
>> +                           "Zone Append Size Limit (ZASL) of %d bytes is too "
>> +                           "small; must be at least %d bytes",
>> +                           n->params.zasl_bs, n->page_size);
>>              return -1;
>>          }
>>          n->zasl = 31 - clz32(n->params.zasl_bs / n->page_size);
>> @@ -4503,6 +4505,12 @@ static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
>>              error_setg(errp, "zone append size limit has to be a power of 2");
>>              return;
>>          }
>> +
>> +        if (n->params.zasl_bs < 4096) {
>> +            error_setg(errp, "zone append size limit must be at least "
>> +                       "4096 bytes");
>> +            return;
>> +        }
>>      }
>>  }
> 
> The guest error is less confusing than simply a trace. LGTM.

Trace events are meant for the developers when debugging, they
are usually stripped out in final build.

Errors are reported to the user / operator (i.e. incorrect
configuration).

Regards,

Phil.
Klaus Jensen Feb. 10, 2021, 8:56 p.m. UTC | #5 
On Feb  8 09:25, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The Zone Append Size Limit (ZASL) must be at least 4096 bytes, so
> improve the user experience by adding an early parameter check in
> nvme_check_constraints.
> 
> When ZASL is still too small due to the host configuring the device for
> an even larger page size, convert the trace point in nvme_start_ctrl to
> an NVME_GUEST_ERR such that this is logged by QEMU instead of only
> traced.
> 

Thanks for the review; applied to nvme-next!
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index c2f0c88fbf39..d96888cd2333 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3983,8 +3983,10 @@  static int nvme_start_ctrl(NvmeCtrl *n)
         n->zasl = n->params.mdts;
     } else {
         if (n->params.zasl_bs < n->page_size) {
-            trace_pci_nvme_err_startfail_zasl_too_small(n->params.zasl_bs,
-                                                        n->page_size);
+            NVME_GUEST_ERR(pci_nvme_err_startfail_zasl_too_small,
+                           "Zone Append Size Limit (ZASL) of %d bytes is too "
+                           "small; must be at least %d bytes",
+                           n->params.zasl_bs, n->page_size);
             return -1;
         }
         n->zasl = 31 - clz32(n->params.zasl_bs / n->page_size);
@@ -4503,6 +4505,12 @@  static void nvme_check_constraints(NvmeCtrl *n, Error **errp)
             error_setg(errp, "zone append size limit has to be a power of 2");
             return;
         }
+
+        if (n->params.zasl_bs < 4096) {
+            error_setg(errp, "zone append size limit must be at least "
+                       "4096 bytes");
+            return;
+        }
     }
 }