Re: [Xen-devel] [PATCH] xen/arm: Switch OMAP5 secondary cores into hyp mode

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxx>
> > > ---
> > >   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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel