diff mbox series

[2/2] hw/block/nvme: refactor the logic for zone write checks

Message ID 20210119135500.265403-3-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series hw/block/nvme: zoned fixes | expand

Commit Message

Klaus Jensen Jan. 19, 2021, 1:55 p.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

Refactor the zone write check logic such that the most "meaningful"
error is returned first. That is, first, if the zone is not writable,
return an appropriate status code for that. Then, make sure we are
actually writing at the write pointer and finally check that we do not
cross the zone write boundary. This aligns with the "priority" of status
codes for zone read checks.

Also add a couple of additional descriptive trace events and remove an
always true assert.

Cc: Dmitry Fomichev <dmitry.fomichev@wdc.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c       | 47 +++++++++++++++++++------------------------
 hw/block/trace-events |  5 +++++
 2 files changed, 26 insertions(+), 26 deletions(-)

Comments

Dmitry Fomichev Jan. 26, 2021, 4:58 a.m. UTC | #1 
On Tue, 2021-01-19 at 14:55 +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Refactor the zone write check logic such that the most "meaningful"
> error is returned first. That is, first, if the zone is not writable,
> return an appropriate status code for that. Then, make sure we are
> actually writing at the write pointer and finally check that we do not
> cross the zone write boundary. This aligns with the "priority" of status
> codes for zone read checks.

Right, all boundary checks need to be performed after validating the zone
state and WP checks. 

> 
> Also add a couple of additional descriptive trace events and remove an
> always true assert.

Adding the new trace events looks like a great idea. I think that assert
is useful, but I have no qualms about removing it.

Overall, this looks good to me, except the part about open coding
ZA validation :)

Best regards,
Dmitry

> 
> Cc: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c       | 47 +++++++++++++++++++------------------------
>  hw/block/trace-events |  5 +++++
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index f05dea657b01..193a27259dda 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1106,56 +1106,51 @@ static inline NvmeZone *nvme_get_zone_by_slba(NvmeNamespace *ns, uint64_t slba)
>  
> 
> 
> 
>  static uint16_t nvme_check_zone_state_for_write(NvmeZone *zone)
>  {
> -    uint16_t status;
> +    uint64_t zslba = zone->d.zslba;
>  
> 
> 
> 
>      switch (nvme_get_zone_state(zone)) {
>      case NVME_ZONE_STATE_EMPTY:
>      case NVME_ZONE_STATE_IMPLICITLY_OPEN:
>      case NVME_ZONE_STATE_EXPLICITLY_OPEN:
>      case NVME_ZONE_STATE_CLOSED:
> -        status = NVME_SUCCESS;
> -        break;
> +        return NVME_SUCCESS;
>      case NVME_ZONE_STATE_FULL:
> -        status = NVME_ZONE_FULL;
> -        break;
> +        trace_pci_nvme_err_zone_is_full(zslba);
> +        return NVME_ZONE_FULL;
>      case NVME_ZONE_STATE_OFFLINE:
> -        status = NVME_ZONE_OFFLINE;
> -        break;
> +        trace_pci_nvme_err_zone_is_offline(zslba);
> +        return NVME_ZONE_OFFLINE;
>      case NVME_ZONE_STATE_READ_ONLY:
> -        status = NVME_ZONE_READ_ONLY;
> -        break;
> +        trace_pci_nvme_err_zone_is_read_only(zslba);
> +        return NVME_ZONE_READ_ONLY;
>      default:
>          assert(false);
>      }
> -
> -    return status;
>  }
>  
>  static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
>                                        NvmeZone *zone, uint64_t slba,
>                                        uint32_t nlb)
>  {
> +    uint64_t zcap = nvme_zone_wr_boundary(zone);
>      uint16_t status;
>  
> 
> 
> 
> -    if (unlikely((slba + nlb) > nvme_zone_wr_boundary(zone))) {
> -        status = NVME_ZONE_BOUNDARY_ERROR;
> -    } else {
> -        status = nvme_check_zone_state_for_write(zone);
> +    status = nvme_check_zone_state_for_write(zone);
> +    if (status) {
> +        return status;
>      }
>  
> 
> 
> 
> -    if (status != NVME_SUCCESS) {
> -        trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
> -    } else {
> -        assert(nvme_wp_is_valid(zone));
> -
> -        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;
> -        }
> +    if (unlikely(slba != zone->w_ptr)) {
> +        trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba, zone->w_ptr);
> +        return NVME_ZONE_INVALID_WRITE;
>      }
>  
> 
> 
> 
> -    return status;
> +    if (unlikely((slba + nlb) > zcap)) {
> +        trace_pci_nvme_err_zone_boundary(slba, nlb, zcap);
> +        return NVME_ZONE_BOUNDARY_ERROR;
> +    }
> +
> +    return NVME_SUCCESS;
>  }
>  
> 
>  static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index 78d76b0a71c1..c80b02b85ac9 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -127,6 +127,11 @@ pci_nvme_err_unaligned_zone_cmd(uint8_t action, uint64_t slba, uint64_t zslba) "
>  pci_nvme_err_invalid_zone_state_transition(uint8_t action, uint64_t slba, uint8_t attrs) "action=0x%"PRIx8", slba=%"PRIu64", attrs=0x%"PRIx32""
>  pci_nvme_err_write_not_at_wp(uint64_t slba, uint64_t zone, uint64_t wp) "writing at slba=%"PRIu64", zone=%"PRIu64", but wp=%"PRIu64""
>  pci_nvme_err_append_not_at_start(uint64_t slba, uint64_t zone) "appending at slba=%"PRIu64", but zone=%"PRIu64""
> +pci_nvme_err_zone_is_full(uint64_t zslba) "zslba 0x%"PRIx64""
> +pci_nvme_err_zone_is_read_only(uint64_t zslba) "zslba 0x%"PRIx64""
> +pci_nvme_err_zone_is_offline(uint64_t zslba) "zslba 0x%"PRIx64""
> +pci_nvme_err_zone_boundary(uint64_t slba, uint32_t nlb, uint64_t zcap) "lba 0x%"PRIx64" nlb %"PRIu32" zcap 0x%"PRIx64""
> +pci_nvme_err_zone_invalid_write(uint64_t slba, uint64_t wp) "lba 0x%"PRIx64" wp 0x%"PRIx64""
>  pci_nvme_err_zone_write_not_ok(uint64_t slba, uint32_t nlb, uint32_t status) "slba=%"PRIu64", nlb=%"PRIu32", status=0x%"PRIx16""
>  pci_nvme_err_zone_read_not_ok(uint64_t slba, uint32_t nlb, uint32_t status) "slba=%"PRIu64", nlb=%"PRIu32", status=0x%"PRIx16""
>  pci_nvme_err_append_too_large(uint64_t slba, uint32_t nlb, uint8_t zasl) "slba=%"PRIu64", nlb=%"PRIu32", zasl=%"PRIu8""
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f05dea657b01..193a27259dda 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1106,56 +1106,51 @@  static inline NvmeZone *nvme_get_zone_by_slba(NvmeNamespace *ns, uint64_t slba)
 
 static uint16_t nvme_check_zone_state_for_write(NvmeZone *zone)
 {
-    uint16_t status;
+    uint64_t zslba = zone->d.zslba;
 
     switch (nvme_get_zone_state(zone)) {
     case NVME_ZONE_STATE_EMPTY:
     case NVME_ZONE_STATE_IMPLICITLY_OPEN:
     case NVME_ZONE_STATE_EXPLICITLY_OPEN:
     case NVME_ZONE_STATE_CLOSED:
-        status = NVME_SUCCESS;
-        break;
+        return NVME_SUCCESS;
     case NVME_ZONE_STATE_FULL:
-        status = NVME_ZONE_FULL;
-        break;
+        trace_pci_nvme_err_zone_is_full(zslba);
+        return NVME_ZONE_FULL;
     case NVME_ZONE_STATE_OFFLINE:
-        status = NVME_ZONE_OFFLINE;
-        break;
+        trace_pci_nvme_err_zone_is_offline(zslba);
+        return NVME_ZONE_OFFLINE;
     case NVME_ZONE_STATE_READ_ONLY:
-        status = NVME_ZONE_READ_ONLY;
-        break;
+        trace_pci_nvme_err_zone_is_read_only(zslba);
+        return NVME_ZONE_READ_ONLY;
     default:
         assert(false);
     }
-
-    return status;
 }
 
 static uint16_t nvme_check_zone_write(NvmeCtrl *n, NvmeNamespace *ns,
                                       NvmeZone *zone, uint64_t slba,
                                       uint32_t nlb)
 {
+    uint64_t zcap = nvme_zone_wr_boundary(zone);
     uint16_t status;
 
-    if (unlikely((slba + nlb) > nvme_zone_wr_boundary(zone))) {
-        status = NVME_ZONE_BOUNDARY_ERROR;
-    } else {
-        status = nvme_check_zone_state_for_write(zone);
+    status = nvme_check_zone_state_for_write(zone);
+    if (status) {
+        return status;
     }
 
-    if (status != NVME_SUCCESS) {
-        trace_pci_nvme_err_zone_write_not_ok(slba, nlb, status);
-    } else {
-        assert(nvme_wp_is_valid(zone));
-
-        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;
-        }
+    if (unlikely(slba != zone->w_ptr)) {
+        trace_pci_nvme_err_write_not_at_wp(slba, zone->d.zslba, zone->w_ptr);
+        return NVME_ZONE_INVALID_WRITE;
     }
 
-    return status;
+    if (unlikely((slba + nlb) > zcap)) {
+        trace_pci_nvme_err_zone_boundary(slba, nlb, zcap);
+        return NVME_ZONE_BOUNDARY_ERROR;
+    }
+
+    return NVME_SUCCESS;
 }
 
 static uint16_t nvme_check_zone_state_for_read(NvmeZone *zone)
diff --git a/hw/block/trace-events b/hw/block/trace-events
index 78d76b0a71c1..c80b02b85ac9 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -127,6 +127,11 @@  pci_nvme_err_unaligned_zone_cmd(uint8_t action, uint64_t slba, uint64_t zslba) "
 pci_nvme_err_invalid_zone_state_transition(uint8_t action, uint64_t slba, uint8_t attrs) "action=0x%"PRIx8", slba=%"PRIu64", attrs=0x%"PRIx32""
 pci_nvme_err_write_not_at_wp(uint64_t slba, uint64_t zone, uint64_t wp) "writing at slba=%"PRIu64", zone=%"PRIu64", but wp=%"PRIu64""
 pci_nvme_err_append_not_at_start(uint64_t slba, uint64_t zone) "appending at slba=%"PRIu64", but zone=%"PRIu64""
+pci_nvme_err_zone_is_full(uint64_t zslba) "zslba 0x%"PRIx64""
+pci_nvme_err_zone_is_read_only(uint64_t zslba) "zslba 0x%"PRIx64""
+pci_nvme_err_zone_is_offline(uint64_t zslba) "zslba 0x%"PRIx64""
+pci_nvme_err_zone_boundary(uint64_t slba, uint32_t nlb, uint64_t zcap) "lba 0x%"PRIx64" nlb %"PRIu32" zcap 0x%"PRIx64""
+pci_nvme_err_zone_invalid_write(uint64_t slba, uint64_t wp) "lba 0x%"PRIx64" wp 0x%"PRIx64""
 pci_nvme_err_zone_write_not_ok(uint64_t slba, uint32_t nlb, uint32_t status) "slba=%"PRIu64", nlb=%"PRIu32", status=0x%"PRIx16""
 pci_nvme_err_zone_read_not_ok(uint64_t slba, uint32_t nlb, uint32_t status) "slba=%"PRIu64", nlb=%"PRIu32", status=0x%"PRIx16""
 pci_nvme_err_append_too_large(uint64_t slba, uint32_t nlb, uint8_t zasl) "slba=%"PRIu64", nlb=%"PRIu32", zasl=%"PRIu8""