| Message ID | 1577808589-31892-7-git-send-email-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: Various fixes | expand |
On Tue, 31 Dec 2019 16:09:37 +0000 Alexandru Elisei <alexandru.elisei@arm.com> 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? 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@arm.com> > --- > 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) 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))
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@arm.com> 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@arm.com> > > --- > > 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)) >
Hi Andre, Thank you for reviewing the patches! On 1/2/20 6:11 PM, Andre Przywara wrote: > On Tue, 31 Dec 2019 16:09:37 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> 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? The test checks for the return value of the CPU_ON function. What the CPU does while it's live is inconsequential. > 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@arm.com> >> --- >> 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. The SMC calling convention only specifies the values for the arguments that are used by a function, not the values for all possible arguments. kvm-unit-tests sets the other arguments to 0 because the function prototype that does the actual SMC call takes 4 arguments. The value 0 is a random value that was chosen for those unused parameters. For example, it could have been a random number on each call. Let me put it another way. Suggesting that unused arguments should be set to 0 is the same as suggesting that normal C function that adheres to procedure call standard for arm64 should always have 8 arguments, and for a particular function that doesn't use all of them, they should be set to 0 by the caller. @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he wants to chime in on this. Thanks, Alex > 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) > > 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))
Hi, On 1/3/20 3:31 PM, Andrew Jones wrote: > 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@arm.com> 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. I don't like this idea, I'll explain below why. >> 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@arm.com> >>> --- >>> 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 Regarding zero'ing unused arguments, I've explained my point of view in my reply to Andre. > 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)) Trying to turn a CPU on and off concurrently from separate threads seems like a nifty test to me, and good for catching possible races. With your version, 2 threads are enough to get all possible results: success and CPU already on. Besides that, if my memory serves me right, this exact test was the reason for a refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't write junk to sysregs on reset"). My preference would be to keep calling CPU_ON and CPU_OFF repeatedly. Thanks, Alex > 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))
On Mon, 6 Jan 2020 10:41:55 +0000 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi Andre, > > Thank you for reviewing the patches! > > On 1/2/20 6:11 PM, Andre Przywara wrote: > > On Tue, 31 Dec 2019 16:09:37 +0000 > > Alexandru Elisei <alexandru.elisei@arm.com> 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? > > The test checks for the return value of the CPU_ON function. What the CPU does > while it's live is inconsequential. OK, I see. > > 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@arm.com> > >> --- > >> 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. > > The SMC calling convention only specifies the values for the arguments that are > used by a function, not the values for all possible arguments. kvm-unit-tests sets > the other arguments to 0 because the function prototype that does the actual SMC > call takes 4 arguments. The value 0 is a random value that was chosen for those > unused parameters. For example, it could have been a random number on each call. > > Let me put it another way. Suggesting that unused arguments should be set to 0 is > the same as suggesting that normal C function that adheres to procedure call > standard for arm64 should always have 8 arguments, and for a particular function > that doesn't use all of them, they should be set to 0 by the caller. I understand that, but was wondering if the SMCCC would mandate stricter requirements. After all every PSCI call from Linux goes through a function which clears all unused input parameters. Cheers, Andre. > @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he > wants to chime in on this. > > Thanks, > Alex > > 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) > > > > 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))
Hi, On 1/6/20 11:17 AM, Andre Przywara wrote: > On Mon, 6 Jan 2020 10:41:55 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi Andre, >> >> Thank you for reviewing the patches! >> >> On 1/2/20 6:11 PM, Andre Przywara wrote: >>> On Tue, 31 Dec 2019 16:09:37 +0000 >>> Alexandru Elisei <alexandru.elisei@arm.com> 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? >> The test checks for the return value of the CPU_ON function. What the CPU does >> while it's live is inconsequential. > OK, I see. > >>> 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@arm.com> >>>> --- >>>> 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. >> The SMC calling convention only specifies the values for the arguments that are >> used by a function, not the values for all possible arguments. kvm-unit-tests sets >> the other arguments to 0 because the function prototype that does the actual SMC >> call takes 4 arguments. The value 0 is a random value that was chosen for those >> unused parameters. For example, it could have been a random number on each call. >> >> Let me put it another way. Suggesting that unused arguments should be set to 0 is >> the same as suggesting that normal C function that adheres to procedure call >> standard for arm64 should always have 8 arguments, and for a particular function >> that doesn't use all of them, they should be set to 0 by the caller. > I understand that, but was wondering if the SMCCC would mandate stricter requirements. After all every PSCI call from Linux goes through a function which clears all unused input parameters. I don't think that's a good idea. kvm-unit-tests is designed to test correctness. Doing more than it is strictly necessary might hide real issues. SMCCC mandating stricter requirements could happen at some point, yes. But I think KVM/qemu dropping support for current versions of PSCI is a long way into the future. In the meantime, I would not make decisions based on hypothetical situations that might or might not happen. Thanks, Alex > > Cheers, > Andre. > >> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he >> wants to chime in on this. >> >> Thanks, >> Alex >>> 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) >>> >>> 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))
On Mon, Jan 06, 2020 at 11:17:16AM +0000, Andre Przywara wrote: > On Mon, 6 Jan 2020 10:41:55 +0000 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > Hi Andre, > > > > Thank you for reviewing the patches! > > > > On 1/2/20 6:11 PM, Andre Przywara wrote: > > >> +.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. This is a Linux implementation detail, and is not intended to be an SMCCC contract detail. To be generic to all SMCCC calls, the invocation functions have to take the largest set of potential arguments, and callers have to pass /something/ for the unused values. It would be perfectly valid for callers to pass the result of get_random_long() instead. The SMCCC implementation should not derive any meaning from registers which are not arguments to the call in question. > > The SMC calling convention only specifies the values for the arguments that are > > used by a function, not the values for all possible arguments. kvm-unit-tests sets > > the other arguments to 0 because the function prototype that does the actual SMC > > call takes 4 arguments. The value 0 is a random value that was chosen for those > > unused parameters. For example, it could have been a random number on each call. > > > > Let me put it another way. Suggesting that unused arguments should be set to 0 is > > the same as suggesting that normal C function that adheres to procedure call > > standard for arm64 should always have 8 arguments, and for a particular function > > that doesn't use all of them, they should be set to 0 by the caller. > > I understand that, but was wondering if the SMCCC would mandate > stricter requirements. After all every PSCI call from Linux goes > through a function which clears all unused input parameters. Callers are not deliberately clearingh the unused parameters, but rather passing arbitrary values because they are forced to. Thanks, Mark.
On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote: > On 1/2/20 6:11 PM, Andre Przywara wrote: > > On Tue, 31 Dec 2019 16:09:37 +0000 > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> +.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. > > The SMC calling convention only specifies the values for the arguments that are > used by a function, not the values for all possible arguments. kvm-unit-tests sets > the other arguments to 0 because the function prototype that does the actual SMC > call takes 4 arguments. The value 0 is a random value that was chosen for those > unused parameters. For example, it could have been a random number on each call. That's correct. A caller can leave arbitrary values in non-argument registers, in the same manner as a caller of an AAPCS function. The callee should not consume those values as they are not arguments. > Let me put it another way. Suggesting that unused arguments should be set to 0 is > the same as suggesting that normal C function that adheres to procedure call > standard for arm64 should always have 8 arguments, and for a particular function > that doesn't use all of them, they should be set to 0 by the caller. Heh, same rationale. :) > @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he > wants to chime in on this. > > Thanks, > Alex > > 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) There's no need to zero non-argument registers, and it could potentially mask bugs in callees, so I don't think it's a good idea to do so. If you really want to test that the callee is robust and correct, it would be better to randomize the content of non-argument regsiters to fuzz the callee. Thanks, Mark.
On Mon, Jan 06, 2020 at 11:02:16AM +0000, Alexandru Elisei wrote: > Hi, > > On 1/3/20 3:31 PM, Andrew Jones wrote: > > 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@arm.com> 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. > > I don't like this idea, I'll explain below why. > > >> 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@arm.com> > >>> --- > >>> 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 > > Regarding zero'ing unused arguments, I've explained my point of view in my reply > to Andre. > > > 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)) > > Trying to turn a CPU on and off concurrently from separate threads seems like a > nifty test to me, and good for catching possible races. With your version, 2 > threads are enough to get all possible results: success and CPU already on. > > Besides that, if my memory serves me right, this exact test was the reason for a > refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't > write junk to sysregs on reset"). It wasn't because there was a CPU_OFF involved. Two simultaneous CPU_ON's was enough. The first, successful CPU_ON reset the target VCPU causing the second CPU_ON to fail to look it up by MPIDR, resulting in PSCI_RET_INVALID_PARAMS. > > My preference would be to keep calling CPU_ON and CPU_OFF repeatedly. > Your analysis showed we never called CPU_OFF, because CPU1 got stuck in an exception loop. So we haven't actually done a repeated ON/OFF test yet. That's not a bad idea, but I also like a test that ensures a single successful CPU_ON, like the patch below would ensure if we halted instead of CPU_OFF'ed. It should be possible to have both tests if we do the CPU_OFF test first. Thanks, drew diff --git a/arm/psci.c b/arm/psci.c index 5c1accb6cea4..8a24860bde28 100644 --- a/arm/psci.c +++ b/arm/psci.c @@ -86,7 +86,7 @@ static void cpu_on_secondary_entry(void) static bool psci_cpu_on_test(void) { bool failed = false; - int cpu; + int cpu, ret_success = 0; cpumask_set_cpu(1, &cpu_on_ready); cpumask_set_cpu(1, &cpu_on_done); @@ -113,7 +113,12 @@ static bool psci_cpu_on_test(void) for_each_present_cpu(cpu) { if (cpu == 1) continue; - if (cpu_on_ret[cpu] != PSCI_RET_SUCCESS && cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) { + if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) { + if (++ret_success > 1) { + report_info("more than one cpu_on success"); + failed = true; + } + } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) { report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]); failed = true; }
On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote: > On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote: > > On 1/2/20 6:11 PM, Andre Przywara wrote: > > > On Tue, 31 Dec 2019 16:09:37 +0000 > > > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > >> +.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. > > > > The SMC calling convention only specifies the values for the arguments that are > > used by a function, not the values for all possible arguments. kvm-unit-tests sets > > the other arguments to 0 because the function prototype that does the actual SMC > > call takes 4 arguments. The value 0 is a random value that was chosen for those > > unused parameters. For example, it could have been a random number on each call. > > That's correct. > > A caller can leave arbitrary values in non-argument registers, in the > same manner as a caller of an AAPCS function. The callee should not > consume those values as they are not arguments. > > > Let me put it another way. Suggesting that unused arguments should be set to 0 is > > the same as suggesting that normal C function that adheres to procedure call > > standard for arm64 should always have 8 arguments, and for a particular function > > that doesn't use all of them, they should be set to 0 by the caller. > > Heh, same rationale. :) This is a good rationale for the function to not zero parameters. > > > @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he > > wants to chime in on this. > > > > Thanks, > > Alex > > > 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) > > There's no need to zero non-argument registers, and it could potentially > mask bugs in callees, so I don't think it's a good idea to do so. > > If you really want to test that the callee is robust and correct, it > would be better to randomize the content of non-argument regsiters to > fuzz the callee. > But this indicates there is risk that we'll be less robust if we don't zero the parameters. Since this function is a common utility function and kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody wants to try and target, then if there's any chance that zeroing unused parameters is more robust, I believe we should do that here. If we want to test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write explicit test cases to do that. I can't speak to the risk of not zeroing, but due to the way we've been calling PSCI functions with C, I can say up until now we always have. Thanks, drew
Hi, On 1/6/20 1:03 PM, Andrew Jones wrote: > On Mon, Jan 06, 2020 at 11:02:16AM +0000, Alexandru Elisei wrote: >> Hi, >> >> On 1/3/20 3:31 PM, Andrew Jones wrote: >>> 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@arm.com> 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. >> I don't like this idea, I'll explain below why. >> >>>> 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@arm.com> >>>>> --- >>>>> 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 >> Regarding zero'ing unused arguments, I've explained my point of view in my reply >> to Andre. >> >>> 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)) >> Trying to turn a CPU on and off concurrently from separate threads seems like a >> nifty test to me, and good for catching possible races. With your version, 2 >> threads are enough to get all possible results: success and CPU already on. >> >> Besides that, if my memory serves me right, this exact test was the reason for a >> refactoring of the VCPU reset code, in commit 03fdfb269009 ("KVM: arm64: Don't >> write junk to sysregs on reset"). > It wasn't because there was a CPU_OFF involved. Two simultaneous CPU_ON's > was enough. The first, successful CPU_ON reset the target VCPU causing the > second CPU_ON to fail to look it up by MPIDR, resulting in > PSCI_RET_INVALID_PARAMS. My bad. >> My preference would be to keep calling CPU_ON and CPU_OFF repeatedly. >> > Your analysis showed we never called CPU_OFF, because CPU1 got stuck > in an exception loop. So we haven't actually done a repeated ON/OFF True. > test yet. That's not a bad idea, but I also like a test that ensures > a single successful CPU_ON, like the patch below would ensure if we > halted instead of CPU_OFF'ed. > > It should be possible to have both tests if we do the CPU_OFF test first. > > Thanks, > drew > > > diff --git a/arm/psci.c b/arm/psci.c > index 5c1accb6cea4..8a24860bde28 100644 > --- a/arm/psci.c > +++ b/arm/psci.c > @@ -86,7 +86,7 @@ static void cpu_on_secondary_entry(void) > static bool psci_cpu_on_test(void) > { > bool failed = false; > - int cpu; > + int cpu, ret_success = 0; > > cpumask_set_cpu(1, &cpu_on_ready); > cpumask_set_cpu(1, &cpu_on_done); > @@ -113,7 +113,12 @@ static bool psci_cpu_on_test(void) > for_each_present_cpu(cpu) { > if (cpu == 1) > continue; > - if (cpu_on_ret[cpu] != PSCI_RET_SUCCESS && cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) { > + if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) { > + if (++ret_success > 1) { > + report_info("more than one cpu_on success"); > + failed = true; > + } > + } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) { > report_info("unexpected cpu_on return value: caller=CPU%d, ret=%d", cpu, cpu_on_ret[cpu]); > failed = true; > } > Your patch looks fine to me. Tested it together with changing the CPU_ON address to halt and running it 1000 times on Ampere EMAG: Tested-by: Alexandru Elisei <alexandru.elisei@arm.com> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> I'm fine with dropping my patch from the series and sending it as an enhancement after the other fixes get merged. Thanks, Alex
Hi, On 1/6/20 1:17 PM, Andrew Jones wrote: > On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote: >> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote: >>> On 1/2/20 6:11 PM, Andre Przywara wrote: >>>> On Tue, 31 Dec 2019 16:09:37 +0000 >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote: >>>>> +.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. >>> The SMC calling convention only specifies the values for the arguments that are >>> used by a function, not the values for all possible arguments. kvm-unit-tests sets >>> the other arguments to 0 because the function prototype that does the actual SMC >>> call takes 4 arguments. The value 0 is a random value that was chosen for those >>> unused parameters. For example, it could have been a random number on each call. >> That's correct. >> >> A caller can leave arbitrary values in non-argument registers, in the >> same manner as a caller of an AAPCS function. The callee should not >> consume those values as they are not arguments. >> >>> Let me put it another way. Suggesting that unused arguments should be set to 0 is >>> the same as suggesting that normal C function that adheres to procedure call >>> standard for arm64 should always have 8 arguments, and for a particular function >>> that doesn't use all of them, they should be set to 0 by the caller. >> Heh, same rationale. :) > This is a good rationale for the function to not zero parameters. > >>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he >>> wants to chime in on this. >>> >>> Thanks, >>> Alex >>>> 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) >> There's no need to zero non-argument registers, and it could potentially >> mask bugs in callees, so I don't think it's a good idea to do so. >> >> If you really want to test that the callee is robust and correct, it >> would be better to randomize the content of non-argument regsiters to >> fuzz the callee. >> > But this indicates there is risk that we'll be less robust if we don't > zero the parameters. Since this function is a common utility function and > kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody > wants to try and target, then if there's any chance that zeroing unused > parameters is more robust, I believe we should do that here. If we want to We agree that zero'ing unused parameters is not required by the specification, right? After that, I think it depends how you see kvm-unit-tests. I am of the opinion that as a testing tool, if not zero'ing parameters (I'm not talking here about fuzzing) causes an error in whatever piece of software you are running, then that piece of software is not specification compliant and kvm-unit-tests has done its job. Either way, I'm not advocating changing our PSCI code. I'm fine with dropping this patch for now (I mentioned this in another thread), so we can resume this conversation when I rework it. Thanks, Alex > test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write > explicit test cases to do that. > > I can't speak to the risk of not zeroing, but due to the way we've been > calling PSCI functions with C, I can say up until now we always have. > > Thanks, > drew >
On Mon, Jan 06, 2020 at 02:12:46PM +0000, Alexandru Elisei wrote: > Hi, > > On 1/6/20 1:17 PM, Andrew Jones wrote: > > On Mon, Jan 06, 2020 at 11:41:49AM +0000, Mark Rutland wrote: > >> On Mon, Jan 06, 2020 at 10:41:55AM +0000, Alexandru Elisei wrote: > >>> On 1/2/20 6:11 PM, Andre Przywara wrote: > >>>> On Tue, 31 Dec 2019 16:09:37 +0000 > >>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >>>>> +.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. > >>> The SMC calling convention only specifies the values for the arguments that are > >>> used by a function, not the values for all possible arguments. kvm-unit-tests sets > >>> the other arguments to 0 because the function prototype that does the actual SMC > >>> call takes 4 arguments. The value 0 is a random value that was chosen for those > >>> unused parameters. For example, it could have been a random number on each call. > >> That's correct. > >> > >> A caller can leave arbitrary values in non-argument registers, in the > >> same manner as a caller of an AAPCS function. The callee should not > >> consume those values as they are not arguments. > >> > >>> Let me put it another way. Suggesting that unused arguments should be set to 0 is > >>> the same as suggesting that normal C function that adheres to procedure call > >>> standard for arm64 should always have 8 arguments, and for a particular function > >>> that doesn't use all of them, they should be set to 0 by the caller. > >> Heh, same rationale. :) > > This is a good rationale for the function to not zero parameters. > > > >>> @Mark Rutland has worked on the SMC implementation for the Linux kernel, if he > >>> wants to chime in on this. > >>> > >>> Thanks, > >>> Alex > >>>> 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) > >> There's no need to zero non-argument registers, and it could potentially > >> mask bugs in callees, so I don't think it's a good idea to do so. > >> > >> If you really want to test that the callee is robust and correct, it > >> would be better to randomize the content of non-argument regsiters to > >> fuzz the callee. > >> > > But this indicates there is risk that we'll be less robust if we don't > > zero the parameters. Since this function is a common utility function and > > kvm-unit-tests targets KVM, QEMU/tcg, and anything else that somebody > > wants to try and target, then if there's any chance that zeroing unused > > parameters is more robust, I believe we should do that here. If we want to > > We agree that zero'ing unused parameters is not required by the specification, > right? Definitely > After that, I think it depends how you see kvm-unit-tests. I am of the > opinion that as a testing tool, if not zero'ing parameters (I'm not talking here > about fuzzing) causes an error in whatever piece of software you are running, then > that piece of software is not specification compliant and kvm-unit-tests has done > its job. We generally do our best to make sure the supporting code in the framework is robust and fails cleanly (usually with asserts). If there's reason to believe that the SUT may not implement a specification correctly, but we have test results proving it at least works under certain constrains, then we should probably keep the support code within those constraints. We can also write independent test cases to check that the SUT implements the specification completely. The reason for this approach is because not every user of kvm-unit-tests is willing to debug a random, potentially difficult to isolate failure when they're attempting to use the framework to test something unrelated. > > Either way, I'm not advocating changing our PSCI code. I'm fine with dropping this > patch for now (I mentioned this in another thread), so we can resume this > conversation when I rework it. Sounds good. And, like I said, I can't speak to the risk of not zeroing. Perhaps there's not enough concern here to bother with it. Maybe we could write those PSCI fuzzing tests sooner than later, confirm that on a reasonable sample of targets that there's no risk in not zeroing, and then implement all supporting functions as we wish. Thanks, drew > > Thanks, > Alex > > test/fuzz the PSCI/SMC emulation with kvm-unit-tests, then we can write > > explicit test cases to do that. > > > > I can't speak to the risk of not zeroing, but due to the way we've been > > calling PSCI functions with C, I can say up until now we always have. > > > > Thanks, > > drew > > >
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 . + .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))
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. 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@arm.com> --- arm/cstart.S | 7 +++++++ arm/cstart64.S | 7 +++++++ arm/psci.c | 5 +++-- 3 files changed, 17 insertions(+), 2 deletions(-)