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;
Message-ID: <69bd66c8-d4f5-7649-33bc-71a507c65ce1@nxp.com>
Date:   Thu, 28 Oct 2021 14:32:09 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
 virtualization context
Content-Language: en-US
To:     Leo Li <leoyang.li@nxp.com>, Ioana Ciornei <ioana.ciornei@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>
References: <20211018151034.137918-1-ioana.ciornei@nxp.com>
 <20211018151034.137918-5-ioana.ciornei@nxp.com>
 <AS8PR04MB8946734C740B202DC3CA88F08F849@AS8PR04MB8946.eurprd04.prod.outlook.com>
From:   Diana Madalina Craciun <diana.craciun@nxp.com>
In-Reply-To: <AS8PR04MB8946734C740B202DC3CA88F08F849@AS8PR04MB8946.eurprd04.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YlZPSVluZE0rVFZKMjNlWUo0ZE5kSEtycTlRZVlVeUNvcEJvdEkrUjR4bHkz?=
 =?utf-8?B?SVlleGZpNHo2eVJsc1RXajA1R3NIdDhsK3pFKzlCa3Z1LytsSjlmOWp4SnNR?=
 =?utf-8?B?dG04ZllSTDUwcXV2bG41T1dtM2plUUNxZENtN215R21vOHVDdHUxdG1vMmZu?=
 =?utf-8?B?ZUo0c0F2Z1pvaEZ3clJXUVZPUk82WVJjMFdYTlFJZVY4eDVOWFpOL284VGh4?=
 =?utf-8?B?RXl3TVhQL2dqSkUxTkxVeUo5OS8wbWFLakZNR1U4T3FiOW5oa3A1YWFVLzQv?=
 =?utf-8?B?UnBkM0V2cDEzSE5lbnpITTQrZHd3eDdQU2MyOU1CYVJ1NWtuZzRzOU9HSlF5?=
 =?utf-8?B?UzJyblBab1YyZURJOTlvYmc2MVhSZU1tQk55YjROalNIc0RWbmVYQkRHenB2?=
 =?utf-8?B?ei8zYjUvY3I0U0hMMkxhcHhJa3FWUyt0Wi9Gcm5vck5lSys0OUJva2JYa3dZ?=
 =?utf-8?B?SEpwVmVBSlE5NUtoRDJlSDN2ak1uZmtCbnN3TEZCdWhvQ09wdmd3YnNlU2Fq?=
 =?utf-8?B?VEJ4a0svbmhINUdWOEg1QzZMZ204TXJKU0xvcXRoQTk3VmgwTW9LWmswL2hS?=
 =?utf-8?B?NE1ibE9ud2crc0xzVGxqalhUQ1lPdXJWMFFHalRBWTdZQS9RTWpHTEJ1Wm9S?=
 =?utf-8?B?NVhWUHRFcERHM09Xb2phblpvRW42VXhsUjF3V29DMFlwMkF0T1ZYc3U3elRG?=
 =?utf-8?B?Zm1qdThrclRwRGJzVHc3ZTNnOHNkRXJRNnAzYjIzd0Q1Sm5CdFBtc2RQU0Js?=
 =?utf-8?B?UlBPQzViRk83K0ZZVGE2cHBPTEtTVm9LSjUwSGRkYWJEZ3pUZ09ZN3NTZlQv?=
 =?utf-8?B?OWlNS0dZQThTUXpGTFZEVXU3KzlFNEJWS3ZlRjZpZWNNRy8rSkViZmk2dVJl?=
 =?utf-8?B?RFFGRTBVUXFtUERTZk9GUW9IUDZwMDlQVFBJeEZyK0NTT1pYbkt6enhlVm9S?=
 =?utf-8?B?VER1T3JvZFNuUHF2NEhQZGphRzMwcG14ZEJEcTV5Nm9pajY5SWZSVXhNc1VS?=
 =?utf-8?B?YVRsc3I0b2pWSHBjZzQrbjFFaVFoOHAyZXZ0Mm5pVTVBSmRlcUpoeW1KRUkw?=
 =?utf-8?B?bUIwMksyNWd0eHE5QjBiRll3RGhUWHRxdXBBOXFMb3BnYkRqbTNzRmNLTEF1?=
 =?utf-8?B?Q2RPODRjVXJqQ0ZBN05qSnlCOGxCU3JpMHlycEtib0dLdkFxRlh0SVBrdnFL?=
 =?utf-8?B?LzVyb0RRVTNlOGFvblRzeW9Qd2trbnZqYlRLOE0rUG54MnhQbTRjdTdQZVZR?=
 =?utf-8?B?RzNEU0FBcGpYM2wvY1VJaWNyamd0ZGgwSVgxbXR0Z2JQS3ZBa0xGcGhEcnVm?=
 =?utf-8?B?LzQzSmp0bzNJaTVZM2hhQnZ0TTF0azNIRmZlQ3pLQjZxSzZPbnAvV2hjQWNq?=
 =?utf-8?B?MHJabHRtZG9vNUErVm84UHpXbEVndUpNeHFOYlZXWWhlZm8xeTBERDVZdmc3?=
 =?utf-8?B?bGl3OWRNN2VnWVN6UmVLWnVkcGQyamN1Y1lWZnZMU3U4TnE4d3J5ME1wT1Bi?=
 =?utf-8?B?aGQ3amtVeFM3VXlGcDlrRVRQME1vY3U2OE80Vnp3bjdXVnZxQ09ERlRCRmRB?=
 =?utf-8?B?QVZtZUl6emZXL2l4Umd2eUxta3hXS1dFUEgwMTlraEcxTzNvNTJsMEpaVTcv?=
 =?utf-8?B?V2hXLzlIS1F4dG80a1E0bi9FdUg1NHdwQXRxZGcxWUx0d3REc25Ha1pMYVhz?=
 =?utf-8?B?UXNESDhOd1R0Y2MzbGR6YVJiY3N4MjhFSzNDeTZWeUVGQUU1Y1JNOU1iQkpa?=
 =?utf-8?B?eFpINWRadVIvbmNOSkRiVnI4MWZqc2VLc09GWXZuZnpjL0RPNjhUVmhRdExH?=
 =?utf-8?B?aUFqRnQxK21MbEs3R29iYTlseE5VOC9HUWY0eFVMVXlnNHpHK0FDYkVCMzgx?=
 =?utf-8?B?STVKOGU4RUpKNVdRRTJvN3N2cWswSmVYdUxJZStJZjVRTHdkUVJFd0NjbWV2?=
 =?utf-8?B?aHNLMnQ4MUd6ZDNaNFVVL3lOTUdVL1diNWRFOTFCbUl1dWQrQ3BmOFRkOWVo?=
 =?utf-8?B?d3RiTVIzYVVkOXIrNktSdXd6V3d1a3pJU0FrTWxudXNVblZYajVmZm1qbjY4?=
 =?utf-8?B?aENXRTlnK0ZFWkV5ODlyQnJFbmRzNXphSkozaGhWMGhYNnk2YTRsdDlHQVFW?=
 =?utf-8?B?bGxBNXkvWWkvU1E5SFk0R2JsZnd0azZSMGlxemY4dWNRczE2VGV3K0tjM2du?=
 =?utf-8?Q?gEL9L//Ia0aL/YFYOjA6nXU=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 567af497-14a9-4acd-94ce-08d99a069586
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0402MB3503.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2021 11:32:12.0731
 (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: G6A/7iatK/PkNsrIzfwgpwmGyaYD8kxZPvucrrydcIECz3JoV+jsTtlUw75n5+wk3LtZM7VAJ/+RIYmCplFCXw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6368
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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