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

[Leo Li]

> -----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