[PATCH v2 04/12] powerpc/dexcr: Handle hashchk exception

[Russell Currey]

On Thu, 2023-03-30 at 16:50 +1100, Benjamin Gray wrote:
> Recognise and pass the appropriate signal to the user program when a
> hashchk instruction triggers. This is independent of allowing
> configuration of DEXCR[NPHIE], as a hypervisor can enforce this
> aspect
> regardless of the kernel.
> 
> The signal mirrors how ARM reports their similar check failure. For
> example, their FPAC handler in arch/arm64/kernel/traps.c
> do_el0_fpac()
> does this. When we fail to read the instruction that caused the fault
> we send a segfault, similar to how emulate_math() does it.
> 
> Signed-off-by: Benjamin Gray <bgray at linux.ibm.com>
> 
> ---
> 
> v1:     * Refactor the hashchk check to return 0 on success, an error
>           code on failure. Determine what to do based on specific
> error
>           code.

0 on success makes sense, but it's not exactly obvious what "success"
is in this case.

>         * Motivate signal and code
> ---
>  arch/powerpc/include/asm/ppc-opcode.h |  1 +
>  arch/powerpc/include/asm/processor.h  |  9 +++++++
>  arch/powerpc/kernel/Makefile          |  1 +
>  arch/powerpc/kernel/dexcr.c           | 36
> +++++++++++++++++++++++++++
>  arch/powerpc/kernel/traps.c           | 10 ++++++++
>  5 files changed, 57 insertions(+)
>  create mode 100644 arch/powerpc/kernel/dexcr.c
> 
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> b/arch/powerpc/include/asm/ppc-opcode.h
> index 21e33e46f4b8..89b316466ed1 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -215,6 +215,7 @@
>  #define OP_31_XOP_STFSX            663
>  #define OP_31_XOP_STFSUX    695
>  #define OP_31_XOP_STFDX     727
> +#define OP_31_XOP_HASHCHK   754
>  #define OP_31_XOP_STFDUX    759
>  #define OP_31_XOP_LHBRX     790
>  #define OP_31_XOP_LFIWAX    855
> diff --git a/arch/powerpc/include/asm/processor.h
> b/arch/powerpc/include/asm/processor.h
> index e96c9b8c2a60..bad64d6a5d36 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -455,6 +455,15 @@ int exit_vmx_usercopy(void);
>  int enter_vmx_ops(void);
>  void *exit_vmx_ops(void *dest);
>  
> +#ifdef CONFIG_PPC_BOOK3S_64
> +int check_hashchk_trap(struct pt_regs const *regs);
> +#else
> +static inline int check_hashchk_trap(struct pt_regs const *regs)
> +{
> +       return -EINVAL;
> +}
> +#endif /* CONFIG_PPC_BOOK3S_64 */
> +
>  #endif /* __KERNEL__ */
>  #endif /* __ASSEMBLY__ */
>  #endif /* _ASM_POWERPC_PROCESSOR_H */
> diff --git a/arch/powerpc/kernel/Makefile
> b/arch/powerpc/kernel/Makefile
> index 9bf2be123093..07181e508754 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -88,6 +88,7 @@ obj-$(CONFIG_HAVE_HW_BREAKPOINT)      +=
> hw_breakpoint.o
>  obj-$(CONFIG_PPC_DAWR)         += dawr.o
>  obj-$(CONFIG_PPC_BOOK3S_64)    += cpu_setup_ppc970.o
> cpu_setup_pa6t.o
>  obj-$(CONFIG_PPC_BOOK3S_64)    += cpu_setup_power.o
> +obj-$(CONFIG_PPC_BOOK3S_64)    += dexcr.o
>  obj-$(CONFIG_PPC_BOOK3S_64)    += mce.o mce_power.o
>  obj-$(CONFIG_PPC_BOOK3E_64)    += exceptions-64e.o idle_64e.o
>  obj-$(CONFIG_PPC_BARRIER_NOSPEC) += security.o
> diff --git a/arch/powerpc/kernel/dexcr.c
> b/arch/powerpc/kernel/dexcr.c
> new file mode 100644
> index 000000000000..f263e5439cc6
> --- /dev/null
> +++ b/arch/powerpc/kernel/dexcr.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * DEXCR infrastructure

May as well spell out DEXCR here

> + *
> + * Copyright 2023, Benjamin Gray, IBM Corporation.
> + */
> +#include <linux/compiler_types.h>
> +
> +#include <asm/cpu_has_feature.h>
> +#include <asm/cputable.h>
> +#include <asm/disassemble.h>
> +#include <asm/errno.h>
> +#include <asm/inst.h>
> +#include <asm/ppc-opcode.h>
> +#include <asm/ptrace.h>
> +#include <asm/reg.h>
> +
> +int check_hashchk_trap(struct pt_regs const *regs)
> +{
> +       ppc_inst_t insn;
> +
> +       if (!cpu_has_feature(CPU_FTR_DEXCR_NPHIE))
> +               return -EINVAL;
> +
> +       if (!user_mode(regs))
> +               return -EINVAL;
> +
> +       if (get_user_instr(insn, (void __user *)regs->nip))
> +               return -EFAULT;
> +
> +       if (ppc_inst_primary_opcode(insn) != 31 ||
> +           get_xop(ppc_inst_val(insn)) != OP_31_XOP_HASHCHK)
> +               return -EINVAL;
> +
> +       return 0;
> +}

The return values here are quite confusing and only start to make sense
in the context of the calling function, which isn't great for something
living in a different file.  So if we return 0 we SIGILL because of
hashchk, return -EFAULT to SEGV_MAPERR and return -EINVAL to fall
through to SIGTRAP.  I would like a comment for that since it's not
very intuitive.

> diff --git a/arch/powerpc/kernel/traps.c
> b/arch/powerpc/kernel/traps.c
> index 9bdd79aa51cf..ade67e23b974 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1516,6 +1516,16 @@ static void do_program_check(struct pt_regs
> *regs)
>                                 return;
>                         }
>                 }
> +
> +               switch (check_hashchk_trap(regs)) {
> +               case 0:
> +                       _exception(SIGILL, regs, ILL_ILLOPN, regs-
> >nip);
> +                       return;
> +               case -EFAULT:
> +                       _exception(SIGSEGV, regs, SEGV_MAPERR, regs-
> >nip);
> +                       return;
> +               }
> +

I offer no constructive feedback on how to rearrange it, but this just
looks gross.

>                 _exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
>                 return;
>         }