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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 28 Oct 2021 14:32:09 +0300
From:   Diana Madalina Craciun <diana.craciun@....com>
To:     Leo Li <leoyang.li@....com>, Ioana Ciornei <ioana.ciornei@....com>
Cc:     Youri Querry <youri.querry_1@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
 virtualization context

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