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

Denis Obrezkov posted 1 patch 17 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/ee1f4b9b969e6cf67278905e0405bc4fa5d6080c.1561147189.git.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(-)

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

Posted by Denis Obrezkov 17 weeks ago
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
+
 /*
  * 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
-- 
2.20.1


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

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

Posted by Julien Grall 17 weeks ago
(+ 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

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

Posted by Andrew Cooper 17 weeks ago
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Posted by Julien Grall 17 weeks ago
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,

-- 
Julien Grall

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