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

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

[Denis Obrezkov]

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>
---
Changes in v2:
- move code to platform specific file
---
 xen/arch/arm/platforms/omap5.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index aee24e4d28..79764a6cd6 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -23,6 +23,17 @@
 #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, omap5_hyp        \n\t"
+        "        smc   #0                   \n\t"
+        "omap5_hyp:                         \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 */
@@ -128,8 +139,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);
-- 
2.20.1


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

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

[Julien Grall]

Hi Denis,

On 6/28/19 8:42 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.
As I pointed out in the previous version, the current code is likely 
working on some omap5 platform. So your commit message should provide 
some information on why this is necessary and on which platform (i.e 
beagleboard x15).

It would be nice to also have summary of why it is fine to extend to all 
the omap5 platform. For reminder, below the analysis I wrote earlier on:

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.

> 
> Signed-off-by: Denis Obrezkov <denisobrezkov@gmail.com>
> ---
> Changes in v2:
> - move code to platform specific file
> ---
>   xen/arch/arm/platforms/omap5.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
> index aee24e4d28..79764a6cd6 100644
> --- a/xen/arch/arm/platforms/omap5.c
> +++ b/xen/arch/arm/platforms/omap5.c
> @@ -23,6 +23,17 @@
>   #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, omap5_hyp        \n\t"
> +        "        smc   #0                   \n\t"
> +        "omap5_hyp:                         \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 */
> @@ -128,8 +139,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);
> 

Cheers,

-- 
Julien Grall

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

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

[Denis Obrezkov]

Hi Julien,

> 
> 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.
> I don't understand your concerns to the full extent. What should be
investigated?

-- 
Regards, Denis Obrezkov

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

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

[Julien Grall]

Hi Denis,

On 17/07/2019 17:32, Denis Obrezkov wrote:
>>
>> 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.
>> I don't understand your concerns to the full extent. What should be
> investigated?

Well, Xen has been supported the omap5 for quite a while. So there are two 
possibilities regarding the current SMP code:
    1) It is totally broken and therefore never worked on any omap5 platform.
    2) It works but with maybe modification in U-boot.

Looking at the mailing list archive and wiki, I believe 2) is closer to the 
current reality. So this raise the question whether your patch will break any 
existing setup.

Don't get me wrong, the code is perfectly fine as, to the best of my knowledge, 
it matches what Linux does. However, I would like to see an analysis written in 
the commit message regarding what's going to happen for any existing setup.

Cheers,

-- 
Julien Grall

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

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

[Denis Obrezkov]

Hi,

> 
> Well, Xen has been supported the omap5 for quite a while. So there are
> two possibilities regarding the current SMP code:
>    1) It is totally broken and therefore never worked on any omap5
> platform.
>    2) It works but with maybe modification in U-boot.
> 
> Looking at the mailing list archive and wiki, I believe 2) is closer to
> the current reality. So this raise the question whether your patch will
> break any existing setup.
> 
> Don't get me wrong, the code is perfectly fine as, to the best of my
> knowledge, it matches what Linux does. However, I would like to see an
> analysis written in the commit message regarding what's going to happen
> for any existing setup.
> 
> Cheers,
> 

I hoped to hear more opinions because I don't really understand where to
start - I don't know much about how xen and Linux worked with omap5.

-- 
Regards, Denis Obrezkov

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

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

[Julien Grall]

On 7/17/19 10:55 PM, Denis Obrezkov wrote:
> Hi,
Hi,

>> Well, Xen has been supported the omap5 for quite a while. So there are
>> two possibilities regarding the current SMP code:
>>     1) It is totally broken and therefore never worked on any omap5
>> platform.
>>     2) It works but with maybe modification in U-boot.
>>
>> Looking at the mailing list archive and wiki, I believe 2) is closer to
>> the current reality. So this raise the question whether your patch will
>> break any existing setup.
>>
>> Don't get me wrong, the code is perfectly fine as, to the best of my
>> knowledge, it matches what Linux does. However, I would like to see an
>> analysis written in the commit message regarding what's going to happen
>> for any existing setup.
>>
>> Cheers,
>>
> 
> I hoped to hear more opinions because I don't really understand where to
> start - I don't know much about how xen and Linux worked with omap5.

I am afraid I never had the chance to use the omap5. I provided an 
analysis in my first answer based on my finding. This is the kind of 
things I would expect in the commit message.

However, I would like more opinions from people who have been working 
with the omap5 platform. I would give until Monday for an answer, if 
none is received then please go ahead resending a version with my analysis.

Cheers,

-- 
Julien Grall

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

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

[Andrii Anisov]

Hello Julien,

I put my two cents in:

On 18.07.19 13:09, Julien Grall wrote:
> On 7/17/19 10:55 PM, Denis Obrezkov wrote:
>> Hi,
> Hi,
> 
>>> Well, Xen has been supported the omap5 for quite a while. So there are
>>> two possibilities regarding the current SMP code:
>>>     1) It is totally broken and therefore never worked on any omap5
>>> platform.
>>>     2) It works but with maybe modification in U-boot.

It works on Jacinto6 (dra7xx) with modifications in u-boot. As suggested in 4557c2292854d047ba8e44a69e2d60d99533d155. I'd cite the message:

     As part of this change drop support for switching from secure mode to NS HYP as
     well as the early CPU kick. Xen now requires that it is launched in NS HYP
     mode and that firmware configure things such that secondary CPUs can be woken
     up by a primarly CPU in HYP mode. This may require fixes to bootloaders or the
     use of a boot wrapper.

I'm not sure what Chen upstreamed for OMAP5. Prior to 4557c2292 there is no mode switch code for omap5. Maybe, it was a switch in u-boot from the beginning.

If we are talking about beagleboard-x15 it has no OMAP5, but has Sitara AM5728 SoC. Surprisingly, in am57xx-beagle-x15-common.dtsi, I see the following:
     
     compatible = "ti,am572x-beagle-x15", "ti,am5728", "ti,dra742", "ti,dra74", "ti,dra7";

I guess we also need another patch which adds "ti,am5728" as the platform supported by XEN.

>>> Looking at the mailing list archive and wiki, I believe 2) is closer to
>>> the current reality. So this raise the question whether your patch will
>>> break any existing setup.

I'd bet, the patch will not break support of any existing setup.
OMAP5 is discontinued long ago.
Jacinto6 (dra7xx) is from a conservative automotive world. Existing setups likely would not be updated. At least blindly. (I hope).
Sitara SoC's are not supported yet.

> if none is received then please go ahead resending a version with my analysis

 From my point of view, the commit message should describe, that we are switching from the model suggested in 4557c2292854d047ba8e44a69e2d60d99533d155 to a different one. With the appropriate reasoning, e.g. aligning to Linux kernel.

-- 
Sincerely,
Andrii Anisov.

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