| Message ID | ee1f4b9b969e6cf67278905e0405bc4fa5d6080c.1561147189.git.denisobrezkov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Switch OMAP5 secondary cores into hyp mode | expand |
(+ GSOC mentors and Andre) Hi Denis, Thank you for the patch. First of all, may I ask to CC the other mentors? On 6/21/19 9:02 PM, Denis Obrezkov wrote: > This function allows xen to bring secondary CPU cores into non-secure > HYP mode. This is done by using a Secure Monitor call. > > Signed-off-by: Denis Obrezkov <denisobrezkov@gmail.com> > --- > xen/arch/arm/arm32/head.S | 11 ++++++++++- > xen/arch/arm/platforms/omap5.c | 5 +++-- > xen/include/asm-arm/platforms/omap5.h | 3 +++ > 3 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S > index 5f817d473e..120e034934 100644 > --- a/xen/arch/arm/arm32/head.S > +++ b/xen/arch/arm/arm32/head.S > @@ -36,6 +36,10 @@ > #include EARLY_PRINTK_INC > #endif > > + > +#define API_HYP_ENTRY 0x102 > +#define AUX_CORE_BOOT0_PA 0x48281800 > + I have thought a bit more about the placement of the code. I think it would be best if it lives in a separate file (maybe platforms/omap5-head.S). > /* > * Common register usage in this file: > * r0 - > @@ -113,6 +117,12 @@ past_zImage: > > b common_start > > +GLOBAL(omap5_init_secondary) > + ldr r12, =API_HYP_ENTRY NIT: It is 3 spaces after ldr. > + adr r0, init_secondary Same here. > + dsb Why do you need the dsb here? > + smc #0 > + > GLOBAL(init_secondary) > cpsid aif /* Disable all interrupts */ > > @@ -159,7 +169,6 @@ common_start: > PRINT("- CPU doesn't support the virtualization extensions -\r\n") > b fail > 1: > - This is a spurious change. Please remove it. > /* Check that we're already in Hyp mode */ > mrs r0, cpsr > and r0, r0, #0x1f /* Mode is in the low 5 bits of CPSR */ > diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c > index aee24e4d28..6b5cc15af3 100644 > --- a/xen/arch/arm/platforms/omap5.c > +++ b/xen/arch/arm/platforms/omap5.c > @@ -128,8 +128,9 @@ static int __init omap5_smp_init(void) > } > > printk("Set AuxCoreBoot1 to %"PRIpaddr" (%p)\n", > - __pa(init_secondary), init_secondary); > - writel(__pa(init_secondary), wugen_base + OMAP_AUX_CORE_BOOT_1_OFFSET); > + __pa(omap5_init_secondary), omap5_init_secondary); > + writel(__pa(omap5_init_secondary), > + wugen_base + OMAP_AUX_CORE_BOOT_1_OFFSET); I am trying to understand how this ever worked. omap5_smp_init is called by two sets of platforms (based on compatible): - ti,dra7: there were some hacks in U-boot to avoid the SMC. If I am right, then I would not bother to support hacked U-boot. - ti,omap5: [1] suggest that U-boot do the switch for us but it is not clear whether this is upstreamed. @Chen, I know you did the port a long time ago. Do you recall how this worked? Linux seems to use the smc on any dra7 and omap54xx. So maybe we can use safely here. > printk("Set AuxCoreBoot0 to 0x20\n"); > writel(0x20, wugen_base + OMAP_AUX_CORE_BOOT_0_OFFSET); > diff --git a/xen/include/asm-arm/platforms/omap5.h b/xen/include/asm-arm/platforms/omap5.h > index c559c84b61..732b27f403 100644 > --- a/xen/include/asm-arm/platforms/omap5.h > +++ b/xen/include/asm-arm/platforms/omap5.h > @@ -22,6 +22,9 @@ > > #endif /* __ASM_ARM_PLATFORMS_OMAP5_H */ > > +/* Secondary cpu omap5 specific init routine */ > +extern void omap5_init_secondary(void); > + > /* > * Local variables: > * mode: C > [1] https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/OMAP5432_uEVM
On 24/06/2019 12:09, Julien Grall wrote: > (+ GSOC mentors and Andre) > > Hi Denis, > > Thank you for the patch. > > First of all, may I ask to CC the other mentors? > > On 6/21/19 9:02 PM, Denis Obrezkov wrote: >> This function allows xen to bring secondary CPU cores into non-secure >> HYP mode. This is done by using a Secure Monitor call. >> >> Signed-off-by: Denis Obrezkov <denisobrezkov@gmail.com> >> --- >> xen/arch/arm/arm32/head.S | 11 ++++++++++- >> xen/arch/arm/platforms/omap5.c | 5 +++-- >> xen/include/asm-arm/platforms/omap5.h | 3 +++ >> 3 files changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >> index 5f817d473e..120e034934 100644 >> --- a/xen/arch/arm/arm32/head.S >> +++ b/xen/arch/arm/arm32/head.S >> @@ -36,6 +36,10 @@ >> #include EARLY_PRINTK_INC >> #endif >> + >> +#define API_HYP_ENTRY 0x102 >> +#define AUX_CORE_BOOT0_PA 0x48281800 >> + > > I have thought a bit more about the placement of the code. I think it > would be best if it lives in a separate file (maybe > platforms/omap5-head.S). For something this trivial, it is easy to put straight into omap5.c Completely untested, but this ought to work: diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c index 6b5cc15af3..1dcc92d3a4 100644 --- a/xen/arch/arm/platforms/omap5.c +++ b/xen/arch/arm/platforms/omap5.c @@ -23,6 +23,16 @@ #include <xen/vmap.h> #include <asm/io.h> +void omap5_init_secondary(void); +asm ( +".text \n\t" +"omap5_init_secondary: \n\t" +" ldr r12, =0x102 \n\t" /* API_HYP_ENTRY */ +" adr r0, init_secondary \n\t" +" smc #0 \n\t" +" b init_secondary \n\t" +); + static uint16_t num_den[8][2] = { { 0, 0 }, /* not used */ { 26 * 64, 26 * 125 }, /* 12.0 Mhz */ I personally find this favourable to introducing new stub files. Ultimately it is Julien/Stefano's decision, but I'd like to point it out as an option for anyone who is unaware. ~Andrew <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 24/06/2019 12:09, Julien Grall wrote:<br> </div> <blockquote type="cite" cite="mid:ecfa161d-1389-1541-e92c-dfa3b8c7e402@arm.com">(+ GSOC mentors and Andre) <br> <br> Hi Denis, <br> <br> Thank you for the patch. <br> <br> First of all, may I ask to CC the other mentors? <br> <br> On 6/21/19 9:02 PM, Denis Obrezkov wrote: <br> <blockquote type="cite">This function allows xen to bring secondary CPU cores into non-secure <br> HYP mode. This is done by using a Secure Monitor call. <br> <br> Signed-off-by: Denis Obrezkov <a class="moz-txt-link-rfc2396E" href="mailto:denisobrezkov@gmail.com"><denisobrezkov@gmail.com></a> <br> --- <br> xen/arch/arm/arm32/head.S | 11 ++++++++++- <br> xen/arch/arm/platforms/omap5.c | 5 +++-- <br> xen/include/asm-arm/platforms/omap5.h | 3 +++ <br> 3 files changed, 16 insertions(+), 3 deletions(-) <br> <br> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S <br> index 5f817d473e..120e034934 100644 <br> --- a/xen/arch/arm/arm32/head.S <br> +++ b/xen/arch/arm/arm32/head.S <br> @@ -36,6 +36,10 @@ <br> #include EARLY_PRINTK_INC <br> #endif <br> + <br> +#define API_HYP_ENTRY 0x102 <br> +#define AUX_CORE_BOOT0_PA 0x48281800 <br> + <br> </blockquote> <br> I have thought a bit more about the placement of the code. I think it would be best if it lives in a separate file (maybe platforms/omap5-head.S).<br> </blockquote> <br> For something this trivial, it is easy to put straight into omap5.c<br> <br> Completely untested, but this ought to work:<br> <br> <pre>diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c index 6b5cc15af3..1dcc92d3a4 100644 --- a/xen/arch/arm/platforms/omap5.c +++ b/xen/arch/arm/platforms/omap5.c @@ -23,6 +23,16 @@ #include <xen/vmap.h> #include <asm/io.h> +void omap5_init_secondary(void); +asm ( +".text \n\t" +"omap5_init_secondary: \n\t" +" ldr r12, =0x102 \n\t" /* API_HYP_ENTRY */ +" adr r0, init_secondary \n\t" +" smc #0 \n\t" +" b init_secondary \n\t" +); + static uint16_t num_den[8][2] = { { 0, 0 }, /* not used */ { 26 * 64, 26 * 125 }, /* 12.0 Mhz */ </pre> <br> I personally find this favourable to introducing new stub files.<br> <br> Ultimately it is Julien/Stefano's decision, but I'd like to point it out as an option for anyone who is unaware.<br> <br> ~Andrew<br> </body> </html>
Hi Andrew, On 24/06/2019 13:03, Andrew Cooper wrote: > On 24/06/2019 12:09, Julien Grall wrote: >> (+ GSOC mentors and Andre) >> >> Hi Denis, >> >> Thank you for the patch. >> >> First of all, may I ask to CC the other mentors? >> >> On 6/21/19 9:02 PM, Denis Obrezkov wrote: >>> This function allows xen to bring secondary CPU cores into non-secure >>> HYP mode. This is done by using a Secure Monitor call. >>> >>> Signed-off-by: Denis Obrezkov <denisobrezkov@gmail.com> >>> --- >>> xen/arch/arm/arm32/head.S | 11 ++++++++++- >>> xen/arch/arm/platforms/omap5.c | 5 +++-- >>> xen/include/asm-arm/platforms/omap5.h | 3 +++ >>> 3 files changed, 16 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S >>> index 5f817d473e..120e034934 100644 >>> --- a/xen/arch/arm/arm32/head.S >>> +++ b/xen/arch/arm/arm32/head.S >>> @@ -36,6 +36,10 @@ >>> #include EARLY_PRINTK_INC >>> #endif >>> + >>> +#define API_HYP_ENTRY 0x102 >>> +#define AUX_CORE_BOOT0_PA 0x48281800 >>> + >> >> I have thought a bit more about the placement of the code. I think it would be >> best if it lives in a separate file (maybe platforms/omap5-head.S). > > For something this trivial, it is easy to put straight into omap5.c > > Completely untested, but this ought to work: > > diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c > index 6b5cc15af3..1dcc92d3a4 100644 > --- a/xen/arch/arm/platforms/omap5.c > +++ b/xen/arch/arm/platforms/omap5.c > @@ -23,6 +23,16 @@ > #include <xen/vmap.h> > #include <asm/io.h> > > +void omap5_init_secondary(void); > +asm ( > +".text \n\t" > +"omap5_init_secondary: \n\t" > +" ldr r12, =0x102 \n\t" /* API_HYP_ENTRY */ > +" adr r0, init_secondary \n\t" You cannot use adr on external address for Arm32. This is because the immediate constant needs to have a specific format (see "Modified immediate constants in ARM instructions" A5.2.4 in ARM DDI 406C.c). Instead we would need something like: omap5_init_secondary: ldr r12, =0x102 adr r0, omap5_hyp smc #0 omap5_hyp: b init_secondary Note similar code would be needed for the stub file. > +" smc #0 \n\t" > +" b init_secondary \n\t" > +); > + > static uint16_t num_den[8][2] = { > { 0, 0 }, /* not used */ > { 26 * 64, 26 * 125 }, /* 12.0 Mhz */ > > > I personally find this favourable to introducing new stub files. > > Ultimately it is Julien/Stefano's decision, but I'd like to point it out as an > option for anyone who is unaware. Thank you for the suggestion :). This was suggested last week, but no-one came back explaining how it could be implemented. The two are fine with me. Cheers,
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S index 5f817d473e..120e034934 100644 --- a/xen/arch/arm/arm32/head.S +++ b/xen/arch/arm/arm32/head.S @@ -36,6 +36,10 @@ #include EARLY_PRINTK_INC #endif + +#define API_HYP_ENTRY 0x102 +#define AUX_CORE_BOOT0_PA 0x48281800 + /* * Common register usage in this file: * r0 - @@ -113,6 +117,12 @@ past_zImage: b common_start +GLOBAL(omap5_init_secondary) + ldr r12, =API_HYP_ENTRY + adr r0, init_secondary + dsb + smc #0 + GLOBAL(init_secondary) cpsid aif /* Disable all interrupts */ @@ -159,7 +169,6 @@ common_start: PRINT("- CPU doesn't support the virtualization extensions -\r\n") b fail 1: - /* Check that we're already in Hyp mode */ mrs r0, cpsr and r0, r0, #0x1f /* Mode is in the low 5 bits of CPSR */ diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c index aee24e4d28..6b5cc15af3 100644 --- a/xen/arch/arm/platforms/omap5.c +++ b/xen/arch/arm/platforms/omap5.c @@ -128,8 +128,9 @@ static int __init omap5_smp_init(void) } printk("Set AuxCoreBoot1 to %"PRIpaddr" (%p)\n", - __pa(init_secondary), init_secondary); - writel(__pa(init_secondary), wugen_base + OMAP_AUX_CORE_BOOT_1_OFFSET); + __pa(omap5_init_secondary), omap5_init_secondary); + writel(__pa(omap5_init_secondary), + wugen_base + OMAP_AUX_CORE_BOOT_1_OFFSET); printk("Set AuxCoreBoot0 to 0x20\n"); writel(0x20, wugen_base + OMAP_AUX_CORE_BOOT_0_OFFSET); diff --git a/xen/include/asm-arm/platforms/omap5.h b/xen/include/asm-arm/platforms/omap5.h index c559c84b61..732b27f403 100644 --- a/xen/include/asm-arm/platforms/omap5.h +++ b/xen/include/asm-arm/platforms/omap5.h @@ -22,6 +22,9 @@ #endif /* __ASM_ARM_PLATFORMS_OMAP5_H */ +/* Secondary cpu omap5 specific init routine */ +extern void omap5_init_secondary(void); + /* * Local variables: * mode: C
This function allows xen to bring secondary CPU cores into non-secure HYP mode. This is done by using a Secure Monitor call. Signed-off-by: Denis Obrezkov <denisobrezkov@gmail.com> --- xen/arch/arm/arm32/head.S | 11 ++++++++++- xen/arch/arm/platforms/omap5.c | 5 +++-- xen/include/asm-arm/platforms/omap5.h | 3 +++ 3 files changed, 16 insertions(+), 3 deletions(-)