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

[Dmitry Fomichev]

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