| Message ID | 20200720113748.322965-13-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nvme: dma handling and address mapping cleanup | expand |
On 20-07-20 13:37:44, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Move clearing of the structure from "clear before use" to "clear > after use". Yah, agree on this. Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>
On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > Move clearing of the structure from "clear before use" to "clear > after use". > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > --- > hw/block/nvme.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index e2932239c661..431f26c2f589 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -209,6 +209,11 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq) > } > } > > +static void nvme_req_clear(NvmeRequest *req) > +{ > + memset(&req->cqe, 0x0, sizeof(req->cqe)); > +} > + > static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr, > size_t len) > { > @@ -458,6 +463,7 @@ static void nvme_post_cqes(void *opaque) > nvme_inc_cq_tail(cq); > pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe, > sizeof(req->cqe)); > + nvme_req_clear(req); Don't we need some barrier here to avoid reordering the writes? pci_dma_write does seem to include a barrier prior to the write it does but not afterward. Also what is the motivation of switching the order? I think somewhat that it is a good thing to clear a buffer, before it is setup. > QTAILQ_INSERT_TAIL(&sq->req_list, req, entry); > } > if (cq->tail != cq->head) { > @@ -1601,7 +1607,6 @@ static void nvme_process_sq(void *opaque) > req = QTAILQ_FIRST(&sq->req_list); > QTAILQ_REMOVE(&sq->req_list, req, entry); > QTAILQ_INSERT_TAIL(&sq->out_req_list, req, entry); > - memset(&req->cqe, 0, sizeof(req->cqe)); > req->cqe.cid = cmd.cid; > > status = sq->sqid ? nvme_io_cmd(n, &cmd, req) : Best regards, Maxim Levitsky
On Jul 29 20:47, Maxim Levitsky wrote: > On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > Move clearing of the structure from "clear before use" to "clear > > after use". > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > --- > > hw/block/nvme.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index e2932239c661..431f26c2f589 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -209,6 +209,11 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq) > > } > > } > > > > +static void nvme_req_clear(NvmeRequest *req) > > +{ > > + memset(&req->cqe, 0x0, sizeof(req->cqe)); > > +} > > + > > static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr, > > size_t len) > > { > > @@ -458,6 +463,7 @@ static void nvme_post_cqes(void *opaque) > > nvme_inc_cq_tail(cq); > > pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe, > > sizeof(req->cqe)); > > + nvme_req_clear(req); > > Don't we need some barrier here to avoid reordering the writes? > pci_dma_write does seem to include a barrier prior to the write it does > but not afterward. > > Also what is the motivation of switching the order? This was just preference. But I did not consider that I would be breaking any DMA rules here. > I think somewhat that it is a good thing to clear a buffer, > before it is setup. > I'll reverse my preference and keep the status quo since I have no better motivation than personal preference. The introduction of nvme_req_clear is just in preparation for consolidating more cleanup here, but I'll drop this patch and introduce nvme_req_clear later.
diff --git a/hw/block/nvme.c b/hw/block/nvme.c index e2932239c661..431f26c2f589 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -209,6 +209,11 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq) } } +static void nvme_req_clear(NvmeRequest *req) +{ + memset(&req->cqe, 0x0, sizeof(req->cqe)); +} + static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr addr, size_t len) { @@ -458,6 +463,7 @@ static void nvme_post_cqes(void *opaque) nvme_inc_cq_tail(cq); pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe, sizeof(req->cqe)); + nvme_req_clear(req); QTAILQ_INSERT_TAIL(&sq->req_list, req, entry); } if (cq->tail != cq->head) { @@ -1601,7 +1607,6 @@ static void nvme_process_sq(void *opaque) req = QTAILQ_FIRST(&sq->req_list); QTAILQ_REMOVE(&sq->req_list, req, entry); QTAILQ_INSERT_TAIL(&sq->out_req_list, req, entry); - memset(&req->cqe, 0, sizeof(req->cqe)); req->cqe.cid = cmd.cid; status = sq->sqid ? nvme_io_cmd(n, &cmd, req) :