Re: [PATCH v3 09/13] arm64: kernel: Add arch-specific SDEI entry code and CPU masking

[Catalin Marinas]

On Tue, Oct 17, 2017 at 05:34:13PM +0100, James Morse wrote:
> On 16/10/17 14:41, Catalin Marinas wrote:
> > On Fri, Sep 22, 2017 at 07:26:10PM +0100, James Morse wrote:
> >> +  u64 elr = read_sysreg(elr_el1);
> >> +  u32 kernel_mode = read_sysreg(CurrentEL) | 1;   /* +SPSel */
> >> +  unsigned long vbar = read_sysreg(vbar_el1);
> >> +
> >> +  /* Retrieve the missing registers values */
> >> +  for (i = 0; i < clobbered_registers; i++) {
> >> +  /* from within the handler, this call always succeeds */
> >> +  sdei_api_event_context(i, >regs[i]);
> >> +  }
> >> +
> >> +  /*
> >> +   * We didn't take an exception to get here, set PAN. UAO will be cleared
> >> +   * by sdei_event_handler()s set_fs(USER_DS) call.
> >> +   */
> >> +  asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,
> >> +  CONFIG_ARM64_PAN));
> 
> > Can you use uaccess_disable() directly?
> 
> Wouldn't this invoke sw-pan's __uaccess_ttbr0_disable():
> > write_sysreg(ttbr, ttbr0_el1);
> 
> Probing depends on VHE if booted at EL2, can we guarantee to have PAN (and
> therefore not-SW-PAN) too?
> 
> (otherwise I can add 'depends on !ARM64_SW_TTBR0_PAN' to the Kconfig)

We want the Kconfig to be able to include all features. What you can do
though is:

select ARM64_PAN if ARM64_SW_TTBR0_PAN

With VHE (ARMv8.2) we can guarantee that hardware PAN is around.

-- 
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 09/13] arm64: kernel: Add arch-specific SDEI entry code and CPU masking

[James Morse]

Hi Catalin,

On 16/10/17 14:41, Catalin Marinas wrote:
> On Fri, Sep 22, 2017 at 07:26:10PM +0100, James Morse wrote:
>> diff --git a/arch/arm64/kernel/sdei-entry.S b/arch/arm64/kernel/sdei-entry.S
>> new file mode 100644
>> index ..cf12f8da0789
>> --- /dev/null
>> +++ b/arch/arm64/kernel/sdei-entry.S
>> @@ -0,0 +1,122 @@
>> +/*
>> + * Software Delegated Exception entry point from firmware to the kernel.
>> + *
>> + * Copyright (C) 2017 ARM Ltd.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see .
>> + */
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * Software Delegated Exception entry point.
>> + *
>> + * x0: Event number
> 
> Currently the only event number is 0. Shall we plan for having other
> events in the future or we just ignore this argument?

'0' is the only event number specified by the SDEI specification. For use with
RAS the event number is read from the HEST. (and there may me more than one of
them).



>> + * x1: struct sdei_registered_event argument from registration time.
>> + * x2: interrupted PC
>> + * x3: interrupted PSTATE
> [...]
>> +/*
>> + * __sdei_handler() returns one of:
>> + *  SDEI_EV_HANDLED -  success, return to the interrupted context.
>> + *  SDEI_EV_FAILED  -  failure, return this error code to firmare.
>> + *  virtual-address -  success, return to this address.
>> + */
>> +static __kprobes unsigned long _sdei_handler(struct pt_regs *regs,
>> + struct sdei_registered_event *arg)
>> +{
>> +u32 mode;
>> +int i, err = 0;
>> +int clobbered_registers = 4;

> Maybe const int here if you want to avoid magic numbers in the 'for'
> loop below. Otherwise it looks like some variable you intend to modify
> in this function.

Sure.

(this was modified in my various attempts to get this working on non-vhe systems
with KVM).


>> +u64 elr = read_sysreg(elr_el1);
>> +u32 kernel_mode = read_sysreg(CurrentEL) | 1;   /* +SPSel */
>> +unsigned long vbar = read_sysreg(vbar_el1);
>> +
>> +/* Retrieve the missing registers values */
>> +for (i = 0; i < clobbered_registers; i++) {
>> +/* from within the handler, this call always succeeds */
>> +sdei_api_event_context(i, >regs[i]);
>> +}
>> +
>> +/*
>> + * We didn't take an exception to get here, set PAN. UAO will be cleared
>> + * by sdei_event_handler()s set_fs(USER_DS) call.
>> + */
>> +asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,
>> +CONFIG_ARM64_PAN));

> Can you use uaccess_disable() directly?

Wouldn't this invoke sw-pan's __uaccess_ttbr0_disable():
> write_sysreg(ttbr, ttbr0_el1);

Probing depends on VHE if booted at EL2, can we guarantee to have PAN (and
therefore not-SW-PAN) too?

(otherwise I can add 'depends on !ARM64_SW_TTBR0_PAN' to the Kconfig)


>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index d8a9859f6102..1f6633b337aa 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -50,6 +50,7 @@ config ARM_SCPI_POWER_DOMAIN
>>  
>>  config ARM_SDE_INTERFACE
>>  bool "ARM Software Delegated Exception Interface (SDEI)"
>> +depends on ARM64
>>  help
>>The Software Delegated Exception Interface (SDEI) is an ARM
>>standard for registering callbacks from the platform firmware
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index 4b3c7fd99c34..3ea6a19ae439 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -154,6 +154,7 @@ int sdei_api_event_context(u32 query, u64 *result)
>>  return invoke_sdei_fn(SDEI_1_0_FN_SDEI_EVENT_CONTEXT, query, 0, 0, 0, 0,
>>result);
>>  }
>> +NOKPROBE_SYMBOL(sdei_api_event_context);

> Should this be part of the patch introducing arm_sdei.c?

Yes.


>>  static int sdei_api_event_get_info(u32 event, u32 info, u64 *result)
>>  {
>> diff --git a/include/linux/sdei.h b/include/linux/sdei.h
>> index bb3dd000771e..df3fe6373e32 100644
>> --- a/include/linux/sdei.h
>> +++ b/include/linux/sdei.h
>> @@ -32,6 +32,8 @@ enum sdei_conduit_types {
>>  CONDUIT_HVC,
>>  };
>>  
>> +#include 
>> +
>>  /* Arch code should override this to set the entry point from firmware... */
>>  #ifndef sdei_arch_get_entry_point
>>  #define 

Re: [PATCH v3 09/13] arm64: kernel: Add arch-specific SDEI entry code and CPU masking

[Catalin Marinas]

On Fri, Sep 22, 2017 at 07:26:10PM +0100, James Morse wrote:
> diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
> new file mode 100644
> index ..ed329e01a301
> --- /dev/null
> +++ b/arch/arm64/include/asm/sdei.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2017 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +#ifndef __ASM_SDEI_H
> +#define __ASM_SDEI_H
> +
> +/* Values for sdei_exit_mode */
> +#define SDEI_EXIT_HVC  0
> +#define SDEI_EXIT_SMC  1
> +
> +#define SDEI_STACK_SIZE  IRQ_STACK_SIZE
> +
> +#ifndef __ASSEMBLY__
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +extern unsigned long sdei_exit_mode;
> +
> +/* Software Delegated Exception entry point from firmware*/
> +asmlinkage void __sdei_asm_handler(unsigned long event_num, unsigned long 
> arg,
> +unsigned long pc, unsigned long pstate);
> +
> +/*
> + * The above entry point does the minimum to call C code. This function does
> + * anything else, before calling the driver.
> + */
> +struct sdei_registered_event;
> +asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
> + struct sdei_registered_event *arg);
> +
> +extern unsigned long sdei_arch_get_entry_point(int conduit);

Nitpick: drop the "extern" here.

> diff --git a/arch/arm64/kernel/sdei-entry.S b/arch/arm64/kernel/sdei-entry.S
> new file mode 100644
> index ..cf12f8da0789
> --- /dev/null
> +++ b/arch/arm64/kernel/sdei-entry.S
> @@ -0,0 +1,122 @@
> +/*
> + * Software Delegated Exception entry point from firmware to the kernel.
> + *
> + * Copyright (C) 2017 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Software Delegated Exception entry point.
> + *
> + * x0: Event number

Currently the only event number is 0. Shall we plan for having other
events in the future or we just ignore this argument?

> + * x1: struct sdei_registered_event argument from registration time.
> + * x2: interrupted PC
> + * x3: interrupted PSTATE
[...]
> +/*
> + * __sdei_handler() returns one of:
> + *  SDEI_EV_HANDLED -  success, return to the interrupted context.
> + *  SDEI_EV_FAILED  -  failure, return this error code to firmare.
> + *  virtual-address -  success, return to this address.
> + */
> +static __kprobes unsigned long _sdei_handler(struct pt_regs *regs,
> +  struct sdei_registered_event *arg)
> +{
> + u32 mode;
> + int i, err = 0;
> + int clobbered_registers = 4;

Maybe const int here if you want to avoid magic numbers in the 'for'
loop below. Otherwise it looks like some variable you intend to modify
in this function.

> + u64 elr = read_sysreg(elr_el1);
> + u32 kernel_mode = read_sysreg(CurrentEL) | 1;   /* +SPSel */
> + unsigned long vbar = read_sysreg(vbar_el1);
> +
> + /* Retrieve the missing registers values */
> + for (i = 0; i < clobbered_registers; i++) {
> + /* from within the handler, this call always succeeds */
> + sdei_api_event_context(i, >regs[i]);
> + }
> +
> + /*
> +  * We didn't take an exception to get here, set PAN. UAO will be cleared
> +  * by sdei_event_handler()s set_fs(USER_DS) call.
> +  */
> + asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,
> + CONFIG_ARM64_PAN));

Can you use uaccess_disable() directly?

> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index d8a9859f6102..1f6633b337aa 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -50,6 +50,7 @@ config ARM_SCPI_POWER_DOMAIN
>  
>  config ARM_SDE_INTERFACE
>   bool "ARM Software Delegated Exception 

[PATCH v3 09/13] arm64: kernel: Add arch-specific SDEI entry code and CPU masking

[James Morse]

The Software Delegated Exception Interface (SDEI) is an ARM standard
for registering callbacks from the platform firmware into the OS.
This is typically used to implement RAS notifications.

Such notifications enter the kernel at the registered entry-point
with the register values of the interrupted CPU context. Because this
is not a CPU exception, it cannot reuse the existing entry code.
(crucially we don't implicitly know which exception level we interrupted),

Add an sdei-entry.S asm file to set us up for calling into C code. If
the event interrupted code that had interrupts masked, we always return
to that location. Otherwise we pretend this was an IRQ, and use SDEI's
complete_and_resume call to return to vbar_el1 + offset.

This allows the kernel to deliver signals to user space processes. For
KVM this triggers the world switch, a quick spin round vcpu_run, then
back into the guest, unless there are pending signals.

Add sdei_mask_local_cpu() calls to the smp_send_stop() code, this covers
the panic() code-path, which doesn't invoke cpuhotplug notifiers.

Because we can interrupt entry-from/exit-to another EL, we can't trust the
value in sp_el0 or x29, even if we interrupted the kernel, in this case
the code in entry.S will save/restore sp_el0 and use the value in
__entry_task.

When we have VMAP stacks we can interrupt the stack-overflow test, which
stirs x0 into sp, meaning we have to have our own VMAP stack.

Signed-off-by: James Morse 
---
Changes since v2:
 * Added PAN-setting as we didn't take an exception to get here.
 * Added VMAP stack allocation and switching.
 * overwrite x29 on entry from not-the-kernel.
 * Added a pe-mask to crash_smp_send_stop()s 'no secondaries' path (oops).
 * Added ELR-hasn't-changed test. Any exception in the handler will panic KVM
   as its switched VBAR_EL1, but go silently undetected otherwise. Generate a
   warning.

 arch/arm64/Kconfig  |   2 +-
 arch/arm64/include/asm/sdei.h   |  63 ++
 arch/arm64/include/asm/stacktrace.h |   3 +
 arch/arm64/kernel/Makefile  |   1 +
 arch/arm64/kernel/asm-offsets.c |   5 +
 arch/arm64/kernel/sdei-entry.S  | 122 +++
 arch/arm64/kernel/sdei.c| 235 
 arch/arm64/kernel/smp.c |  11 +-
 drivers/firmware/Kconfig|   1 +
 drivers/firmware/arm_sdei.c |   1 +
 include/linux/sdei.h|   2 +
 11 files changed, 444 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/include/asm/sdei.h
 create mode 100644 arch/arm64/kernel/sdei-entry.S
 create mode 100644 arch/arm64/kernel/sdei.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0df64a6a56d4..70dfe4e9ccc5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -98,7 +98,7 @@ config ARM64
select HAVE_IRQ_TIME_ACCOUNTING
select HAVE_MEMBLOCK
select HAVE_MEMBLOCK_NODE_MAP if NUMA
-   select HAVE_NMI if ACPI_APEI_SEA
+   select HAVE_NMI
select HAVE_PATA_PLATFORM
select HAVE_PERF_EVENTS
select HAVE_PERF_REGS
diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
new file mode 100644
index ..ed329e01a301
--- /dev/null
+++ b/arch/arm64/include/asm/sdei.h
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2017 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+#ifndef __ASM_SDEI_H
+#define __ASM_SDEI_H
+
+/* Values for sdei_exit_mode */
+#define SDEI_EXIT_HVC  0
+#define SDEI_EXIT_SMC  1
+
+#define SDEI_STACK_SIZEIRQ_STACK_SIZE
+
+#ifndef __ASSEMBLY__
+
+#include 
+#include 
+
+#include 
+
+extern unsigned long sdei_exit_mode;
+
+/* Software Delegated Exception entry point from firmware*/
+asmlinkage void __sdei_asm_handler(unsigned long event_num, unsigned long arg,
+  unsigned long pc, unsigned long pstate);
+
+/*
+ * The above entry point does the minimum to call C code. This function does
+ * anything else, before calling the driver.
+ */
+struct sdei_registered_event;
+asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
+   struct sdei_registered_event *arg);
+
+extern unsigned long sdei_arch_get_entry_point(int conduit);
+#define sdei_arch_get_entry_point(x)   sdei_arch_get_entry_point(x)
+
+bool _on_sdei_stack(unsigned long sp);
+static inline bool on_sdei_stack(unsigned long sp)
+{