linux-kernel - Re: [PATCH] powerpc: Remove eieio in _memcpy

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b335d035-07a4-457d-99ea-8b7597782fd9@outer-limits.org>
Date: Tue, 28 Jan 2025 16:07:55 +0100
From: Julian Vetter <julian@...er-limits.org>
To: Christophe Leroy <christophe.leroy@...roup.eu>,
 Arnd Bergmann <arnd@...db.de>, Madhavan Srinivasan <maddy@...ux.ibm.com>,
 Michael Ellerman <mpe@...erman.id.au>, Nicholas Piggin <npiggin@...il.com>,
 Naveen N Rao <naveen@...nel.org>
Cc: linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc: Remove eieio in _memcpy_fromio

On 1/28/25 15:16, Christophe Leroy wrote:
> 
> 
> Le 28/01/2025 à 14:57, Julian Vetter a écrit :
>> Remove the eieio() calls in _memcpy_fromio, to bring its implementation
>> closer to the one from lib/iomem_copy.c. These eieio() calls don't seem
>> to be necessary, because the _memcpy_toio completely omits them. Also
>> the legacy code from ppc was not doing them.
> 
> What do you mean exactly by "legacy code" ?
> 
> As far as I can see they were already there before commit 68a64357d15a 
> ("[POWERPC] Merge 32 and 64 bits asm-powerpc/io.h"):
>

With 'ppc' I was refering to 'include/asm-ppc/io.h'. But you're right, 
when going back a bit, in the 'include/asm-powerpc/io.h' there are two 
cases, one (eeh_memcpy_fromio) which does the the 'eieio', and a second, 
i.e., 'iSeries_memcpy_fromio' which does a byte-wise copy. But in the 
ppc code ('include/asm-ppc/io.h') there is a simple memcpy. I was 
referring to this one. But my description is not very clear. Sorry for that.

> -static inline void eeh_memcpy_fromio(void *dest, const volatile void 
> __iomem *src,
> +static inline void eeh_memcpy_fromio(void *dest, const
> +                     volatile void __iomem *src,
>                        unsigned long n)
>   {
> -    void *vsrc = (void __force *) src;
> -    void *destsave = dest;
> -    unsigned long nsave = n;
> -
> -    __asm__ __volatile__ ("sync" : : : "memory");
> -    while(n && (!EEH_CHECK_ALIGN(vsrc, 4) || !EEH_CHECK_ALIGN(dest, 4))) {
> -        *((u8 *)dest) = *((volatile u8 *)vsrc);
> -        __asm__ __volatile__ ("eieio" : : : "memory");
> -        vsrc++;
> -        dest++;
> -        n--;
> -    }
> -    while(n > 4) {
> -        *((u32 *)dest) = *((volatile u32 *)vsrc);
> -        __asm__ __volatile__ ("eieio" : : : "memory");
> -        vsrc += 4;
> -        dest += 4;
> -        n -= 4;
> -    }
> -    while(n) {
> -        *((u8 *)dest) = *((volatile u8 *)vsrc);
> -        __asm__ __volatile__ ("eieio" : : : "memory");
> -        vsrc++;
> -        dest++;
> -        n--;
> -    }
> -    __asm__ __volatile__ ("sync" : : : "memory");
> +    _memcpy_fromio(dest, src, n);
> 
>       /* Look for ffff's here at dest[n].  Assume that at least 4 bytes
>        * were copied. Check all four bytes.
>        */
> -    if ((nsave >= 4) &&
> -        (EEH_POSSIBLE_ERROR((*((u32 *) destsave+nsave-4)), u32))) {
> -        eeh_check_failure(src, (*((u32 *) destsave+nsave-4)));
> -    }
> +    if (n >= 4 && EEH_POSSIBLE_ERROR(*((u32 *)(dest + n - 4)), u32))
> +        eeh_check_failure(src, *((u32 *)(dest + n - 4)));
>   }
> 
> 
> 
>>
>> Signed-off-by: Julian Vetter <julian@...er-limits.org>
>> ---
>>   arch/powerpc/kernel/io.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/io.c b/arch/powerpc/kernel/io.c
>> index 6af535905984..81e5d54260a1 100644
>> --- a/arch/powerpc/kernel/io.c
>> +++ b/arch/powerpc/kernel/io.c
>> @@ -155,21 +155,18 @@ void _memcpy_fromio(void *dest, const volatile 
>> void __iomem *src,
>>          __asm__ __volatile__ ("sync" : : : "memory");
>>          while(n && (!IO_CHECK_ALIGN(vsrc, 4) || !IO_CHECK_ALIGN(dest, 
>> 4))) {
>>                  *((u8 *)dest) = *((volatile u8 *)vsrc);
>> -               eieio();
>>                  vsrc++;
>>                  dest++;
>>                  n--;
>>          }
>>          while(n >= 4) {
>>                  *((u32 *)dest) = *((volatile u32 *)vsrc);
>> -               eieio();
>>                  vsrc += 4;
>>                  dest += 4;
>>                  n -= 4;
>>          }
>>          while(n) {
>>                  *((u8 *)dest) = *((volatile u8 *)vsrc);
>> -               eieio();
>>                  vsrc++;
>>                  dest++;
>>                  n--;
>> -- 
>> 2.34.1
>>
>