Hi Dmitry,
On 10/30/20 3:32 AM, Dmitry Fomichev wrote:
> The emulation code has been changed to advertise NVM Command Set when
> "zoned" device property is not set (default) and Zoned Namespace
> Command Set otherwise.
>
> Define values and structures that are needed to support Zoned
> Namespace Command Set (NVMe TP 4053) in PCI NVMe controller emulator.
> Define trace events where needed in newly introduced code.
>
> In order to improve scalability, all open, closed and full zones
> are organized in separate linked lists. Consequently, almost all
> zone operations don't require scanning of the entire zone array
> (which potentially can be quite large) - it is only necessary to
> enumerate one or more zone lists.
>
> Handlers for three new NVMe commands introduced in Zoned Namespace
> Command Set specification are added, namely for Zone Management
> Receive, Zone Management Send and Zone Append.
>
> Device initialization code has been extended to create a proper
> configuration for zoned operation using device properties.
>
> Read/Write command handler is modified to only allow writes at the
> write pointer if the namespace is zoned. For Zone Append command,
> writes implicitly happen at the write pointer and the starting write
> pointer value is returned as the result of the command. Write Zeroes
> handler is modified to add zoned checks that are identical to those
> done as a part of Write flow.
>
> Subsequent commits in this series add ZDE support and checks for
> active and open zone limits.
>
> Signed-off-by: Niklas Cassel <niklas.cas...@wdc.com>
> Signed-off-by: Hans Holmberg <hans.holmb...@wdc.com>
> Signed-off-by: Ajay Joshi <ajay.jo...@wdc.com>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulka...@wdc.com>
> Signed-off-by: Matias Bjorling <matias.bjorl...@wdc.com>
> Signed-off-by: Aravind Ramesh <aravind.ram...@wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawas...@wdc.com>
> Signed-off-by: Adam Manzanares <adam.manzana...@wdc.com>
> Signed-off-by: Dmitry Fomichev <dmitry.fomic...@wdc.com>
> Reviewed-by: Niklas Cassel <niklas.cas...@wdc.com>
> ---
> block/nvme.c | 2 +-
> hw/block/nvme-ns.c | 173 ++++++++
> hw/block/nvme-ns.h | 54 +++
> hw/block/nvme.c | 977 +++++++++++++++++++++++++++++++++++++++++-
> hw/block/nvme.h | 8 +
> hw/block/trace-events | 18 +-
> include/block/nvme.h | 113 ++++-
When you start modifying include/ files, it is recommended
to start using scripts/git.orderfile as this makes review
easier (no need to scroll back / up constantly).
As "block/nvme.h" is shared by 2 subsystems, keeping its
changes in a separate patch is preferred.
> 7 files changed, 1322 insertions(+), 23 deletions(-)
>
> diff --git a/block/nvme.c b/block/nvme.c
> index 05485fdd11..7a513c9a17 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -333,7 +333,7 @@ static inline int nvme_translate_error(const NvmeCqe *c)
> {
> uint16_t status = (le16_to_cpu(c->status) >> 1) & 0xFF;
> if (status) {
> - trace_nvme_error(le32_to_cpu(c->result),
> + trace_nvme_error(le32_to_cpu(c->result32),
> le16_to_cpu(c->sq_head),
> le16_to_cpu(c->sq_id),
> le16_to_cpu(c->cid),
...
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 3653b4aefc..ba8a45edf5 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -489,6 +489,9 @@ enum NvmeIoCommands {
> NVME_CMD_COMPARE = 0x05,
> NVME_CMD_WRITE_ZEROES = 0x08,
> NVME_CMD_DSM = 0x09,
> + NVME_CMD_ZONE_MGMT_SEND = 0x79,
> + NVME_CMD_ZONE_MGMT_RECV = 0x7a,
> + NVME_CMD_ZONE_APPEND = 0x7d,
> };
>
> typedef struct QEMU_PACKED NvmeDeleteQ {
> @@ -649,8 +652,10 @@ typedef struct QEMU_PACKED NvmeAerResult {
> } NvmeAerResult;
>
> typedef struct QEMU_PACKED NvmeCqe {
> - uint32_t result;
> - uint32_t rsvd;
> + union {
> + uint64_t result64;
> + uint32_t result32;
> + };
When using packed structure you want to define all fields to
avoid alignment confusion (and I'm surprised the compiler doesn't
complain...). So this would be:
union {
uint64_t result64;
struct {
uint32_t result32;
uint32_t rsvd32;
};
};
But since the ZNS is still a technical proposal and not in the spec,
this doesn't look correct (the spec list this field as 32-bit).
What do you think about adding NvmeCqeZNS?
Maybe:
typedef struct QEMU_PACKED NvmeCqeZNS {
uint64_t result;
uint16_t sq_head;
uint16_t sq_id;
uint16_t cid;
uint16_t status;
} NvmeCqeZNS;
Or clever:
typedef union QEMU_PACKED NvmeCqeZNS {
union {
struct {
uint64_t result;
uint32_t dw2;
uint32_t dw3;
};
NvmeCqe cqe;
};
} NvmeCqeZNS;
I wonder what part could go in hw/block/nvme-ns.h or "block/nvme-zns.h".
> uint16_t sq_head;
> uint16_t sq_id;
> uint16_t cid;
> @@ -678,6 +683,7 @@ enum NvmeStatusCodes {
> NVME_SGL_DESCR_TYPE_INVALID = 0x0011,
> NVME_INVALID_USE_OF_CMB = 0x0012,
> NVME_CMD_SET_CMB_REJECTED = 0x002b,
> + NVME_INVALID_CMD_SET = 0x002c,
> NVME_LBA_RANGE = 0x0080,
> NVME_CAP_EXCEEDED = 0x0081,
> NVME_NS_NOT_READY = 0x0082,
> @@ -702,6 +708,14 @@ enum NvmeStatusCodes {
> NVME_CONFLICTING_ATTRS = 0x0180,
> NVME_INVALID_PROT_INFO = 0x0181,
> NVME_WRITE_TO_RO = 0x0182,
> + NVME_ZONE_BOUNDARY_ERROR = 0x01b8,
> + NVME_ZONE_FULL = 0x01b9,
> + NVME_ZONE_READ_ONLY = 0x01ba,
> + NVME_ZONE_OFFLINE = 0x01bb,
> + NVME_ZONE_INVALID_WRITE = 0x01bc,
> + NVME_ZONE_TOO_MANY_ACTIVE = 0x01bd,
> + NVME_ZONE_TOO_MANY_OPEN = 0x01be,
> + NVME_ZONE_INVAL_TRANSITION = 0x01bf,
> NVME_WRITE_FAULT = 0x0280,
> NVME_UNRECOVERED_READ = 0x0281,
> NVME_E2E_GUARD_ERROR = 0x0282,
> @@ -886,6 +900,11 @@ typedef struct QEMU_PACKED NvmeIdCtrl {
> uint8_t vs[1024];
> } NvmeIdCtrl;
>
> +typedef struct NvmeIdCtrlZoned {
> + uint8_t zasl;
> + uint8_t rsvd1[4095];
> +} NvmeIdCtrlZoned;
...
