linux-kernel - Re: [Patch v3 10/11] driver/edac/layerscape

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: <20160809111241.GH27508@arm.com>
Date:	Tue, 9 Aug 2016 12:12:42 +0100
From:	Will Deacon <will.deacon@....com>
To:	york sun <york.sun@....com>
Cc:	Marc Zyngier <marc.zyngier@....com>,
	"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
	"morbidrsa@...il.com" <morbidrsa@...il.com>,
	"oss@...error.net" <oss@...error.net>,
	Stuart Yoder <stuart.yoder@....com>,
	"bp@...en8.de" <bp@...en8.de>,
	Russell King <linux@...linux.org.uk>,
	Catalin Marinas <catalin.marinas@....com>,
	Doug Thompson <dougthompson@...ssion.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>,
	James Morse <james.morse@....com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	AKASHI Takahiro <takahiro.akashi@...aro.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [Patch v3 10/11] driver/edac/layerscape_edac: Add Layerscape
 EDAC support

On Mon, Aug 08, 2016 at 07:56:04PM +0000, york sun wrote:
> On 08/08/2016 11:07 AM, Marc Zyngier wrote:
> > On Thu, 4 Aug 2016 15:58:35 -0700
> > York Sun <york.sun@....com> wrote:
> >
> >> Add DDR EDAC for ARM-based compatible controllers. Both big-endian
> >> and little-endian are supported.
> >>
> >> Signed-off-by: York Sun <york.sun@....com>
> >>
> >> ---
> >> Change log
> >>   v3: no change
> >>   v2: Create new driver using shared DDR object
> >>
> >>  arch/arm64/Kconfig.platforms           |  1 +
> >>  arch/{arm => arm64}/include/asm/edac.h | 21 +++++------
> >>  arch/arm64/include/asm/irq.h           |  4 ++
> >>  drivers/edac/Kconfig                   |  7 ++++
> >>  drivers/edac/Makefile                  |  3 ++
> >>  drivers/edac/fsl_ddr_edac.c            |  1 +
> >>  drivers/edac/layerscape_edac.c         | 67 ++++++++++++++++++++++++++++++++++
> >>  7 files changed, 92 insertions(+), 12 deletions(-)
> >>  copy arch/{arm => arm64}/include/asm/edac.h (79%)
> >>  create mode 100644 drivers/edac/layerscape_edac.c
> >>
> >> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> >> index 7ef1d05..185a215 100644
> >> --- a/arch/arm64/Kconfig.platforms
> >> +++ b/arch/arm64/Kconfig.platforms
> >> @@ -41,6 +41,7 @@ config ARCH_EXYNOS
> >>
> >>  config ARCH_LAYERSCAPE
> >>  	bool "ARMv8 based Freescale Layerscape SoC family"
> >> +	select EDAC_SUPPORT
> >>  	help
> >>  	  This enables support for the Freescale Layerscape SoC family.
> >>
> >> diff --git a/arch/arm/include/asm/edac.h b/arch/arm64/include/asm/edac.h
> >> similarity index 79%
> >> copy from arch/arm/include/asm/edac.h
> >> copy to arch/arm64/include/asm/edac.h
> >> index 5189fa8..36a226c 100644
> >> --- a/arch/arm/include/asm/edac.h
> >> +++ b/arch/arm64/include/asm/edac.h
> >> @@ -18,16 +18,15 @@
> >>  #define ASM_EDAC_H
> >>  /*
> >>   * ECC atomic, DMA, SMP and interrupt safe scrub function.
> >> - * Implements the per arch edac_atomic_scrub() that EDAC use for software
> >> + * Implements the per arch atomic_scrub() that EDAC use for software
> >>   * ECC scrubbing.  It reads memory and then writes back the original
> >>   * value, allowing the hardware to detect and correct memory errors.
> >>   */
> >> -
> >> -static inline void edac_atomic_scrub(void *va, u32 size)
> >> +static inline void atomic_scrub(void *va, u32 size)
> >>  {
> >> -#if __LINUX_ARM_ARCH__ >= 6
> >> -	unsigned int *virt_addr = va;
> >> -	unsigned int temp, temp2;
> >> +	unsigned long *virt_addr = va;
> >> +	unsigned long temp;
> >> +	unsigned int temp2;
> >>  	unsigned int i;
> >>
> >>  	for (i = 0; i < size / sizeof(*virt_addr); i++, virt_addr++) {
> >> @@ -35,15 +34,13 @@ static inline void edac_atomic_scrub(void *va, u32 size)
> >>  		 * so we are interrupt, DMA and SMP safe.
> >>  		 */
> >>  		__asm__ __volatile__("\n"
> >> -			"1:	ldrex	%0, [%2]\n"
> >> -			"	strex	%1, %0, [%2]\n"
> >> -			"	teq	%1, #0\n"
> >> -			"	bne	1b\n"
> >> +			"1:	ldxr	%0, [%2]\n"
> >> +			"	stxr	%w1, %0, [%2]\n"
> >> +			"	cbnz	%w1, 1b\n"
> >
> > Hiding architecture code in a driver, are we? Hmmm.
> 
> This adds atomic_scrub() for arm64. Arm already has this function. Are 
> you suggesting to separate this part as an individual patch?
> >
> > Also, how can this be safe if you have non cache-coherent DMA going on?
> 
> I am clueless on this topic. What's your suggestion.

We've discussed this in the past for ARMv7 and the bottom line is that
it's not safe to scrub memory backing non-coherent DMA buffers. You will
likely end up with data corruption if you do that.

The two options are:

  (1) Restrict scrubbing to non-DMA memory, or
  (2) Only enable scrubbing if the system is completely cache-coherent

The problem with (1) is that we can't detect that, particularly when
the streaming DMA API is in use.

The problem with (2) is that you can have a coherent system that decides
dynamically to drop in and out of coherency as a result of, e.g. PCI
NoSnoop transactions.

Will