Re: [PATCH 12/16] hw/block/nvme: refactor NvmeRequest clearing
On Jul 29 20:47, Maxim Levitsky wrote:
> On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Move clearing of the structure from "clear before use" to "clear
> > after use".
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > 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(>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(>parent_obj, addr, (void *)>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.
Re: [PATCH 12/16] hw/block/nvme: refactor NvmeRequest clearing
On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Move clearing of the structure from "clear before use" to "clear
> after use".
>
> Signed-off-by: Klaus Jensen
> ---
> 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(>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(>parent_obj, addr, (void *)>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(>req_list, req, entry);
> }
> if (cq->tail != cq->head) {
> @@ -1601,7 +1607,6 @@ static void nvme_process_sq(void *opaque)
> req = QTAILQ_FIRST(>req_list);
> QTAILQ_REMOVE(>req_list, req, entry);
> QTAILQ_INSERT_TAIL(>out_req_list, req, entry);
> -memset(>cqe, 0, sizeof(req->cqe));
> req->cqe.cid = cmd.cid;
>
> status = sq->sqid ? nvme_io_cmd(n, , req) :
Best regards,
Maxim Levitsky
Re: [PATCH 12/16] hw/block/nvme: refactor NvmeRequest clearing
On 20-07-20 13:37:44, Klaus Jensen wrote: > From: Klaus Jensen > > Move clearing of the structure from "clear before use" to "clear > after use". Yah, agree on this. Reviewed-by: Minwoo Im
[PATCH 12/16] hw/block/nvme: refactor NvmeRequest clearing
From: Klaus Jensen
Move clearing of the structure from "clear before use" to "clear
after use".
Signed-off-by: Klaus Jensen
---
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(>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(>parent_obj, addr, (void *)>cqe,
sizeof(req->cqe));
+nvme_req_clear(req);
QTAILQ_INSERT_TAIL(>req_list, req, entry);
}
if (cq->tail != cq->head) {
@@ -1601,7 +1607,6 @@ static void nvme_process_sq(void *opaque)
req = QTAILQ_FIRST(>req_list);
QTAILQ_REMOVE(>req_list, req, entry);
QTAILQ_INSERT_TAIL(>out_req_list, req, entry);
-memset(>cqe, 0, sizeof(req->cqe));
req->cqe.cid = cmd.cid;
status = sq->sqid ? nvme_io_cmd(n, , req) :
--
2.27.0
