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: <81ee3098-1e43-4b88-975a-d223bc2338e8@csgroup.eu>
Date: Tue, 28 Jan 2025 15:16:23 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Julian Vetter <julian@...er-limits.org>, 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



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"):

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