Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
On Jul 20 09:51, Peter Maydell wrote:
> On Thu, 20 Jul 2023 at 09:49, Klaus Jensen wrote:
> >
> > On Jul 20 09:43, Peter Maydell wrote:
> > > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev wrote:
> > > >
> > > > 19.07.2023 10:36, Klaus Jensen wrote:
> > > > pu(req->cmd.dptr.prp2);
> > > > > +uint32_t v;
> > > >
> > > > > if (sq) {
> > > > > +v = cpu_to_le32(sq->tail);
> > > >
> > > > > -pci_dma_write(pci, sq->db_addr, >tail,
> > > > > sizeof(sq->tail));
> > > > > +pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
> > > >
> > > > This and similar cases hurts my eyes.
> > > >
> > > > Why we pass address of v here, but use sizeof(sq->tail) ?
> > > >
> > > > Yes, I know both in theory should be of the same size, but heck,
> > > > this is puzzling at best, and confusing in a regular case.
> > > >
> > > > Dunno how it slipped in the review, it instantly catched my eye
> > > > in a row of applied patches..
> > > >
> > > > Also, why v is computed a few lines before it is used, with
> > > > some expressions between the assignment and usage?
> > > >
> > > > How about the following patch:
> > >
> > > If you're going to change this, better to take the approach
> > > Philippe suggested in review of using stl_le_pci_dma().
> > >
> > > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
> > >
> >
> > Yup, that was my plan for next. But the original patch was already
> > verified on hardware and mutiple testes, so wanted to go with that for
> > the "fix".
> >
> > But yes, I will refactor into the much nicer stl/ldl api.
>
> FWIW, I don't think this bug fix was so urgent that we
> needed to go with a quick fix and a followup -- we're
> not yet that close to 8.1 release.
>
Alright, noted ;) I will spin this into the other fix I have under
review.
signature.asc
Description: PGP signature
Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
On Thu, 20 Jul 2023 at 09:49, Klaus Jensen wrote:
>
> On Jul 20 09:43, Peter Maydell wrote:
> > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev wrote:
> > >
> > > 19.07.2023 10:36, Klaus Jensen wrote:
> > > pu(req->cmd.dptr.prp2);
> > > > +uint32_t v;
> > >
> > > > if (sq) {
> > > > +v = cpu_to_le32(sq->tail);
> > >
> > > > -pci_dma_write(pci, sq->db_addr, >tail,
> > > > sizeof(sq->tail));
> > > > +pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
> > >
> > > This and similar cases hurts my eyes.
> > >
> > > Why we pass address of v here, but use sizeof(sq->tail) ?
> > >
> > > Yes, I know both in theory should be of the same size, but heck,
> > > this is puzzling at best, and confusing in a regular case.
> > >
> > > Dunno how it slipped in the review, it instantly catched my eye
> > > in a row of applied patches..
> > >
> > > Also, why v is computed a few lines before it is used, with
> > > some expressions between the assignment and usage?
> > >
> > > How about the following patch:
> >
> > If you're going to change this, better to take the approach
> > Philippe suggested in review of using stl_le_pci_dma().
> >
> > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
> >
>
> Yup, that was my plan for next. But the original patch was already
> verified on hardware and mutiple testes, so wanted to go with that for
> the "fix".
>
> But yes, I will refactor into the much nicer stl/ldl api.
FWIW, I don't think this bug fix was so urgent that we
needed to go with a quick fix and a followup -- we're
not yet that close to 8.1 release.
thanks
-- PMM
Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
On Jul 20 09:43, Peter Maydell wrote:
> On Wed, 19 Jul 2023 at 21:13, Michael Tokarev wrote:
> >
> > 19.07.2023 10:36, Klaus Jensen wrote:
> > pu(req->cmd.dptr.prp2);
> > > +uint32_t v;
> >
> > > if (sq) {
> > > +v = cpu_to_le32(sq->tail);
> >
> > > -pci_dma_write(pci, sq->db_addr, >tail, sizeof(sq->tail));
> > > +pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
> >
> > This and similar cases hurts my eyes.
> >
> > Why we pass address of v here, but use sizeof(sq->tail) ?
> >
> > Yes, I know both in theory should be of the same size, but heck,
> > this is puzzling at best, and confusing in a regular case.
> >
> > Dunno how it slipped in the review, it instantly catched my eye
> > in a row of applied patches..
> >
> > Also, why v is computed a few lines before it is used, with
> > some expressions between the assignment and usage?
> >
> > How about the following patch:
>
> If you're going to change this, better to take the approach
> Philippe suggested in review of using stl_le_pci_dma().
>
> https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
>
Yup, that was my plan for next. But the original patch was already
verified on hardware and mutiple testes, so wanted to go with that for
the "fix".
But yes, I will refactor into the much nicer stl/ldl api.
signature.asc
Description: PGP signature
Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
On Wed, 19 Jul 2023 at 21:13, Michael Tokarev wrote:
>
> 19.07.2023 10:36, Klaus Jensen wrote:
> pu(req->cmd.dptr.prp2);
> > +uint32_t v;
>
> > if (sq) {
> > +v = cpu_to_le32(sq->tail);
>
> > -pci_dma_write(pci, sq->db_addr, >tail, sizeof(sq->tail));
> > +pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
>
> This and similar cases hurts my eyes.
>
> Why we pass address of v here, but use sizeof(sq->tail) ?
>
> Yes, I know both in theory should be of the same size, but heck,
> this is puzzling at best, and confusing in a regular case.
>
> Dunno how it slipped in the review, it instantly catched my eye
> in a row of applied patches..
>
> Also, why v is computed a few lines before it is used, with
> some expressions between the assignment and usage?
>
> How about the following patch:
If you're going to change this, better to take the approach
Philippe suggested in review of using stl_le_pci_dma().
https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673f...@linaro.org/
thanks
-- PMM
Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
19.07.2023 10:36, Klaus Jensen wrote:
pu(req->cmd.dptr.prp2);
+uint32_t v;
if (sq) {
+v = cpu_to_le32(sq->tail);
-pci_dma_write(pci, sq->db_addr, >tail, sizeof(sq->tail));
+pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
This and similar cases hurts my eyes.
Why we pass address of v here, but use sizeof(sq->tail) ?
Yes, I know both in theory should be of the same size, but heck,
this is puzzling at best, and confusing in a regular case.
Dunno how it slipped in the review, it instantly catched my eye
in a row of applied patches..
Also, why v is computed a few lines before it is used, with
some expressions between the assignment and usage?
How about the following patch:
From: Michael Tokarev
Date: Wed, 19 Jul 2023 23:10:53 +0300
Subject: [PATCH trivial] hw/nvme: fix sizeof() misuse and move endianness
conversions
closer to users
Signed-off-by: Michael Tokarev
Fixes: ea3c76f1494d0
---
hw/nvme/ctrl.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dadc2dc7da..e33b28cf66 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6820,6 +6820,4 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
if (sq) {
-v = cpu_to_le32(sq->tail);
-
/*
* CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
@@ -6829,5 +6827,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
sq->db_addr = dbs_addr + (i << 3);
sq->ei_addr = eis_addr + (i << 3);
-pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
+v = cpu_to_le32(sq->tail);
+pci_dma_write(pci, sq->db_addr, , sizeof(v));
if (n->params.ioeventfd && sq->sqid != 0) {
@@ -6839,10 +6838,9 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
if (cq) {
-v = cpu_to_le32(cq->head);
-
/* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
-pci_dma_write(pci, cq->db_addr, , sizeof(cq->head));
+v = cpu_to_le32(cq->head);
+pci_dma_write(pci, cq->db_addr, , sizeof(v));
if (n->params.ioeventfd && cq->cqid != 0) {
@@ -7661,5 +7659,5 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
if (!qid && n->dbbuf_enabled) {
v = cpu_to_le32(cq->head);
-pci_dma_write(pci, cq->db_addr, , sizeof(cq->head));
+pci_dma_write(pci, cq->db_addr, , sizeof(v));
}
if (start_sqs) {
@@ -7721,6 +7719,4 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
sq->tail = new_tail;
if (!qid && n->dbbuf_enabled) {
-v = cpu_to_le32(sq->tail);
-
/*
* The spec states "the host shall also update the controller's
@@ -7736,5 +7732,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
* so we can't trust reading it for an appropriate sq tail.
*/
-pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
+v = cpu_to_le32(sq->tail);
+pci_dma_write(pci, sq->db_addr, , sizeof(v));
}
[PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells
From: Klaus Jensen
In commit 2fda0726e514 ("hw/nvme: fix missing endian conversions for
doorbell buffers"), we fixed shadow doorbells for big-endian guests
running on little endian hosts. But I did not fix little-endian guests
on big-endian hosts. Fix this.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1765
Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
Cc: qemu-sta...@nongnu.org
Reported-by: Thomas Huth
Tested-by: Cédric Le Goater
Tested-by: Thomas Huth
Reviewed-by: Keith Busch
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8e8e870b9a80..dadc2dc7da10 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6801,6 +6801,7 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
PCIDevice *pci = PCI_DEVICE(n);
uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1);
uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2);
+uint32_t v;
int i;
/* Address should be page aligned */
@@ -6818,6 +6819,8 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
NvmeCQueue *cq = n->cq[i];
if (sq) {
+v = cpu_to_le32(sq->tail);
+
/*
* CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
* nvme_process_db() uses this hard-coded way to calculate
@@ -6825,7 +6828,7 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
*/
sq->db_addr = dbs_addr + (i << 3);
sq->ei_addr = eis_addr + (i << 3);
-pci_dma_write(pci, sq->db_addr, >tail, sizeof(sq->tail));
+pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
if (n->params.ioeventfd && sq->sqid != 0) {
if (!nvme_init_sq_ioeventfd(sq)) {
@@ -6835,10 +6838,12 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const
NvmeRequest *req)
}
if (cq) {
+v = cpu_to_le32(cq->head);
+
/* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
-pci_dma_write(pci, cq->db_addr, >head, sizeof(cq->head));
+pci_dma_write(pci, cq->db_addr, , sizeof(cq->head));
if (n->params.ioeventfd && cq->cqid != 0) {
if (!nvme_init_cq_ioeventfd(cq)) {
@@ -7587,7 +7592,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr,
unsigned size)
static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
{
PCIDevice *pci = PCI_DEVICE(n);
-uint32_t qid;
+uint32_t qid, v;
if (unlikely(addr & ((1 << 2) - 1))) {
NVME_GUEST_ERR(pci_nvme_ub_db_wr_misaligned,
@@ -7654,7 +7659,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
start_sqs = nvme_cq_full(cq) ? 1 : 0;
cq->head = new_head;
if (!qid && n->dbbuf_enabled) {
-pci_dma_write(pci, cq->db_addr, >head, sizeof(cq->head));
+v = cpu_to_le32(cq->head);
+pci_dma_write(pci, cq->db_addr, , sizeof(cq->head));
}
if (start_sqs) {
NvmeSQueue *sq;
@@ -7714,6 +7720,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
sq->tail = new_tail;
if (!qid && n->dbbuf_enabled) {
+v = cpu_to_le32(sq->tail);
+
/*
* The spec states "the host shall also update the controller's
* corresponding doorbell property to match the value of that entry
@@ -7727,7 +7735,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
* including ones that run on Linux, are not updating Admin Queues,
* so we can't trust reading it for an appropriate sq tail.
*/
-pci_dma_write(pci, sq->db_addr, >tail, sizeof(sq->tail));
+pci_dma_write(pci, sq->db_addr, , sizeof(sq->tail));
}
qemu_bh_schedule(sq->bh);
--
2.41.0
