diff mbox series

[2/3] hw/block/nvme: Check zone state before checking boundaries

Message ID 20210126050248.9077-3-dmitry.fomichev@wdc.com (mailing list archive)
State New, archived
Headers show
Series Fix zone write validation | expand

Commit Message

Dmitry Fomichev Jan. 26, 2021, 5:02 a.m. UTC
The code in nvme_check_zone_write() first checks for zone boundary
condition violation and only after that it proceeds to verify that the
zone state is suitable the write to happen. This is not consistent with
nvme_check_zone_read() flow - in that function, zone state is checked
before making any boundary checks. Besides, checking that ZSLBA + NLB
does not cross the write boundary is now redundant for Zone Append and
only needs to be done for writes.

Move the check in the code to be performed for Write and Write Zeroes
commands only and to occur after zone state checks.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 hw/block/nvme.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Klaus Jensen Jan. 26, 2021, 7:54 a.m. UTC | #1
On Jan 26 14:02, Dmitry Fomichev wrote:
> The code in nvme_check_zone_write() first checks for zone boundary
> condition violation and only after that it proceeds to verify that the
> zone state is suitable the write to happen. This is not consistent with
> nvme_check_zone_read() flow - in that function, zone state is checked
> before making any boundary checks. Besides, checking that ZSLBA + NLB
> does not cross the write boundary is now redundant for Zone Append and
> only needs to be done for writes.
> 
> Move the check in the code to be performed for Write and Write Zeroes
> commands only and to occur after zone state checks.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  hw/block/nvme.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 67538010ef..b712634c27 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1138,13 +1138,8 @@ static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
>      uint64_t bndry = nvme_zone_wr_boundary(zone);
>      uint16_t status;
>  
> -    if (unlikely(slba + nlb > bndry)) {
> -        status = NVME_ZONE_BOUNDARY_ERROR;
> -    } else {
> -        status = nvme_check_zone_state_for_write(zone);
> -    }

Alright. The double check on boundary that I pointed out in the previous
patch is fixed here.

> -
> -    if (status != NVME_SUCCESS) {
> +    status = nvme_check_zone_state_for_write(zone);
> +    if (status) {
>          trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
>      } else {
>          assert(nvme_wp_is_valid(zone));
> @@ -1158,10 +1153,14 @@ static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
>                  trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
>                  status = NVME_INVALID_FIELD;
>              }
> -        } else if (unlikely(slba != zone->w_ptr)) {
> -            trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
> -                                               zone->w_ptr);
> -            status = NVME_ZONE_INVALID_WRITE;
> +        } else {
> +            if (unlikely(slba != zone->w_ptr)) {
> +                trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
> +                                                   zone->w_ptr);
> +                status = NVME_ZONE_INVALID_WRITE;
> +            } else if (unlikely(slba + nlb > bndry)) {
> +                status = NVME_ZONE_BOUNDARY_ERROR;
> +            }
>          }
>      }
>  
> -- 
> 2.28.0
> 
>
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 67538010ef..b712634c27 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1138,13 +1138,8 @@  static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
     uint64_t bndry = nvme_zone_wr_boundary(zone);
     uint16_t status;
 
-    if (unlikely(slba + nlb > bndry)) {
-        status = NVME_ZONE_BOUNDARY_ERROR;
-    } else {
-        status = nvme_check_zone_state_for_write(zone);
-    }
-
-    if (status != NVME_SUCCESS) {
+    status = nvme_check_zone_state_for_write(zone);
+    if (status) {
         trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
     } else {
         assert(nvme_wp_is_valid(zone));
@@ -1158,10 +1153,14 @@  static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
                 trace_pci_nvme_err_append_too_large(slba, nlb, n->zasl);
                 status = NVME_INVALID_FIELD;
             }
-        } else if (unlikely(slba != zone->w_ptr)) {
-            trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
-                                               zone->w_ptr);
-            status = NVME_ZONE_INVALID_WRITE;
+        } else {
+            if (unlikely(slba != zone->w_ptr)) {
+                trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba,
+                                                   zone->w_ptr);
+                status = NVME_ZONE_INVALID_WRITE;
+            } else if (unlikely(slba + nlb > bndry)) {
+                status = NVME_ZONE_BOUNDARY_ERROR;
+            }
         }
     }