Re: [PATCH] MIPS: OCTEON: Enable KASLR

[Matt Redfearn <matt.redfearn@xxxxxxxxxx>]

Hi David,


On 09/12/16 18:46, David Daney wrote:
> On 12/09/2016 01:35 AM, Steven J. Hill wrote:
> > This patch enables KASLR for OCTEON systems. The SMP startup code is
> > such that the secondaries monitor the volatile variable
> > 'octeon_processor_relocated_kernel_entry' for any non-zero value.
> > The 'plat_post_relocation hook' is used to set that value to the
> > kernel entry point of the relocated kernel. The secondary CPUs will
> > then jumps to the new kernel, perform their initialization again
> > and begin waiting for the boot CPU to start them via the relocated
> > loop 'octeon_spin_wait_boot'. Inspired by Steven's code from Cavium.
> >
> > Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> > Signed-off-by: Steven J. Hill <Steven.Hill@xxxxxxxxxx>
> > ---
> >  arch/mips/Kconfig                                         |  3 ++-
> >  arch/mips/cavium-octeon/smp.c                             |  7 +++++--
> >  .../include/asm/mach-cavium-octeon/kernel-entry-init.h    | 15 
> > ++++++++++++++-
> >  3 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> > index b3c5bde..65d7e20 100644
> > --- a/arch/mips/Kconfig
> > +++ b/arch/mips/Kconfig
> > @@ -909,6 +909,7 @@ config CAVIUM_OCTEON_SOC
> >      select NR_CPUS_DEFAULT_16
> >      select BUILTIN_DTB
> >      select MTD_COMPLEX_MAPPINGS
> > +    select SYS_SUPPORTS_RELOCATABLE
> >      help
> >        This option supports all of the Octeon reference boards from 
> > Cavium
> >        Networks. It builds a kernel that dynamically determines the 
> > Octeon
> > @@ -2570,7 +2571,7 @@ config SYS_SUPPORTS_NUMA
> >
> >  config RELOCATABLE
> >      bool "Relocatable kernel"
> > -    depends on SYS_SUPPORTS_RELOCATABLE && (CPU_MIPS32_R2 || 
> > CPU_MIPS64_R2 || CPU_MIPS32_R6 || CPU_MIPS64_R6)
> > +    depends on SYS_SUPPORTS_RELOCATABLE && (CPU_MIPS32_R2 || 
> > CPU_MIPS64_R2 || CPU_MIPS32_R6 || CPU_MIPS64_R6 || CAVIUM_OCTEON_SOC)
> >      help
> >        This builds a kernel image that retains relocation information
> >        so it can be loaded someplace besides the default 1MB.
> > diff --git a/arch/mips/cavium-octeon/smp.c 
> > b/arch/mips/cavium-octeon/smp.c
> > index 256fe6f..65c8369 100644
> > --- a/arch/mips/cavium-octeon/smp.c
> > +++ b/arch/mips/cavium-octeon/smp.c
> > @@ -24,12 +24,17 @@
> >  volatile unsigned long octeon_processor_boot = 0xff;
> >  volatile unsigned long octeon_processor_sp;
> >  volatile unsigned long octeon_processor_gp;
> > +#ifdef CONFIG_RELOCATABLE
> > +volatile unsigned long octeon_processor_relocated_kernel_entry;
> > +#endif /* CONFIG_RELOCATABLE */
> >
> >  #ifdef CONFIG_HOTPLUG_CPU
> >  uint64_t octeon_bootloader_entry_addr;
> >  EXPORT_SYMBOL(octeon_bootloader_entry_addr);
> >  #endif
> >
> > +extern void kernel_entry(unsigned long arg1, ...);
> > +
> >  static void octeon_icache_flush(void)
> >  {
> >      asm volatile ("synci 0($0)\n");
> > @@ -333,8 +338,6 @@ void play_dead(void)
> >          ;
> >  }
> >
> > -extern void kernel_entry(unsigned long arg1, ...);
> > -
> >  static void start_after_reset(void)
> >  {
> >      kernel_entry(0, 0, 0);    /* set a2 = 0 for secondary core */
> > diff --git 
> > a/arch/mips/include/asm/mach-cavium-octeon/kernel-entry-init.h 
> > b/arch/mips/include/asm/mach-cavium-octeon/kernel-entry-init.h
> > index c4873e8..f69611c 100644
> > --- a/arch/mips/include/asm/mach-cavium-octeon/kernel-entry-init.h
> > +++ b/arch/mips/include/asm/mach-cavium-octeon/kernel-entry-init.h
> > @@ -99,9 +99,22 @@
> >      # to begin
> >      #
> >
> > +octeon_spin_wait_boot:
> > +#ifdef CONFIG_RELOCATABLE
> > +    PTR_LA    t0, octeon_processor_relocated_kernel_entry
> > +    LONG_L    t0, (t0)
> > +    beq    zero, t0, 1f
> > +
>
> This doesn't look correct.
>
> Please explain how this code implements:
>
>    loop here until octeon_processor_relocated_kernel_entry is set.

This part of the loop is checking if 
octeon_processor_relocated_kernel_entry becomes set, in which case don't 
take the branch and jump to the relocated kernel entry point that has 
been stored in octeon_processor_relocated_kernel_entry by the boot CPU 
(that part was missing from Stephens v1 post of this patch). Otherwise 
the semantics of the loop remain the same as before, checking for 
octeon_processor_boot to be non-zero to start a secondary running this 
kernel.

This same loop is executed in the relocated kernel and of course in that 
case octeon_processor_relocated_kernel_entry will not be set, just 
octeon_processor_boot will be set when the boot CPU is starting the 
secondary CPUs.

> > +    /* If kernel has been relocated, jump to it's new entry point */
> > +    move    a0, zero
> > +    move    a1, zero
> > +    move    a2, zero
>
> Why change the values of a*?  Don't these registers already contain 
> the proper values from the bootloader?

I don't see where the secondaries make use of any arguments passed from 
the bootloader in the entry code. But this follows the same semantics as 
the CPU hotplug code, which restarts secondaries with their arguments 
set to 0.

Thanks,
Matt

> > +    jr    t0
> > +1:
> > +#endif /* CONFIG_RELOCATABLE */
> > +
> >      # This is the variable where the next core to boot os stored
> >      PTR_LA    t0, octeon_processor_boot
> > -octeon_spin_wait_boot:
> >      # Get the core id of the next to be booted
> >      LONG_L    t1, (t0)
> >      # Keep looping if it isn't me