Messages in this thread | | | From | Leo Li <> | Subject | RE: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the virtualization context | Date | Tue, 26 Oct 2021 23:47:18 +0000 |
| |
> -----Original Message----- > From: Ioana Ciornei <ioana.ciornei@nxp.com> > Sent: Monday, October 18, 2021 10:11 AM > To: Leo Li <leoyang.li@nxp.com> > Cc: Youri Querry <youri.querry_1@nxp.com>; linux-kernel@vger.kernel.org; > Diana Madalina Craciun <diana.craciun@nxp.com>; Ioana Ciornei > <ioana.ciornei@nxp.com> > Subject: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the > virtualization context > > From: Diana Craciun <diana.craciun@nxp.com> > > When running as a guest, under KVM, the CENA region is mapped as device > memory, so uncacheable. When the memory is mapped as device memory, > the unaligned accesses are not allowed. Memcpy is optimized to transfer 8 > bytes at a time regardless of the start address and might cause alignment > issues. > > Signed-off-by: Diana Craciun <diana.craciun@nxp.com> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
We get the following feedback from Arnd about this patch. Could you respond to the comments?
"This patch looks very suspicious to me, I don't think it's generally safe to use memcpy_toio() on a normal pointer, as the __iomem tokens may be in a separate address range, even though this currently works on arm64. Adding the (__iomem void *) cast without a comment that explains why it's added seems similarly wrong, and finally the changeset text does not seem to match what the code does:
According to the text, the pointer is to a virtual address mapped as "device memory" (i.e. PROT_DEVICE_nGnRE or PROT_DEVICE_nGnRnE), but the code suggests it's actually write-combining normal (PROT_NORMAL_NC)."
> --- > drivers/soc/fsl/dpio/qbman-portal.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/soc/fsl/dpio/qbman-portal.c > b/drivers/soc/fsl/dpio/qbman-portal.c > index 3fd54611ed98..ef9cafd12534 100644 > --- a/drivers/soc/fsl/dpio/qbman-portal.c > +++ b/drivers/soc/fsl/dpio/qbman-portal.c > @@ -679,9 +679,9 @@ int qbman_swp_enqueue_multiple_direct(struct > qbman_swp *s, > for (i = 0; i < num_enqueued; i++) { > p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & > half_mask)); > /* Skip copying the verb */ > - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1); > - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)], > - &fd[i], sizeof(*fd)); > + memcpy_toio((__iomem void *)&p[1], &cl[1], > EQ_DESC_SIZE_WITHOUT_FD - 1); > + memcpy_toio((__iomem void > *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)], > + &fd[i], sizeof(*fd)); > eqcr_pi++; > } > > @@ -763,9 +763,9 @@ int > qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s, > for (i = 0; i < num_enqueued; i++) { > p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & > half_mask)); > /* Skip copying the verb */ > - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1); > - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)], > - &fd[i], sizeof(*fd)); > + memcpy_toio((__iomem void *)&p[1], &cl[1], > EQ_DESC_SIZE_WITHOUT_FD - 1); > + memcpy_toio((__iomem void > *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)], > + &fd[i], sizeof(*fd)); > eqcr_pi++; > } > > @@ -837,9 +837,9 @@ int > qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s, > p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & > half_mask)); > cl = (uint32_t *)(&d[i]); > /* Skip copying the verb */ > - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1); > - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)], > - &fd[i], sizeof(*fd)); > + memcpy_toio((__iomem void *)&p[1], &cl[1], > EQ_DESC_SIZE_WITHOUT_FD - 1); > + memcpy_toio((__iomem void > *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)], > + &fd[i], sizeof(*fd)); > eqcr_pi++; > } > > @@ -907,9 +907,9 @@ int > qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s, > p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi & > half_mask)); > cl = (uint32_t *)(&d[i]); > /* Skip copying the verb */ > - memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1); > - memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)], > - &fd[i], sizeof(*fd)); > + memcpy_toio((__iomem void *)&p[1], &cl[1], > EQ_DESC_SIZE_WITHOUT_FD - 1); > + memcpy_toio((__iomem void > *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)], > + &fd[i], sizeof(*fd)); > eqcr_pi++; > } > > -- > 2.33.1
| |