On Thu, Jan 02, 2020 at 06:11:21PM +0000, Andre Przywara wrote:
> On Tue, 31 Dec 2019 16:09:37 +0000
> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
>
> Hi,
>
> > The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
> > done by setting the entry point for the CPU_ON call to the physical address
> > of the C function cpu_psci_cpu_die.
> >
> > The compiler is well within its rights to use the stack when generating
> > code for cpu_psci_cpu_die.
>
> I am a bit puzzled: Is this an actual test failure at the moment? Or just a potential problem? Because I see it using the stack pointer in the generated code in lib/arm/psci.o. But the psci test seems to pass. Or is that just because the SP is somehow not cleared, because of some KVM implementation specifics?
I think the test just doesn't care that the CPU is in an infinite
exception loop. Indeed we should probably put the CPU into an
infinite loop instead of attempting to call PSCI die with it, as
the status of a dying or dead CPU may conflict with our expected
status of the test.
>
> One more thing below ...
>
> > However, because no stack initialization has
> > been done, the stack pointer is zero, as set by KVM when creating the VCPU.
> > This causes a data abort without a change in exception level. The VBAR_EL1
> > register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
> > and we end up trying to fetch instructions from address 0x200.
> >
> > At this point, a stage 2 instruction abort is generated which is taken to
> > KVM. KVM interprets this as an instruction fetch from an I/O region, and
> > injects a prefetch abort into the guest. Prefetch abort is a synchronous
> > exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
> > which is... 0x200. The VCPU ends up in an infinite loop causing a prefetch
> > abort while fetching the instruction to service the said abort.
> >
> > cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
> > provide an assembly implementation for the function which will serve as the
> > entry point for CPU_ON.
> >
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> > ---
> > arm/cstart.S | 7 +++++++
> > arm/cstart64.S | 7 +++++++
> > arm/psci.c | 5 +++--
> > 3 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index 2c81d39a666b..dfef48e4dbb2 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -7,6 +7,7 @@
> > */
> > #define __ASSEMBLY__
> > #include <auxinfo.h>
> > +#include <linux/psci.h>
> > #include <asm/thread_info.h>
> > #include <asm/asm-offsets.h>
> > #include <asm/pgtable-hwdef.h>
> > @@ -139,6 +140,12 @@ secondary_entry:
> > blx r0
> > b do_idle
> >
> > +.global asm_cpu_psci_cpu_die
> > +asm_cpu_psci_cpu_die:
> > + ldr r0, =PSCI_0_2_FN_CPU_OFF
> > + hvc #0
> > + b .
>
> I am wondering if this implementation is actually too simple. Both the current implementation and the kernel clear at least the first three arguments to 0.
> I failed to find a requirement for doing this (nothing in the SMCCC or the PSCI spec), but I guess it would make sense when looking at forward compatibility.
>
> At the very least it's a change in behaviour (ignoring the missing printf).
> So shall we just clear r1, r2 and r3 here? (Same for arm64 below)
If we were to keep this function, then I agree we should zero the
registers, but as I said above, I think the proper fix for this issue is
to just not call PSCI die. Rather we should drop into an infinite loop,
which also doesn't use the stack. Maybe something like this will work
diff --git a/arm/psci.c b/arm/psci.c
index 5c1accb6cea4..74c179d4976c 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -79,10 +79,14 @@ static void cpu_on_secondary_entry(void)
cpumask_set_cpu(cpu, &cpu_on_ready);
while (!cpu_on_start)
cpu_relax();
- cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
cpumask_set_cpu(cpu, &cpu_on_done);
}
+/*
+ * This test expects CPU1 to not have previously been boot,
+ * and when this test completes CPU1 will be stuck in halt.
+ */
static bool psci_cpu_on_test(void)
{
bool failed = false;
@@ -104,7 +108,7 @@ static bool psci_cpu_on_test(void)
cpu_on_start = 1;
smp_mb();
- cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
+ cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
cpumask_set_cpu(0, &cpu_on_done);
while (!cpumask_full(&cpu_on_done))
Thanks,
drew
>
> Cheers,
> Andre
>
> > +
> > .globl halt
> > halt:
> > 1: wfi
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index b0e8baa1a23a..c98842f11e90 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -7,6 +7,7 @@
> > */
> > #define __ASSEMBLY__
> > #include <auxinfo.h>
> > +#include <linux/psci.h>
> > #include <asm/asm-offsets.h>
> > #include <asm/ptrace.h>
> > #include <asm/processor.h>
> > @@ -128,6 +129,12 @@ secondary_entry:
> > blr x0
> > b do_idle
> >
> > +.globl asm_cpu_psci_cpu_die
> > +asm_cpu_psci_cpu_die:
> > + ldr x0, =PSCI_0_2_FN_CPU_OFF
> > + hvc #0
> > + b .
> > +
> > .globl halt
> > halt:
> > 1: wfi
> > diff --git a/arm/psci.c b/arm/psci.c
> > index 5c1accb6cea4..c45a39c7d6e8 100644
> > --- a/arm/psci.c
> > +++ b/arm/psci.c
> > @@ -72,6 +72,7 @@ static int cpu_on_ret[NR_CPUS];
> > static cpumask_t cpu_on_ready, cpu_on_done;
> > static volatile int cpu_on_start;
> >
> > +extern void asm_cpu_psci_cpu_die(void);
> > static void cpu_on_secondary_entry(void)
> > {
> > int cpu = smp_processor_id();
> > @@ -79,7 +80,7 @@ static void cpu_on_secondary_entry(void)
> > cpumask_set_cpu(cpu, &cpu_on_ready);
> > while (!cpu_on_start)
> > cpu_relax();
> > - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> > cpumask_set_cpu(cpu, &cpu_on_done);
> > }
> >
> > @@ -104,7 +105,7 @@ static bool psci_cpu_on_test(void)
> > cpu_on_start = 1;
> > smp_mb();
> >
> > - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(cpu_psci_cpu_die));
> > + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(asm_cpu_psci_cpu_die));
> > cpumask_set_cpu(0, &cpu_on_done);
> >
> > while (!cpumask_full(&cpu_on_done))
>
