| Message ID | 1566999511-24916-3-git-send-email-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Run at EL2 | expand |
On Wed, Aug 28, 2019 at 02:38:17PM +0100, Alexandru Elisei wrote: > 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(-) > > diff --git a/arm/cstart.S b/arm/cstart.S > index 114726feab82..5d4fe4b1570b 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/ptrace.h> > @@ -138,6 +139,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 halt Shouldn't we load PSCI_POWER_STATE_TYPE_POWER_DOWN into r1 and zero out r2 and r3, as cpu_psci_cpu_die() does? And maybe we should just do a 'b .' here instead of 'b halt' in order to avoid confusion as to how we ended up in halt(), if the psci invocation were to ever fail. > + > .globl halt > halt: > 1: wfi > diff --git a/arm/cstart64.S b/arm/cstart64.S > index b0e8baa1a23a..20f832fd57f7 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 halt Same as above > + > .globl halt > halt: > 1: wfi > diff --git a/arm/psci.c b/arm/psci.c > index 5cb4d5c7c233..0440c4cdbc59 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)) > -- > 2.7.4 > Thanks, drew
On 8/28/19 3:45 PM, Andrew Jones wrote: > On Wed, Aug 28, 2019 at 02:38:17PM +0100, Alexandru Elisei wrote: >> 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(-) >> >> diff --git a/arm/cstart.S b/arm/cstart.S >> index 114726feab82..5d4fe4b1570b 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/ptrace.h> >> @@ -138,6 +139,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 halt > Shouldn't we load PSCI_POWER_STATE_TYPE_POWER_DOWN into r1 and > zero out r2 and r3, as cpu_psci_cpu_die() does? And maybe we > should just do a 'b .' here instead of 'b halt' in order to > avoid confusion as to how we ended up in halt(), if the psci > invocation were to ever fail. Not really, I'm not really sure where the extra parameter in cpu_psci_cpu_die comes from. I have looked at ARM DEN 0022D, section 5.1.3, and the CPU_OFFcall has exactly one parameter, the function id. I've also looked at how KVM handles this call, and it doesn't use anything else other than the function id. Please correct me if I missed something. As for zero'ing the extra registers, this is not part of the SMC calling convention, this is just something that the C code for psci does. The SMC calling convention states that registers 0-3 will be modified after the call, so having 4 arguments to the psci_invoke function will tell the compiler to save/restore the registers in the caller. As for doing 'b .' instead of branching to halt, that's a good idea, I'll do that. But it will only be useful if the last CPU_OFF call fails. Thanks, Alex
On 8/28/19 4:14 PM, Alexandru Elisei wrote: > On 8/28/19 3:45 PM, Andrew Jones wrote: >> On Wed, Aug 28, 2019 at 02:38:17PM +0100, Alexandru Elisei wrote: >>> 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(-) >>> >>> diff --git a/arm/cstart.S b/arm/cstart.S >>> index 114726feab82..5d4fe4b1570b 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/ptrace.h> >>> @@ -138,6 +139,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 halt >> Shouldn't we load PSCI_POWER_STATE_TYPE_POWER_DOWN into r1 and >> zero out r2 and r3, as cpu_psci_cpu_die() does? And maybe we >> should just do a 'b .' here instead of 'b halt' in order to >> avoid confusion as to how we ended up in halt(), if the psci >> invocation were to ever fail. > Not really, I'm not really sure where the extra parameter in cpu_psci_cpu_die > comes from. I have looked at ARM DEN 0022D, section 5.1.3, and the CPU_OFFcall > has exactly one parameter, the function id. I've also looked at how KVM handles > this call, and it doesn't use anything else other than the function id. Please > correct me if I missed something. Did some digging, apparently the power state parameter was required for the very first version of PSCI. ARM DEN 0022D states that it has been removed in PSCIv0.2: "7.3 Changes in PSCIv0.2 from first proposal [..] Removed power_state parameter for CPU_OFF." The kvm-unit-tests implementation of psci uses fixed function ids (as opposed to first psci version, where the ids were taken from the DT), so I think that we can drop the PSCI_POWER_STATE_TYPE_POWER_DOWN parameter in cpu_psci_cpu_die altogether. What do you think? Thanks, Alex > As for zero'ing the extra registers, this is not part of the SMC calling > convention, this is just something that the C code for psci does. The SMC > calling convention states that registers 0-3 will be modified after the call, so > having 4 arguments to the psci_invoke function will tell the compiler to > save/restore the registers in the caller. > > As for doing 'b .' instead of branching to halt, that's a good idea, I'll do > that. But it will only be useful if the last CPU_OFF call fails. > > Thanks, > Alex
On Mon, Sep 02, 2019 at 03:55:48PM +0100, Alexandru Elisei wrote: > On 8/28/19 4:14 PM, Alexandru Elisei wrote: > > > On 8/28/19 3:45 PM, Andrew Jones wrote: > >> On Wed, Aug 28, 2019 at 02:38:17PM +0100, Alexandru Elisei wrote: > >>> 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(-) > >>> > >>> diff --git a/arm/cstart.S b/arm/cstart.S > >>> index 114726feab82..5d4fe4b1570b 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/ptrace.h> > >>> @@ -138,6 +139,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 halt > >> Shouldn't we load PSCI_POWER_STATE_TYPE_POWER_DOWN into r1 and > >> zero out r2 and r3, as cpu_psci_cpu_die() does? And maybe we > >> should just do a 'b .' here instead of 'b halt' in order to > >> avoid confusion as to how we ended up in halt(), if the psci > >> invocation were to ever fail. > > Not really, I'm not really sure where the extra parameter in cpu_psci_cpu_die > > comes from. I have looked at ARM DEN 0022D, section 5.1.3, and the CPU_OFFcall > > has exactly one parameter, the function id. I've also looked at how KVM handles > > this call, and it doesn't use anything else other than the function id. Please > > correct me if I missed something. > > Did some digging, apparently the power state parameter was required for the very > first version of PSCI. ARM DEN 0022D states that it has been removed in PSCIv0.2: > > "7.3 Changes in PSCIv0.2 from first proposal > > [..] > > Removed power_state parameter for CPU_OFF." > > The kvm-unit-tests implementation of psci uses fixed function ids (as opposed to > first psci version, where the ids were taken from the DT), so I think that we > can drop the PSCI_POWER_STATE_TYPE_POWER_DOWN parameter in cpu_psci_cpu_die > altogether. What do you think? Sounds good to me. Thanks for the digging. drew > > Thanks, > Alex > > As for zero'ing the extra registers, this is not part of the SMC calling > > convention, this is just something that the C code for psci does. The SMC > > calling convention states that registers 0-3 will be modified after the call, so > > having 4 arguments to the psci_invoke function will tell the compiler to > > save/restore the registers in the caller. > > > > As for doing 'b .' instead of branching to halt, that's a good idea, I'll do > > that. But it will only be useful if the last CPU_OFF call fails. > > > > Thanks, > > Alex
diff --git a/arm/cstart.S b/arm/cstart.S index 114726feab82..5d4fe4b1570b 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/ptrace.h> @@ -138,6 +139,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 halt + .globl halt halt: 1: wfi diff --git a/arm/cstart64.S b/arm/cstart64.S index b0e8baa1a23a..20f832fd57f7 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 halt + .globl halt halt: 1: wfi diff --git a/arm/psci.c b/arm/psci.c index 5cb4d5c7c233..0440c4cdbc59 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(-)