[Skiboot] [RFC PATCH 1/2] move the __this_cpu register to r16, reserve r13-r15
Oliver O'Halloran
oohall at gmail.com
Mon Dec 2 22:44:55 AEDT 2019
On Mon, Dec 2, 2019 at 6:46 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>
> There have been several bugs between Linux and OPAL caused by both
> using r13 for their primary per-CPU data address. This patch moves
> OPAL to use r16 for this, and prevents the compiler from touching
> r13-r15. This helps things to be a little more robust, and also makes
> crashes in OPAL easier to debug.
I'm ok with using r16 instead of r13 in skiboot, but what's the
significance of r14 and r15?
> Later, if we allow interrupts (other than non-maskable) to be taken when
> running in skiboot, Linux's interrupt return handler does not restore
> r13 if the interrupt was taken in PR=0 state, which would corrupt the
> skiboot r13 register.
What's the goal here? Is it just to avoid hard-disabling IRQs when we
make an OPAL call or do you have bigger plans? There's a lot of
potential for that to turn into a total trainwreck, but you're welcome
to try.
> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
> ---
> Makefile.main | 7 +++++--
> asm/head.S | 28 ++++++++++++++--------------
> asm/misc.S | 8 ++++----
> include/cpu.h | 2 +-
> 4 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/Makefile.main b/Makefile.main
> index e26a9ab17..527cd8ae9 100644
> --- a/Makefile.main
> +++ b/Makefile.main
> @@ -87,7 +87,10 @@ endif
> CFLAGS := -fno-strict-aliasing -pie -fpie -fno-pic -m64 -fno-asynchronous-unwind-tables
> CFLAGS += -mcpu=power8
> CFLAGS += -Wl,--oformat,elf64-powerpc -ggdb
> -CFLAGS += $(call try-cflag,$(CC),-ffixed-r13)
> +CFLAGS += -ffixed-r13
> +CFLAGS += -ffixed-r14
> +CFLAGS += -ffixed-r15
> +CFLAGS += -ffixed-r16
> CFLAGS += $(call try-cflag,$(CC),-std=gnu11)
>
> ifeq ($(LITTLE_ENDIAN),1)
> @@ -118,7 +121,7 @@ endif
>
> # Check if the new parametrized stack protector option is supported
> # by gcc, otherwise disable stack protector
> -STACK_PROT_CFLAGS := -mstack-protector-guard=tls -mstack-protector-guard-reg=r13
> +STACK_PROT_CFLAGS := -mstack-protector-guard=tls -mstack-protector-guard-reg=r16
> STACK_PROT_CFLAGS += -mstack-protector-guard-offset=0
> HAS_STACK_PROT := $(call test_cflag,$(CC),$(STACK_PROT_CFLAGS))
>
> diff --git a/asm/head.S b/asm/head.S
> index 0b4b1a5f0..825f4e74a 100644
> --- a/asm/head.S
> +++ b/asm/head.S
> @@ -25,7 +25,7 @@
> addi stack_reg,stack_reg,EMERGENCY_CPU_STACKS_OFFSET at l;
>
> #define GET_CPU() \
> - clrrdi %r13,%r1,STACK_SHIFT
> + clrrdi %r16,%r1,STACK_SHIFT
>
> #define SAVE_GPR(reg,sp) std %r##reg,STACK_GPR##reg(sp)
> #define REST_GPR(reg,sp) ld %r##reg,STACK_GPR##reg(sp)
> @@ -464,18 +464,18 @@ boot_entry:
> addi %r3,%r3,8
> bdnz 1b
>
> - /* Get our per-cpu pointer into r13 */
> + /* Get our per-cpu pointer into r16 */
> GET_CPU()
>
> #ifdef STACK_CHECK_ENABLED
> /* Initialize stack bottom mark to 0, it will be updated in C code */
> li %r0,0
> - std %r0,CPUTHREAD_STACK_BOT_MARK(%r13)
> + std %r0,CPUTHREAD_STACK_BOT_MARK(%r16)
> #endif
> /* Initialize the stack guard */
> LOAD_IMM64(%r3,STACK_CHECK_GUARD_BASE);
> xor %r3,%r3,%r31
> - std %r3,0(%r13)
> + std %r3,0(%r16)
>
> /* Jump to C */
> mr %r3,%r27
> @@ -592,12 +592,12 @@ reset_wakeup:
> /* Get PIR */
> mfspr %r31,SPR_PIR
>
> - /* Get that CPU stack base and use it to restore r13 */
> + /* Get that CPU stack base and use it to restore r16 */
> GET_STACK(%r1,%r31)
> GET_CPU()
>
> /* Restore original stack pointer */
> - ld %r1,CPUTHREAD_SAVE_R1(%r13)
> + ld %r1,CPUTHREAD_SAVE_R1(%r16)
>
> /* Restore more stuff */
> lwz %r4,STACK_CR(%r1)
> @@ -655,7 +655,7 @@ reset_fast_reboot_wakeup:
> /* Get PIR */
> mfspr %r31,SPR_PIR
>
> - /* Get that CPU stack base and use it to restore r13 */
> + /* Get that CPU stack base and use it to restore r16 */
> GET_STACK(%r1,%r31)
> GET_CPU()
>
> @@ -923,17 +923,17 @@ opal_entry:
> std %r9,STACK_GPR9(%r1)
> std %r10,STACK_GPR10(%r1)
>
> - /* Save Token (r0), LR and r13 */
> + /* Save Token (r0), LR and r16 */
> mflr %r12
> std %r0,STACK_GPR0(%r1)
> - std %r13,STACK_GPR13(%r1)
> + std %r16,STACK_GPR16(%r1)
> std %r12,STACK_LR(%r1)
>
> /* Get the CPU thread */
> GET_CPU()
>
> /* Store token in CPU thread */
> - std %r0,CPUTHREAD_CUR_TOKEN(%r13)
> + std %r0,CPUTHREAD_CUR_TOKEN(%r16)
>
> /* Mark the stack frame */
> li %r12,STACK_ENTRY_OPAL_API
> @@ -975,14 +975,14 @@ opal_entry:
> bl opal_exit_check /* r3 is preserved */
>
> /*
> - * Restore r1 and r13 before decrementing in_opal_call.
> - * Move per-cpu pointer to volatile r12, restore lr, r1, r13.
> + * Restore r1 and r16 before decrementing in_opal_call.
> + * Move per-cpu pointer to volatile r12, restore lr, r1, r16.
> */
> .Lreturn:
> ld %r12,STACK_LR(%r1)
> mtlr %r12
> - mr %r12,%r13
> - ld %r13,STACK_GPR13(%r1)
> + mr %r12,%r16
> + ld %r16,STACK_GPR16(%r1)
> ld %r1,STACK_GPR1(%r1)
> .Lreject:
> sync /* release barrier vs quiescing */
> diff --git a/asm/misc.S b/asm/misc.S
> index 647f60b26..9904b806f 100644
> --- a/asm/misc.S
> +++ b/asm/misc.S
> @@ -213,7 +213,7 @@ enter_p8_pm_state:
> bl pm_save_regs
>
> /* Save stack pointer in struct cpu_thread */
> - std %r1,CPUTHREAD_SAVE_R1(%r13)
> + std %r1,CPUTHREAD_SAVE_R1(%r16)
>
> /* Winkle or nap ? */
> cmpli %cr0,0,%r3,0
> @@ -221,7 +221,7 @@ enter_p8_pm_state:
>
> /* nap sequence */
> ptesync
> -0: ld %r0,CPUTHREAD_SAVE_R1(%r13)
> +0: ld %r0,CPUTHREAD_SAVE_R1(%r16)
> cmpd cr0,%r0,%r0
> bne 0b
> PPC_INST_NAP
> @@ -229,7 +229,7 @@ enter_p8_pm_state:
>
> /* rvwinkle sequence */
> 1: ptesync
> -0: ld %r0,CPUTHREAD_SAVE_R1(%r13)
> +0: ld %r0,CPUTHREAD_SAVE_R1(%r16)
> cmpd cr0,%r0,%r0
> bne 0b
> PPC_INST_RVWINKLE
> @@ -250,7 +250,7 @@ enter_p9_pm_state:
> bl pm_save_regs
>
> /* Save stack pointer in struct cpu_thread */
> - std %r1,CPUTHREAD_SAVE_R1(%r13)
> + std %r1,CPUTHREAD_SAVE_R1(%r16)
>
> mtspr SPR_PSSCR,%r3
> PPC_INST_STOP
> diff --git a/include/cpu.h b/include/cpu.h
> index 686310d71..9b7f41dfb 100644
> --- a/include/cpu.h
> +++ b/include/cpu.h
> @@ -212,7 +212,7 @@ extern u8 get_available_nr_cores_in_chip(u32 chip_id);
> core = next_available_core_in_chip(core, chip_id))
>
> /* Return the caller CPU (only after init_cpu_threads) */
> -register struct cpu_thread *__this_cpu asm("r13");
> +register struct cpu_thread *__this_cpu asm("r16");
> static inline __nomcount struct cpu_thread *this_cpu(void)
> {
> return __this_cpu;
> --
> 2.23.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list