Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Leo Li <leoyang.li@nxp.com>
To:     Ioana Ciornei <ioana.ciornei@nxp.com>,
        Diana Madalina Craciun <diana.craciun@nxp.com>
CC:     Youri Querry <youri.querry_1@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Arnd Bergmann <arnd@arndb.de>
Subject: RE: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
 virtualization context
Thread-Topic: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
 virtualization context
Thread-Index: AQHXxDJcQzXRP2ZQ6EKg60CnRbWkUqvl/pfw
Date:   Tue, 26 Oct 2021 23:47:18 +0000
Message-ID: <AS8PR04MB8946734C740B202DC3CA88F08F849@AS8PR04MB8946.eurprd04.prod.outlook.com>
References: <20211018151034.137918-1-ioana.ciornei@nxp.com>
 <20211018151034.137918-5-ioana.ciornei@nxp.com>
In-Reply-To: <20211018151034.137918-5-ioana.ciornei@nxp.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?tMyTPbwtG73Q8jFRJouD8B4WOz+LvEGZU6ADFv0PcUbLbZTGaY89PwMf40qF?=
 =?us-ascii?Q?KkPvBfcnh8amCU7Htv19Q8qxjgzumwULRdWmM2GZRCOaBhRnZlqYKqvBWGM7?=
 =?us-ascii?Q?BDdIE8q3IHYd45P7xwhpmxCa63xi4vqz5NP1aPaq/Q0lQc0EkHVHMN1iVLog?=
 =?us-ascii?Q?vRBnvDMo4utSrAowNzw+ARwjmOBugWew8ODC/rerIiHGYj6Vbya1RwxpIn1z?=
 =?us-ascii?Q?h1Re6SoMwXHOSh0Ec9AIzdjmVMIyqg09gyr8O8AFCv7Ra5SxH9by/fufaogs?=
 =?us-ascii?Q?7e7fkEC2755yW74v43QqPazDTN/rWV2dNl5VbOVJu8ghsT2EQgUHUbH2o3ti?=
 =?us-ascii?Q?FYF1X0ZAfaOxA8vAVhH0e/2sKv2VASxC3lb817OPpKV9K8bFlPKZ1AcPC3AF?=
 =?us-ascii?Q?H3DmP8JhuklnrM+IzNwIXzyh0ZfNthjSRJ5v0G2tcUK3dAgxVtWVbIC12JOR?=
 =?us-ascii?Q?KX3/6H2LyyJpnFHLzolSaHE+Wm97M+WPA283LA/C/D+fJ/yrpRDlisii2NRA?=
 =?us-ascii?Q?FUaHlp6l/mdRjGqtXXkZHVfXggRALb7YBUTEMqaDjsLB83bCnpePIC3T7gV1?=
 =?us-ascii?Q?H60ICXuNVFNrV+MRXqfmtV0xFbzU6CyzTbpuNMgWhgmGUKJKVA/3+QDZHknV?=
 =?us-ascii?Q?xVWu497bRRVGUOKCvC4GN6w5bcGs3S+Pl7go8HSO3+8votssLTDZOA33B5vh?=
 =?us-ascii?Q?HBkoNOesR3RrD1njDfa5SFFKYoRk8hNabO0KTd6b8gGvQbr9nFwy2TWUhhcC?=
 =?us-ascii?Q?BAypWOZtu8LLyVL5waKO2TRBbX9lZuCJ7zjbpcQj+8zTYm4CII9uxlqa455h?=
 =?us-ascii?Q?4QChDwCBGA1izFd/2BTQ16GBzSiCNX42aVvyqLuYV9uE8nVDHcfejjiqC38P?=
 =?us-ascii?Q?5qvt4aIvaCRpSUpHoiuKT02BN+hsjqD94L5jytH6k6fXOEkxAHkQseCmwx6Z?=
 =?us-ascii?Q?wsfvBbTntTRBgrxDG6+2MURoM5mhJNPZcgDS3DDL52IGrEX6qoEY9q4kVsPp?=
 =?us-ascii?Q?1Z6VPZiIwEeXVTSi8H5Ae1H/q8fD8VN74eHwGXu7n0RA07vO6zcAQIv2Ld1y?=
 =?us-ascii?Q?i7OdOuioKhKae2T125wC8seYkOg3u0Ex7g6PDy7jq47qjuIxa7G9v5IALJQH?=
 =?us-ascii?Q?jJhNOB3cEMqCJ67aCXdncS5ULSleuiCYeC3Tzm0Ynk/JxmGslNav4x8OIbUW?=
 =?us-ascii?Q?90iZtjQEpldQvBFqPYZJhg11GH/efvy5lW/LUg54+NmXxzFBq7Y+sTEAk+fF?=
 =?us-ascii?Q?dYR01Yy1BaAVq0DsKQZouLx3jP5+gE+e4FrCqv2RociS2IbEPby+C4X+0HIf?=
 =?us-ascii?Q?QXYnAWS+WFqSSlFiytZEgG5vjtxhoX2c9FzxxAHuwjJGdX6LQ8psH1KHBtnN?=
 =?us-ascii?Q?xQpTlzmaaPVT3qSj6vrf0ebgu6aNLOVJbp/BnCIyLyGIBl23mPPYKO4X65gW?=
 =?us-ascii?Q?TN+qjTMDjyCwV45QXSCYCr8Bkiu5w3hQ7IXlmh5jI4RjedQjvzVxvtR5lTHu?=
 =?us-ascii?Q?u+uQtkWwMwaGTLE7WSm7Tbt3ojqqYwYWk2OezdAPxDJxrBh3DuNnvAmnVkTv?=
 =?us-ascii?Q?0WQzm0h0Q+5lpQz9pPn2iOtOsvG33F2NOZarIuR2skx+us/H73pmV/lY38OW?=
 =?us-ascii?Q?FA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB8946.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dea7f482-6bb7-499e-758b-08d998daf247
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Oct 2021 23:47:18.3413
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: sfDTQ1ilWhzmFiDlf5EeCuwBnjMCOVfQJOrc9LD6wTwcjxkGCb+nw/ea4fQS/3E9P8A44mGFL/n31//E06hS6w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB8737
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----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
>=20
> From: Diana Craciun <diana.craciun@nxp.com>
>=20
> 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.
>=20
> 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 respon=
d 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(-)
>=20
> 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 =3D 0; i < num_enqueued; i++) {
>  		p =3D (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++;
>  	}
>=20
> @@ -763,9 +763,9 @@ int
> qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s,
>  	for (i =3D 0; i < num_enqueued; i++) {
>  		p =3D (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++;
>  	}
>=20
> @@ -837,9 +837,9 @@ int
> qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
>  		p =3D (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		cl =3D (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++;
>  	}
>=20
> @@ -907,9 +907,9 @@ int
> qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s,
>  		p =3D (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		cl =3D (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++;
>  	}
>=20
> --
> 2.33.1