On Tue, 2021-01-19 at 14:55 +0100, Klaus Jensen wrote:
> From: Klaus Jensen <k.jen...@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.fomic...@wdc.com>
> Signed-off-by: Klaus Jensen <k.jen...@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""
