On Feb 12 13:44, Maxim Levitsky wrote:
> On Tue, 2020-02-04 at 10:51 +0100, Klaus Jensen wrote:
> > Refactor nvme_map_prp and allow PRPs to be located in the CMB. The logic
> > ensures that if some of the PRP is in the CMB, all of it must be located
> > there, as per the specification.
>
> To be honest this looks like not refactoring but a bugfix
> (old code was just assuming that if first prp entry is in cmb, the rest also
> is)
I split it up into a separate bugfix patch.
> >
> > Also combine nvme_dma_{read,write}_prp into a single nvme_dma_prp that
> > takes an additional DMADirection parameter.
>
> To be honest 'nvme_dma_prp' was not a clear function name to me at first
> glance.
> Could you rename this to nvme_dma_prp_rw or so? (Although even that is
> somewhat unclear
> to convey the meaning of read/write the data to/from the guest memory areas
> defined by the prp list.
> Also could you split this change into a new patch?
>
Splitting into new patch.
> >
> > Signed-off-by: Klaus Jensen <klaus.jen...@cnexlabs.com>
> > Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> Now you even use your both addresses :-)
>
> > ---
> > hw/block/nvme.c | 245 +++++++++++++++++++++++++++---------------
> > hw/block/nvme.h | 2 +-
> > hw/block/trace-events | 1 +
> > include/block/nvme.h | 1 +
> > 4 files changed, 160 insertions(+), 89 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 4acfc85b56a2..334265efb21e 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -58,6 +58,11 @@
> >
> > static void nvme_process_sq(void *opaque);
> >
> > +static inline void *nvme_addr_to_cmb(NvmeCtrl *n, hwaddr addr)
> > +{
> > + return &n->cmbuf[addr - n->ctrl_mem.addr];
> > +}
>
> To my taste I would put this together with the patch that
> added nvme_addr_is_cmb. I know that some people are against
> this citing the fact that you should use the code you add
> in the same patch. Your call.
>
> Regardless of this I also prefer to put refactoring patches first in the
> series.
>
> > +
> > static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> > {
> > hwaddr low = n->ctrl_mem.addr;
> > @@ -152,138 +157,187 @@ static void nvme_irq_deassert(NvmeCtrl *n,
> > NvmeCQueue *cq)
> > }
> > }
> >
> > -static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t
> > prp1,
> > - uint64_t prp2, uint32_t len, NvmeCtrl *n)
> > +static uint16_t nvme_map_prp(NvmeCtrl *n, QEMUSGList *qsg, QEMUIOVector
> > *iov,
> > + uint64_t prp1, uint64_t prp2, uint32_t len, NvmeRequest *req)
>
> Split line alignment (it was correct before).
> Also while at the refactoring, it would be great to add some documentation
> to this and few more functions, since its not clear immediately what this
> does.
>
>
> > {
> > hwaddr trans_len = n->page_size - (prp1 % n->page_size);
> > trans_len = MIN(len, trans_len);
> > int num_prps = (len >> n->page_bits) + 1;
> > + uint16_t status = NVME_SUCCESS;
> > + bool is_cmb = false;
> > + bool prp_list_in_cmb = false;
> > +
> > + trace_nvme_dev_map_prp(nvme_cid(req), req->cmd.opcode, trans_len, len,
> > + prp1, prp2, num_prps);
> >
> > if (unlikely(!prp1)) {
> > trace_nvme_dev_err_invalid_prp();
> > return NVME_INVALID_FIELD | NVME_DNR;
> > - } else if (n->cmbsz && prp1 >= n->ctrl_mem.addr &&
> > - prp1 < n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)) {
> > - qsg->nsg = 0;
> > + }
> > +
> > + if (nvme_addr_is_cmb(n, prp1)) {
> > + is_cmb = true;
> > +
> > qemu_iovec_init(iov, num_prps);
> > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp1 - n->ctrl_mem.addr],
> > trans_len);
> > +
> > + /*
> > + * PRPs do not cross page boundaries, so if the start address
> > (here,
> > + * prp1) is within the CMB, it cannot cross outside the controller
> > + * memory buffer range. This is ensured by
> > + *
> > + * len = n->page_size - (addr % n->page_size)
> > + *
> > + * Thus, we can directly add to the iovec without risking an out of
> > + * bounds access. This also holds for the remaining qemu_iovec_add
> > + * calls.
> > + */
> > + qemu_iovec_add(iov, nvme_addr_to_cmb(n, prp1), trans_len);
> > } else {
> > pci_dma_sglist_init(qsg, &n->parent_obj, num_prps);
> > qemu_sglist_add(qsg, prp1, trans_len);
> > }
> > +
> > len -= trans_len;
> > if (len) {
> > if (unlikely(!prp2)) {
> > trace_nvme_dev_err_invalid_prp2_missing();
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > goto unmap;
> > }
> > +
> > if (len > n->page_size) {
> > uint64_t prp_list[n->max_prp_ents];
> > uint32_t nents, prp_trans;
> > int i = 0;
> >
> > + if (nvme_addr_is_cmb(n, prp2)) {
> > + prp_list_in_cmb = true;
> > + }
> > +
> > nents = (len + n->page_size - 1) >> n->page_bits;
> > prp_trans = MIN(n->max_prp_ents, nents) * sizeof(uint64_t);
> > - nvme_addr_read(n, prp2, (void *)prp_list, prp_trans);
> > + nvme_addr_read(n, prp2, (void *) prp_list, prp_trans);
> > while (len != 0) {
> > uint64_t prp_ent = le64_to_cpu(prp_list[i]);
> >
> > if (i == n->max_prp_ents - 1 && len > n->page_size) {
> > if (unlikely(!prp_ent || prp_ent & (n->page_size -
> > 1))) {
> > trace_nvme_dev_err_invalid_prplist_ent(prp_ent);
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > + goto unmap;
> > + }
> > +
> > + if (prp_list_in_cmb != nvme_addr_is_cmb(n, prp_ent)) {
> > + status = NVME_INVALID_USE_OF_CMB | NVME_DNR;
> > goto unmap;
> > }
> >
> > i = 0;
> > nents = (len + n->page_size - 1) >> n->page_bits;
> > prp_trans = MIN(n->max_prp_ents, nents) *
> > sizeof(uint64_t);
> > - nvme_addr_read(n, prp_ent, (void *)prp_list,
> > - prp_trans);
> > + nvme_addr_read(n, prp_ent, (void *) prp_list,
> > prp_trans);
> > prp_ent = le64_to_cpu(prp_list[i]);
> > }
> >
> > if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
> > trace_nvme_dev_err_invalid_prplist_ent(prp_ent);
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > + goto unmap;
> > + }
> > +
> > + if (is_cmb != nvme_addr_is_cmb(n, prp_ent)) {
> > + status = NVME_INVALID_USE_OF_CMB | NVME_DNR;
> > goto unmap;
> > }
> >
> > trans_len = MIN(len, n->page_size);
> > - if (qsg->nsg){
> > - qemu_sglist_add(qsg, prp_ent, trans_len);
> > + if (is_cmb) {
> > + qemu_iovec_add(iov, nvme_addr_to_cmb(n, prp_ent),
> > + trans_len);
> > } else {
> > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp_ent -
> > n->ctrl_mem.addr], trans_len);
> > + qemu_sglist_add(qsg, prp_ent, trans_len);
> > }
> > +
> > len -= trans_len;
> > i++;
> > }
> > } else {
> > + if (is_cmb != nvme_addr_is_cmb(n, prp2)) {
> > + status = NVME_INVALID_USE_OF_CMB | NVME_DNR;
> > + goto unmap;
> > + }
> > +
> > if (unlikely(prp2 & (n->page_size - 1))) {
> > trace_nvme_dev_err_invalid_prp2_align(prp2);
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > goto unmap;
> > }
> > - if (qsg->nsg) {
> > +
> > + if (is_cmb) {
> > + qemu_iovec_add(iov, nvme_addr_to_cmb(n, prp2), len);
> > + } else {
> > qemu_sglist_add(qsg, prp2, len);
> > - } else {
> > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp2 -
> > n->ctrl_mem.addr], trans_len);
> > }
> > }
> > }
> > +
> > return NVME_SUCCESS;
> >
> > - unmap:
> > - qemu_sglist_destroy(qsg);
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > -}
>
> I haven't checked the new nvme_map_prp to the extent that I am sure that
> it is correct, but it looks reasonable.
>
> > -
> > -static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > - uint64_t prp1, uint64_t prp2)
> > -{
> > - QEMUSGList qsg;
> > - QEMUIOVector iov;
> > - uint16_t status = NVME_SUCCESS;
> > -
> > - if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > - }
> > - if (qsg.nsg > 0) {
> > - if (dma_buf_write(ptr, len, &qsg)) {
> > - status = NVME_INVALID_FIELD | NVME_DNR;
> > - }
> > - qemu_sglist_destroy(&qsg);
> > +unmap:
> > + if (is_cmb) {
> > + qemu_iovec_destroy(iov);
> > } else {
> > - if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
> > - status = NVME_INVALID_FIELD | NVME_DNR;
> > - }
> > - qemu_iovec_destroy(&iov);
> > + qemu_sglist_destroy(qsg);
> > }
> > +
> > return status;
> > }
> >
> > -static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > - uint64_t prp1, uint64_t prp2)
> > +static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > + uint64_t prp1, uint64_t prp2, DMADirection dir, NvmeRequest *req)
> > {
> > QEMUSGList qsg;
> > QEMUIOVector iov;
> > uint16_t status = NVME_SUCCESS;
> > + size_t bytes;
> >
> > - trace_nvme_dev_dma_read(prp1, prp2);
> > -
> > - if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > + status = nvme_map_prp(n, &qsg, &iov, prp1, prp2, len, req);
> > + if (status) {
> > + return status;
> > }
> > +
> > if (qsg.nsg > 0) {
> > - if (unlikely(dma_buf_read(ptr, len, &qsg))) {
> > + uint64_t residual;
> > +
> > + if (dir == DMA_DIRECTION_TO_DEVICE) {
> > + residual = dma_buf_write(ptr, len, &qsg);
> > + } else {
> > + residual = dma_buf_read(ptr, len, &qsg);
> > + }
> > +
> > + if (unlikely(residual)) {
> > trace_nvme_dev_err_invalid_dma();
> > status = NVME_INVALID_FIELD | NVME_DNR;
> > }
> > +
> > qemu_sglist_destroy(&qsg);
> > +
> > + return status;
>
> I would prefer if/else here rather than that early return here.
> It would make code more symmetric.
>
Looks nicer yeah. Done.
> > + }
> > +
> > + if (dir == DMA_DIRECTION_TO_DEVICE) {
> > + bytes = qemu_iovec_to_buf(&iov, 0, ptr, len);
> > } else {
> > - if (unlikely(qemu_iovec_from_buf(&iov, 0, ptr, len) != len)) {
> > - trace_nvme_dev_err_invalid_dma();
> > - status = NVME_INVALID_FIELD | NVME_DNR;
> > - }
> > - qemu_iovec_destroy(&iov);
> > + bytes = qemu_iovec_from_buf(&iov, 0, ptr, len);
> > }
> > +
> > + if (unlikely(bytes != len)) {
> > + trace_nvme_dev_err_invalid_dma();
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > + }
> > +
> > + qemu_iovec_destroy(&iov);
> > +
> > return status;
> > }
> >
> > @@ -420,16 +474,20 @@ static void nvme_rw_cb(void *opaque, int ret)
> > block_acct_failed(blk_get_stats(n->conf.blk), &req->acct);
> > req->status = NVME_INTERNAL_DEV_ERROR;
> > }
> > - if (req->has_sg) {
> > +
> > + if (req->qsg.nalloc) {
> > qemu_sglist_destroy(&req->qsg);
> > }
> > + if (req->iov.nalloc) {
> > + qemu_iovec_destroy(&req->iov);
> > + }
> > +
> > nvme_enqueue_req_completion(cq, req);
> > }
> >
> > static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
> > NvmeRequest *req)
> > {
> > - req->has_sg = false;
> > block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
> > BLOCK_ACCT_FLUSH);
> > req->aiocb = blk_aio_flush(n->conf.blk, nvme_rw_cb, req);
> > @@ -453,7 +511,6 @@ static uint16_t nvme_write_zeros(NvmeCtrl *n,
> > NvmeNamespace *ns, NvmeCmd *cmd,
> > return NVME_LBA_RANGE | NVME_DNR;
> > }
> >
> > - req->has_sg = false;
> > block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
> > BLOCK_ACCT_WRITE);
> > req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, offset, count,
> > @@ -485,21 +542,24 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace
> > *ns, NvmeCmd *cmd,
> > return NVME_LBA_RANGE | NVME_DNR;
> > }
> >
> > - if (nvme_map_prp(&req->qsg, &req->iov, prp1, prp2, data_size, n)) {
> > + if (nvme_map_prp(n, &req->qsg, &req->iov, prp1, prp2, data_size, req))
> > {
> > block_acct_invalid(blk_get_stats(n->conf.blk), acct);
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > - dma_acct_start(n->conf.blk, &req->acct, &req->qsg, acct);
> > if (req->qsg.nsg > 0) {
> > - req->has_sg = true;
> > + block_acct_start(blk_get_stats(n->conf.blk), &req->acct,
> > req->qsg.size,
> > + acct);
> > +
> > req->aiocb = is_write ?
> > dma_blk_write(n->conf.blk, &req->qsg, data_offset,
> > BDRV_SECTOR_SIZE,
> > nvme_rw_cb, req) :
> > dma_blk_read(n->conf.blk, &req->qsg, data_offset,
> > BDRV_SECTOR_SIZE,
> > nvme_rw_cb, req);
> > } else {
> > - req->has_sg = false;
> > + block_acct_start(blk_get_stats(n->conf.blk), &req->acct,
> > req->iov.size,
> > + acct);
> > +
> > req->aiocb = is_write ?
> > blk_aio_pwritev(n->conf.blk, data_offset, &req->iov, 0,
> > nvme_rw_cb,
> > req) :
> > @@ -596,7 +656,7 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n,
> > uint64_t dma_addr,
> > sq->size = size;
> > sq->cqid = cqid;
> > sq->head = sq->tail = 0;
> > - sq->io_req = g_new(NvmeRequest, sq->size);
> > + sq->io_req = g_new0(NvmeRequest, sq->size);
> >
> > QTAILQ_INIT(&sq->req_list);
> > QTAILQ_INIT(&sq->out_req_list);
> > @@ -704,8 +764,8 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd
> > *cmd, uint8_t rae,
> > nvme_clear_events(n, NVME_AER_TYPE_SMART);
> > }
> >
> > - return nvme_dma_read_prp(n, (uint8_t *) &smart + off, trans_len, prp1,
> > - prp2);
> > + return nvme_dma_prp(n, (uint8_t *) &smart + off, trans_len, prp1,
> > + prp2, DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t
> > buf_len,
> > @@ -724,8 +784,8 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd
> > *cmd, uint32_t buf_len,
> >
> > trans_len = MIN(sizeof(fw_log) - off, buf_len);
> >
> > - return nvme_dma_read_prp(n, (uint8_t *) &fw_log + off, trans_len, prp1,
> > - prp2);
> > + return nvme_dma_prp(n, (uint8_t *) &fw_log + off, trans_len, prp1,
> > + prp2, DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > static uint16_t nvme_get_log(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > @@ -869,18 +929,20 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd
> > *cmd)
> > return NVME_SUCCESS;
> > }
> >
> > -static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeIdentify *c)
> > +static uint16_t nvme_identify_ctrl(NvmeCtrl *n, NvmeIdentify *c,
> > + NvmeRequest *req)
> > {
> > uint64_t prp1 = le64_to_cpu(c->prp1);
> > uint64_t prp2 = le64_to_cpu(c->prp2);
> >
> > trace_nvme_dev_identify_ctrl();
> >
> > - return nvme_dma_read_prp(n, (uint8_t *)&n->id_ctrl, sizeof(n->id_ctrl),
> > - prp1, prp2);
> > + return nvme_dma_prp(n, (uint8_t *)&n->id_ctrl, sizeof(n->id_ctrl),
> > + prp1, prp2, DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > -static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeIdentify *c)
> > +static uint16_t nvme_identify_ns(NvmeCtrl *n, NvmeIdentify *c,
> > + NvmeRequest *req)
> > {
> > NvmeNamespace *ns;
> > uint32_t nsid = le32_to_cpu(c->nsid);
> > @@ -896,11 +958,12 @@ static uint16_t nvme_identify_ns(NvmeCtrl *n,
> > NvmeIdentify *c)
> >
> > ns = &n->namespaces[nsid - 1];
> >
> > - return nvme_dma_read_prp(n, (uint8_t *)&ns->id_ns, sizeof(ns->id_ns),
> > - prp1, prp2);
> > + return nvme_dma_prp(n, (uint8_t *)&ns->id_ns, sizeof(ns->id_ns),
> > + prp1, prp2, DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > -static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeIdentify *c)
> > +static uint16_t nvme_identify_ns_list(NvmeCtrl *n, NvmeIdentify *c,
> > + NvmeRequest *req)
> > {
> > static const int data_len = 4 * KiB;
> > uint32_t min_nsid = le32_to_cpu(c->nsid);
> > @@ -922,12 +985,14 @@ static uint16_t nvme_identify_ns_list(NvmeCtrl *n,
> > NvmeIdentify *c)
> > break;
> > }
> > }
> > - ret = nvme_dma_read_prp(n, (uint8_t *)list, data_len, prp1, prp2);
> > + ret = nvme_dma_prp(n, (uint8_t *)list, data_len, prp1, prp2,
> > + DMA_DIRECTION_FROM_DEVICE, req);
> > g_free(list);
> > return ret;
> > }
> >
> > -static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeCmd *c)
> > +static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeIdentify *c,
> > + NvmeRequest *req)
> > {
> > static const int len = 4096;
> >
> > @@ -963,24 +1028,25 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl
> > *n, NvmeCmd *c)
> > list->nidl = 0x10;
> > *(uint32_t *) &list->nid[12] = cpu_to_be32(nsid);
> >
> > - ret = nvme_dma_read_prp(n, (uint8_t *) list, len, prp1, prp2);
> > + ret = nvme_dma_prp(n, (uint8_t *) list, len, prp1, prp2,
> > + DMA_DIRECTION_FROM_DEVICE, req);
> > g_free(list);
> > return ret;
> > }
> >
> > -static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
> > +static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> > {
> > NvmeIdentify *c = (NvmeIdentify *)cmd;
> >
> > switch (le32_to_cpu(c->cns)) {
> > case 0x00:
> > - return nvme_identify_ns(n, c);
> > + return nvme_identify_ns(n, c, req);
> > case 0x01:
> > - return nvme_identify_ctrl(n, c);
> > + return nvme_identify_ctrl(n, c, req);
> > case 0x02:
> > - return nvme_identify_ns_list(n, c);
> > + return nvme_identify_ns_list(n, c, req);
> > case 0x03:
> > - return nvme_identify_ns_descr_list(n, cmd);
> > + return nvme_identify_ns_descr_list(n, c, req);
> > default:
> > trace_nvme_dev_err_invalid_identify_cns(le32_to_cpu(c->cns));
> > return NVME_INVALID_FIELD | NVME_DNR;
> > @@ -1039,15 +1105,16 @@ static inline uint64_t nvme_get_timestamp(const
> > NvmeCtrl *n)
> > return cpu_to_le64(ts.all);
> > }
> >
> > -static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
> > +static uint16_t nvme_get_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd,
> > + NvmeRequest *req)
> > {
> > uint64_t prp1 = le64_to_cpu(cmd->prp1);
> > uint64_t prp2 = le64_to_cpu(cmd->prp2);
> >
> > uint64_t timestamp = nvme_get_timestamp(n);
> >
> > - return nvme_dma_read_prp(n, (uint8_t *)×tamp,
> > - sizeof(timestamp), prp1, prp2);
> > + return nvme_dma_prp(n, (uint8_t *)×tamp, sizeof(timestamp),
> > + prp1, prp2, DMA_DIRECTION_FROM_DEVICE, req);
> > }
> >
> > static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest
> > *req)
> > @@ -1099,7 +1166,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > trace_nvme_dev_getfeat_numq(result);
> > break;
> > case NVME_TIMESTAMP:
> > - return nvme_get_feature_timestamp(n, cmd);
> > + return nvme_get_feature_timestamp(n, cmd, req);
> > case NVME_INTERRUPT_COALESCING:
> > result = cpu_to_le32(n->features.int_coalescing);
> > break;
> > @@ -1125,15 +1192,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > return NVME_SUCCESS;
> > }
> >
> > -static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd)
> > +static uint16_t nvme_set_feature_timestamp(NvmeCtrl *n, NvmeCmd *cmd,
> > + NvmeRequest *req)
> > {
> > uint16_t ret;
> > uint64_t timestamp;
> > uint64_t prp1 = le64_to_cpu(cmd->prp1);
> > uint64_t prp2 = le64_to_cpu(cmd->prp2);
> >
> > - ret = nvme_dma_write_prp(n, (uint8_t *)×tamp,
> > - sizeof(timestamp), prp1, prp2);
> > + ret = nvme_dma_prp(n, (uint8_t *) ×tamp, sizeof(timestamp),
> > + prp1, prp2, DMA_DIRECTION_TO_DEVICE, req);
> > if (ret != NVME_SUCCESS) {
> > return ret;
> > }
> > @@ -1194,7 +1262,7 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > ((n->params.num_queues - 2) << 16));
> > break;
> > case NVME_TIMESTAMP:
> > - return nvme_set_feature_timestamp(n, cmd);
> > + return nvme_set_feature_timestamp(n, cmd, req);
> > case NVME_ASYNCHRONOUS_EVENT_CONF:
> > n->features.async_config = dw11;
> > break;
> > @@ -1246,7 +1314,7 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeCmd
> > *cmd, NvmeRequest *req)
> > case NVME_ADM_CMD_CREATE_CQ:
> > return nvme_create_cq(n, cmd);
> > case NVME_ADM_CMD_IDENTIFY:
> > - return nvme_identify(n, cmd);
> > + return nvme_identify(n, cmd, req);
> > case NVME_ADM_CMD_ABORT:
> > return nvme_abort(n, cmd, req);
> > case NVME_ADM_CMD_SET_FEATURES:
> > @@ -1282,6 +1350,7 @@ static void nvme_process_sq(void *opaque)
> > QTAILQ_INSERT_TAIL(&sq->out_req_list, req, entry);
> > memset(&req->cqe, 0, sizeof(req->cqe));
> > req->cqe.cid = cmd.cid;
> > + memcpy(&req->cmd, &cmd, sizeof(NvmeCmd));
> >
> > status = sq->sqid ? nvme_io_cmd(n, &cmd, req) :
> > nvme_admin_cmd(n, &cmd, req);
> > @@ -1804,7 +1873,7 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice
> > *pci_dev)
> >
> > NVME_CMBSZ_SET_SQS(n->bar.cmbsz, 1);
> > NVME_CMBSZ_SET_CQS(n->bar.cmbsz, 0);
> > - NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 0);
> > + NVME_CMBSZ_SET_LISTS(n->bar.cmbsz, 1);
> > NVME_CMBSZ_SET_RDS(n->bar.cmbsz, 1);
> > NVME_CMBSZ_SET_WDS(n->bar.cmbsz, 1);
> > NVME_CMBSZ_SET_SZU(n->bar.cmbsz, 2);
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 7ced5fd485a9..d27baa9d5391 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -27,11 +27,11 @@ typedef struct NvmeRequest {
> > struct NvmeSQueue *sq;
> > BlockAIOCB *aiocb;
> > uint16_t status;
> > - bool has_sg;
> > NvmeCqe cqe;
> > BlockAcctCookie acct;
> > QEMUSGList qsg;
> > QEMUIOVector iov;
> > + NvmeCmd cmd;
> > QTAILQ_ENTRY(NvmeRequest)entry;
> > } NvmeRequest;
> >
> > diff --git a/hw/block/trace-events b/hw/block/trace-events
> > index 9e5a4548bde0..77aa0da99ee0 100644
> > --- a/hw/block/trace-events
> > +++ b/hw/block/trace-events
> > @@ -33,6 +33,7 @@ nvme_dev_irq_msix(uint32_t vector) "raising MSI-X IRQ
> > vector %u"
> > nvme_dev_irq_pin(void) "pulsing IRQ pin"
> > nvme_dev_irq_masked(void) "IRQ is masked"
> > nvme_dev_dma_read(uint64_t prp1, uint64_t prp2) "DMA read,
> > prp1=0x%"PRIx64" prp2=0x%"PRIx64""
> > +nvme_dev_map_prp(uint16_t cid, uint8_t opc, uint64_t trans_len, uint32_t
> > len, uint64_t prp1, uint64_t prp2, int num_prps) "cid %"PRIu16" opc
> > 0x%"PRIx8" trans_len %"PRIu64" len %"PRIu32" prp1
> > 0x%"PRIx64" prp2 0x%"PRIx64" num_prps %d"
> > nvme_dev_rw(const char *verb, uint32_t blk_count, uint64_t byte_count,
> > uint64_t lba) "%s %"PRIu32" blocks (%"PRIu64" bytes) from LBA %"PRIu64""
> > nvme_dev_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t
> > qsize, uint16_t qflags) "create submission queue, addr=0x%"PRIx64",
> > sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16",
> > qflags=%"PRIu16""
> > nvme_dev_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t
> > size, uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64",
> > cqid=%"PRIu16", vector=%"PRIu16",
> > qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
> > diff --git a/include/block/nvme.h b/include/block/nvme.h
> > index 31eb9397d8c6..c1de92179596 100644
> > --- a/include/block/nvme.h
> > +++ b/include/block/nvme.h
> > @@ -427,6 +427,7 @@ enum NvmeStatusCodes {
> > NVME_CMD_ABORT_MISSING_FUSE = 0x000a,
> > NVME_INVALID_NSID = 0x000b,
> > NVME_CMD_SEQ_ERROR = 0x000c,
> > + NVME_INVALID_USE_OF_CMB = 0x0012,
> > NVME_LBA_RANGE = 0x0080,
> > NVME_CAP_EXCEEDED = 0x0081,
> > NVME_NS_NOT_READY = 0x0082,
>
>
> Overall I would split this commit into real refactoring and bugfixes.
Done!
> Best regards,
> Maxim Levitsky
>
