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

[Julien Grall <julien.grall@arm.com>]

(+ 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

-- 
Julien Grall

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