Re: [U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 03/07/2018 14:32, Stefan Agner wrote: > Hi Tom, Hi Stefano, > > On 02.07.2018 11:08, Patrick DELAUNAY wrote: >> Hi Stefan >> >>> From: Stefan Agner [mailto:ste...@agner.ch] >>> Sent: mercredi 27 juin 2018 10:36 >>> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on >>> secondary CPUs >>> >>> On 24.06.2018 21:09, Stefan Agner wrote: From: Stefan Agner A proper stack is required to safely use C code in psci_arch_cpu_entry. >>> >>> Patrick, I prefer to have your ack on this since you introduced >>> psci_arch_cpu_entry. >>> >>> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same >>> function in i.MX 7's PSCI implementation the compiler actually pushed stuff >>> on >>> the (uninitialized) stack, which caused the newly brought up CPU to >>> immediately >>> crash. >>> >>> Not sure if in your case the stack pointer is already setup by some other >>> means >>> or your compiler does not use the stack. >>> >>> In any case, I think it is better to just setup the stack properly as done >>> in this >>> patch... >> >> I expected that the secure stack is initialized by bootROM, but after >> check on 2018.07-rc2, >> I got a crash also with the stm32mp1 platform. >> >> After code review, my behavior is clearly not safe: I don't sure that >> the initial BootROM stack >> is not overlapping the installed PSCI monitor code or data. >> So I agree: it is needed to initialize the stack in psci_cpu_entry. >> >> Moreover after your patch the crash is solved for my platform stm32mp1. > > Given that this fixes two platforms I guess it definitely should go into > v2018.07. > > Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 > are fairly straight forward and seem to work fine here. > > Patch 1 is generic, so not sure through which trees the patchset should > go... Sorry for late answer, but benner late as never...;-) I have applied the whole patchset to u-boot-imx. I will push it in a couple of hours on the server. Regards, Stefano -- = DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On Tue, Jul 03, 2018 at 02:32:28PM +0200, Stefan Agner wrote: > Hi Tom, Hi Stefano, > > On 02.07.2018 11:08, Patrick DELAUNAY wrote: > > Hi Stefan > > > >> From: Stefan Agner [mailto:ste...@agner.ch] > >> Sent: mercredi 27 juin 2018 10:36 > >> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on > >> secondary CPUs > >> > >> On 24.06.2018 21:09, Stefan Agner wrote: > >> > From: Stefan Agner > >> > > >> > A proper stack is required to safely use C code in psci_arch_cpu_entry. > >> > >> Patrick, I prefer to have your ack on this since you introduced > >> psci_arch_cpu_entry. > >> > >> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same > >> function in i.MX 7's PSCI implementation the compiler actually pushed > >> stuff on > >> the (uninitialized) stack, which caused the newly brought up CPU to > >> immediately > >> crash. > >> > >> Not sure if in your case the stack pointer is already setup by some other > >> means > >> or your compiler does not use the stack. > >> > >> In any case, I think it is better to just setup the stack properly as done > >> in this > >> patch... > > > > I expected that the secure stack is initialized by bootROM, but after > > check on 2018.07-rc2, > > I got a crash also with the stm32mp1 platform. > > > > After code review, my behavior is clearly not safe: I don't sure that > > the initial BootROM stack > > is not overlapping the installed PSCI monitor code or data. > > So I agree: it is needed to initialize the stack in psci_cpu_entry. > > > > Moreover after your patch the crash is solved for my platform stm32mp1. > > Given that this fixes two platforms I guess it definitely should go into > v2018.07. > > Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 > are fairly straight forward and seem to work fine here. > > Patch 1 is generic, so not sure through which trees the patchset should > go... I'm OK with the series coming in if Stefano is. -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
Hi Tom, Hi Stefano, On 02.07.2018 11:08, Patrick DELAUNAY wrote: > Hi Stefan > >> From: Stefan Agner [mailto:ste...@agner.ch] >> Sent: mercredi 27 juin 2018 10:36 >> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary >> CPUs >> >> On 24.06.2018 21:09, Stefan Agner wrote: >> > From: Stefan Agner >> > >> > A proper stack is required to safely use C code in psci_arch_cpu_entry. >> >> Patrick, I prefer to have your ack on this since you introduced >> psci_arch_cpu_entry. >> >> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same >> function in i.MX 7's PSCI implementation the compiler actually pushed stuff >> on >> the (uninitialized) stack, which caused the newly brought up CPU to >> immediately >> crash. >> >> Not sure if in your case the stack pointer is already setup by some other >> means >> or your compiler does not use the stack. >> >> In any case, I think it is better to just setup the stack properly as done >> in this >> patch... > > I expected that the secure stack is initialized by bootROM, but after > check on 2018.07-rc2, > I got a crash also with the stm32mp1 platform. > > After code review, my behavior is clearly not safe: I don't sure that > the initial BootROM stack > is not overlapping the installed PSCI monitor code or data. > So I agree: it is needed to initialize the stack in psci_cpu_entry. > > Moreover after your patch the crash is solved for my platform stm32mp1. Given that this fixes two platforms I guess it definitely should go into v2018.07. Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5 are fairly straight forward and seem to work fine here. Patch 1 is generic, so not sure through which trees the patchset should go... -- Stefan > >> >> Stefano, I think we really want patch 2/3 applied before the release since >> it fixes >> i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0 >> conformant. But patch 2/3 require this patch to be applied... Not sure how we >> should handle this. >> >> -- >> Stefan > > Acked-by: Patrick DELAUNAY > Tested-by: Patrick DELAUNAY > > Test done on stm32mp1 platform > > Thanks! > Patrick ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
Hi Stefan > From: Stefan Agner [mailto:ste...@agner.ch] > Sent: mercredi 27 juin 2018 10:36 > Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary > CPUs > > On 24.06.2018 21:09, Stefan Agner wrote: > > From: Stefan Agner > > > > A proper stack is required to safely use C code in psci_arch_cpu_entry. > > Patrick, I prefer to have your ack on this since you introduced > psci_arch_cpu_entry. > > As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same > function in i.MX 7's PSCI implementation the compiler actually pushed stuff on > the (uninitialized) stack, which caused the newly brought up CPU to > immediately > crash. > > Not sure if in your case the stack pointer is already setup by some other > means > or your compiler does not use the stack. > > In any case, I think it is better to just setup the stack properly as done in > this > patch... I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2, I got a crash also with the stm32mp1 platform. After code review, my behavior is clearly not safe: I don't sure that the initial BootROM stack is not overlapping the installed PSCI monitor code or data. So I agree: it is needed to initialize the stack in psci_cpu_entry. Moreover after your patch the crash is solved for my platform stm32mp1. > > Stefano, I think we really want patch 2/3 applied before the release since it > fixes > i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0 > conformant. But patch 2/3 require this patch to be applied... Not sure how we > should handle this. > > -- > Stefan Acked-by: Patrick DELAUNAY Tested-by: Patrick DELAUNAY Test done on stm32mp1 platform Thanks! Patrick ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
On 24.06.2018 21:09, Stefan Agner wrote:
> From: Stefan Agner
>
> A proper stack is required to safely use C code in psci_arch_cpu_entry.
Patrick, I prefer to have your ack on this since you introduced
psci_arch_cpu_entry.
As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The
same function in i.MX 7's PSCI implementation the compiler actually
pushed stuff on the (uninitialized) stack, which caused the newly
brought up CPU to immediately crash.
Not sure if in your case the stack pointer is already setup by some
other means or your compiler does not use the stack.
In any case, I think it is better to just setup the stack properly as
done in this patch...
Stefano, I think we really want patch 2/3 applied before the release
since it fixes i.MX 7 PSCI. Right now the implementation is really
broken and not PSCI 1.0 conformant. But patch 2/3 require this patch to
be applied... Not sure how we should handle this.
--
Stefan
>
> Fixes: 486daaa618e1 ("arm: psci: add a weak function psci_arch_cpu_entry")
> Cc: Patrick Delaunay
> Signed-off-by: Stefan Agner
> ---
>
> arch/arm/cpu/armv7/psci.S | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
> index 08b5088675..983cd90442 100644
> --- a/arch/arm/cpu/armv7/psci.S
> +++ b/arch/arm/cpu/armv7/psci.S
> @@ -331,6 +331,8 @@ ENTRY(psci_cpu_entry)
>
> bl _nonsec_init
>
> + bl psci_stack_setup
> +
> bl psci_arch_cpu_entry
>
> bl psci_get_cpu_id @ CPU ID => r0
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
