| 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
| ||
|
Message-ID: <a3890ccb-e948-6ad6-c2ea-5b77b9d3a289@synopsys.com>
Date: Tue, 7 Jan 2020 18:25:57 +0000
From: Vineet Gupta <Vineet.Gupta1@...opsys.com>
To: Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
"linux-snps-arc@...ts.infradead.org"
<linux-snps-arc@...ts.infradead.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Alexey Brodkin" <Alexey.Brodkin@...opsys.com>
Subject: Re: [PATCH 4/5] ARC: add support for DSP-enabled userspace
applications
On 12/27/19 10:03 AM, Eugeniy Paltsev wrote:
> To be able to run DSP-enabled userspace applications we need to
> save and restore following DSP-related registers:
> At IRQ/exception entry/exit:
> * ACC0_GLO, ACC0_GHI, DSP_CTRL
> * ACC0_LO, ACC0_HI (we already save them as r58, r59 pair)
> At context switch:
> * DSP_BFLY0, DSP_FFT_CTRL
Good summary: but the question is this more than needed.
For kernel proper, you've disabled guard bits, saturation etc already. AFAIKS gcc
won't generate complex/fractional math for kernel so your bullet #1 can likely be
considered user space only hence can go to bullet #3. Do you have reasons to
believe otherwise ?
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>
> ---
> arch/arc/Kconfig | 7 +++
> arch/arc/include/asm/arcregs.h | 2 +
> arch/arc/include/asm/dsp-impl.h | 75 +++++++++++++++++++++++++++++-
> arch/arc/include/asm/dsp.h | 42 +++++++++++++++++
> arch/arc/include/asm/entry-arcv2.h | 3 ++
> arch/arc/include/asm/processor.h | 4 ++
> arch/arc/include/asm/ptrace.h | 4 ++
> arch/arc/include/asm/switch_to.h | 2 +
> arch/arc/kernel/asm-offsets.c | 7 +++
> arch/arc/kernel/setup.c | 2 +-
> 10 files changed, 146 insertions(+), 2 deletions(-)
> create mode 100644 arch/arc/include/asm/dsp.h
>
> diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
> index b9cd7ce3f878..c3210754a3d2 100644
> --- a/arch/arc/Kconfig
> +++ b/arch/arc/Kconfig
> @@ -432,6 +432,13 @@ config ARC_DSP_KERNEL
> DSP extension presence in HW, no support for DSP-enabled userspace
> applications. We don't save / restore DSP registers and only do
> some minimal preparations so userspace won't be able to break kernel
> +
> +config ARC_DSP_USERSPACE
> + bool "Support DSP for userspace apps"
> + select ARC_HAS_ACCL_REGS
> + help
> + DSP extension presence in HW, support save / restore DSP registers to
> + run DSP-enabled userspace applications
> endchoice
>
> config ARC_IRQ_NO_AUTOSAVE
> diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
> index 0004b1e9b325..a713819cab3c 100644
> --- a/arch/arc/include/asm/arcregs.h
> +++ b/arch/arc/include/asm/arcregs.h
> @@ -118,6 +118,8 @@
>
> /*
> * DSP-related registers
> + * Registers names must correspond to dsp_callee_regs structure fields names
> + * for automatic offset calculation in DSP_AUX_SAVE_RESTORE macros.
good idea for preventing carbon errors.
> */
> #define ARC_AUX_DSP_BUILD 0x7A
> #define ARC_AUX_ACC0_LO 0x580
> diff --git a/arch/arc/include/asm/dsp-impl.h b/arch/arc/include/asm/dsp-impl.h
> index 788093cbe689..7b640a680dfc 100644
> --- a/arch/arc/include/asm/dsp-impl.h
> +++ b/arch/arc/include/asm/dsp-impl.h
> @@ -7,6 +7,8 @@
> #ifndef __ASM_ARC_DSP_IMPL_H
> #define __ASM_ARC_DSP_IMPL_H
>
> +#include <asm/dsp.h>
> +
> #define DSP_CTRL_DISABLED_ALL 0
>
> #ifdef __ASSEMBLY__
> @@ -28,11 +30,82 @@
> * able to break kernel */
> mov r58, DSP_CTRL_DISABLED_ALL
> sr r58, [ARC_AUX_DSP_CTRL]
> -#endif /* ARC_DSP_KERNEL */
> +
> +#elif defined(ARC_DSP_SAVE_RESTORE_REGS)
> + lr r58, [ARC_AUX_ACC0_GLO]
> + lr r59, [ARC_AUX_ACC0_GHI]
> + ST2 r58, r59, PT_ACC0_GLO
> +
> + lr r58, [ARC_AUX_DSP_CTRL]
> + st r58, [sp, PT_DSP_CTRL]
> +
> +#endif
> +.endm
> +
> +/* clobbers r58, r59 registers pair */
> +.macro DSP_RESTORE_REGFILE_IRQ
> +#if defined(ARC_DSP_SAVE_RESTORE_REGS)
> + LD2 r58, r59, PT_ACC0_GLO
> + sr r58, [ARC_AUX_ACC0_GLO]
> + sr r59, [ARC_AUX_ACC0_GHI]
> +
> + ld r58, [sp, PT_DSP_CTRL]
> + sr r58, [ARC_AUX_DSP_CTRL]
> +
> +#endif
Assuming you still need this, consider using different scratch registers if
possible. I understand it gets tricky in restore path but there even more
registers are available to clobber as they will be restored after this code.
> +#ifdef ARC_DSP_SAVE_RESTORE_REGS
> +
> +/*
> + * As we save new and restore old AUX register value in the same place we
> + * can optimize a bit and use AEX instruction (swap contents of an auxiliary
> + * register with a core register) instead of LR + SR pair.
> + */
> +#define AUX_SAVE_RESTORE(_saveto, _readfrom, _offt, _aux, _scratch) \
> +do { \
> + __asm__ __volatile__( \
> + "ld %0, [%2, %4] \n" \
> + "aex %0, [%3] \n" \
> + "st %0, [%1, %4] \n" \
> + : \
> + "=&r" (_scratch) /* must be early clobber */ \
Define the scratch variable locally for clarity and better liveness tracking - for
both code reader and compiler :-)
Also avoids having to initialize it.
> + : \
> + "r" (_saveto), \
> + "r" (_readfrom), \
> + "I" (_aux), \
> + "I" (_offt) \
> + : \
AEX with "I" constraint will likely be an 8 byte instructions always. Best to give
compiler wiggle room with "Ir"
> + "memory" \
> + ); \
> +} while (0)
> +
> +#define DSP_AUX_SAVE_RESTORE(_saveto, _readfrom, _aux, _scratch) \
> + AUX_SAVE_RESTORE(_saveto, _readfrom, \
> + offsetof(struct dsp_callee_regs, _aux), \
> + ARC_AUX_##_aux, _scratch)
> +
> +static inline void arc_dsp_save_restore(struct task_struct *prev,
> + struct task_struct *next)
> +{
> + long unsigned int *saveto = &prev->thread.dsp.DSP_BFLY0;
> + long unsigned int *readfrom = &next->thread.dsp.DSP_BFLY0;
> +
> + long unsigned int zero = 0;
See comment about pushing scratch to the relevant code block.
> +
> + DSP_AUX_SAVE_RESTORE(saveto, readfrom, DSP_BFLY0, zero);
> + DSP_AUX_SAVE_RESTORE(saveto, readfrom, DSP_FFT_CTRL, zero);
> +}
> +
[snip]
> +
> +#if defined(CONFIG_ARC_DSP_USERSPACE)
> +#define ARC_DSP_SAVE_RESTORE_REGS 1
> +#endif
I can see u added this for consistency with below which is a really bad idea: see
below.
> +
> +#ifndef __ASSEMBLY__
> +
> +/* some defines to simplify config sanitize in kernel/setup.c */
> +#if defined(CONFIG_ARC_DSP_KERNEL) || \
> + defined(CONFIG_ARC_DSP_USERSPACE)
> +#define ARC_DSP_HANDLED 1
> +#else
> +#define ARC_DSP_HANDLED 0
> +#endif
This is a really bad idea - u r introducing explicit include dependencies which
can change even outside of arch changes !
We've dealt with enough of these problems with current.h, so best to avoid, even
if there is some code clutter.
> +
> +#if defined(CONFIG_ARC_DSP_USERSPACE)
> +#define ARC_DSP_OPT_NAME "CONFIG_ARC_DSP_USERSPACE"
> +#else
> +#define ARC_DSP_OPT_NAME "CONFIG_ARC_DSP_KERNEL"
> +#endif
> +
> +/*
> + * DSP-related saved registers - need to be saved only when you are
> + * scheduled out.
> + * structure fields name must correspond to aux register defenitions for
> + * automatic offset calculation in DSP_AUX_SAVE_RESTORE macros
> + */
> +struct dsp_callee_regs {
> + unsigned long DSP_BFLY0, DSP_FFT_CTRL;
> +};
> +
> +#endif /* !__ASSEMBLY__ */
> +
> +#endif /* __ASM_ARC_DSP_H */
> diff --git a/arch/arc/include/asm/entry-arcv2.h b/arch/arc/include/asm/entry-arcv2.h
> index e3f8bd3e2eba..5d079f0b6243 100644
> --- a/arch/arc/include/asm/entry-arcv2.h
> +++ b/arch/arc/include/asm/entry-arcv2.h
> @@ -192,6 +192,9 @@
> ld r25, [sp, PT_user_r25]
> #endif
>
> + /* clobbers r58, r59 registers pair, so must be before r58, r59 restore */
> + DSP_RESTORE_REGFILE_IRQ
> +
> #ifdef CONFIG_ARC_HAS_ACCL_REGS
> LD2 r58, r59, PT_r58
> #endif
> diff --git a/arch/arc/include/asm/processor.h b/arch/arc/include/asm/processor.h
> index 706edeaa5583..1716df0f4472 100644
> --- a/arch/arc/include/asm/processor.h
> +++ b/arch/arc/include/asm/processor.h
> @@ -14,6 +14,7 @@
> #ifndef __ASSEMBLY__
>
> #include <asm/ptrace.h>
> +#include <asm/dsp.h>
>
> #ifdef CONFIG_ARC_FPU_SAVE_RESTORE
> /* These DPFP regs need to be saved/restored across ctx-sw */
> @@ -39,6 +40,9 @@ struct thread_struct {
> unsigned long ksp; /* kernel mode stack pointer */
> unsigned long callee_reg; /* pointer to callee regs */
> unsigned long fault_address; /* dbls as brkpt holder as well */
> +#ifdef ARC_DSP_SAVE_RESTORE_REGS
> + struct dsp_callee_regs dsp;
> +#endif
> #ifdef CONFIG_ARC_FPU_SAVE_RESTORE
> struct arc_fpu fpu;
> #endif
> diff --git a/arch/arc/include/asm/ptrace.h b/arch/arc/include/asm/ptrace.h
> index ba9854ef39e8..a5851ee141de 100644
> --- a/arch/arc/include/asm/ptrace.h
> +++ b/arch/arc/include/asm/ptrace.h
> @@ -8,6 +8,7 @@
> #define __ASM_ARC_PTRACE_H
>
> #include <uapi/asm/ptrace.h>
> +#include <asm/dsp.h>
>
> #ifndef __ASSEMBLY__
>
> @@ -91,6 +92,9 @@ struct pt_regs {
> #ifdef CONFIG_ARC_HAS_ACCL_REGS
> unsigned long r58, r59; /* ACCL/ACCH used by FPU / DSP MPY */
> #endif
> +#ifdef ARC_DSP_SAVE_RESTORE_REGS
Use the Kconfig option name directly or !CONFIG_NO_DSP etc
> + unsigned long ACC0_GLO, ACC0_GHI, DSP_CTRL;
> +#endif
see comments at top.
>
> /*------- Below list auto saved by h/w -----------*/
> unsigned long r0, r1, r2, r3, r4, r5, r6, r7, r8, r9, r10, r11;
[snip]
> }
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index b3995dd673d9..30d31579c51d 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -447,7 +447,7 @@ static void arc_chk_core_config(void)
> CHK_OPT_STRICT(CONFIG_ARC_HAS_ACCL_REGS, present);
>
> present = dsp_exist();
> - CHK_OPT_STRICT(CONFIG_ARC_DSP_KERNEL, present);
> + chk_opt_strict(ARC_DSP_OPT_NAME, present, ARC_DSP_HANDLED);
This needs to be reworked given the header fudge is not a good idea.
Powered by blists - more mailing lists