linux-kernel - Re: [PATCH 2/2] memory: ti-emif-sram: introduce relocatable suspend/resume handlers

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: <20170405135933.GN23750@n2100.armlinux.org.uk>
Date:   Wed, 5 Apr 2017 14:59:34 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        Keerthy J <j-keerthy@...com>, Dave Gerlach <d-gerlach@...com>
Subject: Re: [PATCH 2/2] memory: ti-emif-sram: introduce relocatable
 suspend/resume handlers

On Tue, Apr 04, 2017 at 09:11:52AM -0700, Tony Lindgren wrote:
> Russell,
> 
> * Dave Gerlach <d-gerlach@...com> [170328 13:57]:
> > Certain SoCs like Texas Instruments AM335x and AM437x require parts
> > of the EMIF PM code to run late in the suspend sequence from SRAM,
> > such as saving and restoring the EMIF context and placing the memory
> > into self-refresh.
> > 
> > One requirement for these SoCs to suspend and enter its lowest power
> > mode, called DeepSleep0, is that the PER power domain must be shut off.
> > Because the EMIF (DDR Controller) resides within this power domain, it
> > will lose context during a suspend operation, so we must save it so we
> > can restore once we resume. However, we cannot execute this code from
> > external memory, as it is not available at this point, so the code must
> > be executed late in the suspend path from SRAM.
> > 
> > This patch introduces a ti-emif-sram driver that includes several
> > functions written in ARM ASM that are relocatable so the PM SRAM
> > code can use them. It also allocates a region of writable SRAM to
> > be used by the code running in the executable region of SRAM to save
> > and restore the EMIF context. It can export a table containing the
> > absolute addresses of the available PM functions so that other SRAM
> > code can branch to them. This code is required for suspend/resume on
> > AM335x and AM437x to work.
> > 
> > In addition to this, to be able to share data structures between C and
> > the ti-emif-sram-pm assembly code, we can automatically generate all of
> > the C struct member offsets and sizes as macros by making use of the ARM
> > asm-offsets file. In the same header that we define our data structures
> > in we also define all the macros in an inline function and by adding a
> > call to this in the asm_offsets file all macros are properly generated
> > and available to the assembly code without cluttering up the asm-offsets
> > file.
> > 
> > Signed-off-by: Dave Gerlach <d-gerlach@...com>
> > ---
> >  arch/arm/kernel/asm-offsets.c    |   6 +
> >  drivers/memory/Kconfig           |  10 ++
> >  drivers/memory/Makefile          |   4 +
> >  drivers/memory/emif.h            |  17 ++
> >  drivers/memory/ti-emif-pm.c      | 295 ++++++++++++++++++++++++++++++++++
> >  drivers/memory/ti-emif-sram-pm.S | 334 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/ti-emif-sram.h     | 143 +++++++++++++++++
> >  7 files changed, 809 insertions(+)
> >  create mode 100644 drivers/memory/ti-emif-pm.c
> >  create mode 100644 drivers/memory/ti-emif-sram-pm.S
> >  create mode 100644 include/linux/ti-emif-sram.h
> > 
> > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
> > index 608008229c7d..d728b5660e36 100644
> > --- a/arch/arm/kernel/asm-offsets.c
> > +++ b/arch/arm/kernel/asm-offsets.c
> > @@ -28,6 +28,7 @@
> >  #include <asm/vdso_datapage.h>
> >  #include <asm/hardware/cache-l2x0.h>
> >  #include <linux/kbuild.h>
> > +#include <linux/ti-emif-sram.h>
> >  
> >  /*
> >   * Make sure that the compiler and target are compatible.
> > @@ -183,5 +184,10 @@ int main(void)
> >  #ifdef CONFIG_VDSO
> >    DEFINE(VDSO_DATA_SIZE,	sizeof(union vdso_data_store));
> >  #endif
> > +#if defined(CONFIG_SOC_AM33XX) || defined(CONFIG_SOC_AM43XX)
> > +  BLANK();
> > +  ti_emif_offsets();
> > +#endif
> > +
> >    return 0; 
> >  }
> 
> Does the above look OK to you?

I'm not going to comment on this yet, but I'll instead comment on the
newly appeared sram_exec_copy() stuff.

So, a few years ago, we went to significant effort in ARM land to come
up with a way to _safely_ copy assembler from the kernel into SRAM,
because copying code to SRAM that is compiled in thumb mode and then
executing it is _not_ as simple as memcpy(), cast the pointer to a
function pointer, and then call the function pointer.

The SRAM stuff throws all that out, instead preferring the dumb memcpy()
approach.

This needs resolving, and I'd like to see it resolved to the satisfaction
of architecture maintainers before we progress any further down this
route.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.