Re: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the virtualization context

[Diana Madalina Craciun]

Hi Leo,

Just drop this patch. I realize that the implementation is not correct. 
The problem should be resolved (if possible) elsewhere.

I will open another thread on the relevant lists regarding this problem.

Diana

On 10/27/2021 2:47 AM, Leo Li wrote:
> > -----Original Message-----
> > From: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > Sent: Monday, October 18, 2021 10:11 AM
> > To: Leo Li <leoyang.li@xxxxxxx>
> > Cc: Youri Querry <youri.querry_1@xxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > Diana Madalina Craciun <diana.craciun@xxxxxxx>; Ioana Ciornei
> > <ioana.ciornei@xxxxxxx>
> > Subject: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
> > virtualization context
> >
> > From: Diana Craciun <diana.craciun@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> 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