[PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

If CONFIG_PSCI is enabled then most likely we are running on
PSCI-enabled U-Boot which, we assume, has already taken care of
configuring ARCH timer stuff before switching to non-secure mode.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

---
A bit of context here...

We are highly interested in Renesas "Stout" board support (r8a7790)
in Xen hypervisor. The reason is to have fully supported HW for
performing "OSSTEST" (Xen automatic test system) on ARM32.

To reach that target we need a "generic way" for the secondary CPU cores
bring up and switching them to non-secure hyp mode.
So, the PSCI as a generic well-known way to bring up CPUs, was chosen
for that purpose. 

You can find corresponding patches for U-Boot here:
http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html

You can find corresponding patches for Xen hypervisor here:
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg43332.html

To sumarize:
Together with enabling CONFIG_PSCI in shmobile_defconfig, current patch
is a minimal required change needed to run mainline Linux on top of
PSCI-enabled U-Boot.
There is no need to modify device tree. U-Boot will take care of inserting
proper "enable-method" strings in CPU nodes.

---
 arch/arm/mach-shmobile/setup-rcar-gen2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
index eea60b2..bac4490 100644
--- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
+++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
@@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[] __initconst = {
 	{ /* sentinel */ }
 };
 
-static unsigned int __init get_extal_freq(void)
+static unsigned int __init __maybe_unused get_extal_freq(void)
 {
 	const struct of_device_id *match;
 	struct device_node *cpg, *extal;
@@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
 
 void __init rcar_gen2_timer_init(void)
 {
+/*
+ * If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
+ * U-Boot which, we assume, has already taken care of configuring ARCH timer
+ * stuff before switching to non-secure mode.
+ */
+#if !defined(CONFIG_ARM_PSCI)
 	void __iomem *base;
 	u32 freq;
 
@@ -101,6 +107,7 @@ void __init rcar_gen2_timer_init(void)
 	}
 
 	iounmap(base);
+#endif /* #if !defined(CONFIG_ARM_PSCI) */
 
 	of_clk_init(NULL);
 	timer_probe();
-- 
2.7.4

RE: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Biju Das]

Hi Oleksandr,

Thanks for the patch.

> Subject: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is
> enabled
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
> U-Boot which, we assume, has already taken care of configuring ARCH timer
> stuff before switching to non-secure mode.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> ---
> A bit of context here...
> 
> We are highly interested in Renesas "Stout" board support (r8a7790) in Xen
> hypervisor. The reason is to have fully supported HW for performing
> "OSSTEST" (Xen automatic test system) on ARM32.
> 
> To reach that target we need a "generic way" for the secondary CPU cores
> bring up and switching them to non-secure hyp mode.
> So, the PSCI as a generic well-known way to bring up CPUs, was chosen for
> that purpose.
> 
> You can find corresponding patches for U-Boot here:
> http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-
> SoC-Lager-Stout-boards-td357352.html
> 
> You can find corresponding patches for Xen hypervisor here:
> https://www.mail-archive.com/xen-
> devel@lists.xenproject.org/msg43332.html
> 
> To sumarize:
> Together with enabling CONFIG_PSCI in shmobile_defconfig, current patch is
> a minimal required change needed to run mainline Linux on top of PSCI-
> enabled U-Boot.
> There is no need to modify device tree. U-Boot will take care of inserting
> proper "enable-method" strings in CPU nodes.
> 
> ---
>  arch/arm/mach-shmobile/setup-rcar-gen2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-
> shmobile/setup-rcar-gen2.c
> index eea60b2..bac4490 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[]
> __initconst = {
>  	{ /* sentinel */ }
>  };
> 
> -static unsigned int __init get_extal_freq(void)
> +static unsigned int __init __maybe_unused get_extal_freq(void)
>  {
>  	const struct of_device_id *match;
>  	struct device_node *cpg, *extal;
> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
> 
>  void __init rcar_gen2_timer_init(void)
>  {
> +/*
> + * If CONFIG_PSCI is enabled then most likely we are running on
> +PSCI-enabled
> + * U-Boot which, we assume, has already taken care of configuring ARCH
> +timer
> + * stuff before switching to non-secure mode.
> + */
> +#if !defined(CONFIG_ARM_PSCI)
Is it required?  If you see the below comment, it is already taken care
by the below code.

Is your code entering into this block, when booting  the kernel in NS mode?

<snip>
  /*                                                                       
 87          * Update the timer if it is either not running, or is not at the        
 88          * right frequency. The timer is only configurable in secure mode        
 89          * so this avoids an abort if the loader started the timer and           
 90          * entered the kernel in non-secure mode.                                
 91          */                                                                      
 92                                                                                  
 93         if ((ioread32(base + CNTCR) & 1) == 0 ||                                 
 94             ioread32(base + CNTFID0) != freq) {                                  
 95                 /* Update registers with correct frequency */                    
 96                 iowrite32(freq, base + CNTFID0);                                 
 97                 asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));       
 98                                                                                  
 99                 /* make sure arch timer is started by setting bit 0 of CNTCR */  
100                 iowrite32(1, base + CNTCR);                                      
101         }     


Regards,
Biju

>  	void __iomem *base;
>  	u32 freq;
> 
> @@ -101,6 +107,7 @@ void __init rcar_gen2_timer_init(void)
>  	}
> 
>  	iounmap(base);
> +#endif /* #if !defined(CONFIG_ARM_PSCI) */
> 
>  	of_clk_init(NULL);
>  	timer_probe();
> --
> 2.7.4

Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Julien Grall]

Hi,

On 4/17/19 6:10 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> If CONFIG_PSCI is enabled then most likely we are running on
> PSCI-enabled U-Boot which, we assume, has already taken care of
> configuring ARCH timer stuff before switching to non-secure mode.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> ---
> A bit of context here...
> 
> We are highly interested in Renesas "Stout" board support (r8a7790)
> in Xen hypervisor. The reason is to have fully supported HW for
> performing "OSSTEST" (Xen automatic test system) on ARM32.
> 
> To reach that target we need a "generic way" for the secondary CPU cores
> bring up and switching them to non-secure hyp mode.
> So, the PSCI as a generic well-known way to bring up CPUs, was chosen
> for that purpose.
> 
> You can find corresponding patches for U-Boot here:
> http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html
> 
> You can find corresponding patches for Xen hypervisor here:
> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg43332.html
> 
> To sumarize:
> Together with enabling CONFIG_PSCI in shmobile_defconfig, current patch
> is a minimal required change needed to run mainline Linux on top of
> PSCI-enabled U-Boot.
> There is no need to modify device tree. U-Boot will take care of inserting
> proper "enable-method" strings in CPU nodes.
> 
> ---
>   arch/arm/mach-shmobile/setup-rcar-gen2.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> index eea60b2..bac4490 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[] __initconst = {
>   	{ /* sentinel */ }
>   };
>   
> -static unsigned int __init get_extal_freq(void)
> +static unsigned int __init __maybe_unused get_extal_freq(void)
>   {
>   	const struct of_device_id *match;
>   	struct device_node *cpg, *extal;
> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
>   
>   void __init rcar_gen2_timer_init(void)
>   {
> +/*
> + * If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
> + * U-Boot which, we assume, has already taken care of configuring ARCH timer
> + * stuff before switching to non-secure mode.
> + */

I disagree with that statement. CONFIG_ARM_PSCI may be selected in your 
.config for multiple reasons:
      - CONFIG_XEN will selected it
      - you build a .config targeting multiple platforms

Yet you may still want to boot the kernel on older U-boot. Such change 
would make it impossible.

If there are code that should not be called when PSCI is used, then you 
need to make sure the function is not called at runtime.

Cheers,

-- 
Julien Grall

Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Oleksandr]

On 18.04.19 10:14, Biju Das wrote:
> Hi Oleksandr,

Hi Biju

>
> Thanks for the patch.
>
>> Subject: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is
>> enabled
>>
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
>> U-Boot which, we assume, has already taken care of configuring ARCH timer
>> stuff before switching to non-secure mode.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> ---
>> A bit of context here...
>>
>> We are highly interested in Renesas "Stout" board support (r8a7790) in Xen
>> hypervisor. The reason is to have fully supported HW for performing
>> "OSSTEST" (Xen automatic test system) on ARM32.
>>
>> To reach that target we need a "generic way" for the secondary CPU cores
>> bring up and switching them to non-secure hyp mode.
>> So, the PSCI as a generic well-known way to bring up CPUs, was chosen for
>> that purpose.
>>
>> You can find corresponding patches for U-Boot here:
>> http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-
>> SoC-Lager-Stout-boards-td357352.html
>>
>> You can find corresponding patches for Xen hypervisor here:
>> https://www.mail-archive.com/xen-
>> devel@lists.xenproject.org/msg43332.html
>>
>> To sumarize:
>> Together with enabling CONFIG_PSCI in shmobile_defconfig, current patch is
>> a minimal required change needed to run mainline Linux on top of PSCI-
>> enabled U-Boot.
>> There is no need to modify device tree. U-Boot will take care of inserting
>> proper "enable-method" strings in CPU nodes.
>>
>> ---
>>   arch/arm/mach-shmobile/setup-rcar-gen2.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-
>> shmobile/setup-rcar-gen2.c
>> index eea60b2..bac4490 100644
>> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
>> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[]
>> __initconst = {
>>   	{ /* sentinel */ }
>>   };
>>
>> -static unsigned int __init get_extal_freq(void)
>> +static unsigned int __init __maybe_unused get_extal_freq(void)
>>   {
>>   	const struct of_device_id *match;
>>   	struct device_node *cpg, *extal;
>> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
>>
>>   void __init rcar_gen2_timer_init(void)
>>   {
>> +/*
>> + * If CONFIG_PSCI is enabled then most likely we are running on
>> +PSCI-enabled
>> + * U-Boot which, we assume, has already taken care of configuring ARCH
>> +timer
>> + * stuff before switching to non-secure mode.
>> + */
>> +#if !defined(CONFIG_ARM_PSCI)
> Is it required?  If you see the below comment, it is already taken care
> by the below code.
>
> Is your code entering into this block, when booting  the kernel in NS mode?

No, the code below seems to do the proper things. If arch timer is 
configured in U-Boot (enabled and programmed with correct freq) then

that code doesn't try to update it.

But "#if !defined(CONFIG_ARM_PSCI)" wraps secure_cntvoff_init() call as 
well, which resets CNTVOFF,

and this is something I tried to avoid the first from being executed on 
a CPU, which is in SVC mode (when running on top of Xen).


I was thinking whether I should wrap arch timer updating procedure as 
well and decided to do so, but probably I shouldn't have.

>
> <snip>
>    /*
>   87          * Update the timer if it is either not running, or is not at the
>   88          * right frequency. The timer is only configurable in secure mode
>   89          * so this avoids an abort if the loader started the timer and
>   90          * entered the kernel in non-secure mode.
>   91          */
>   92
>   93         if ((ioread32(base + CNTCR) & 1) == 0 ||
>   94             ioread32(base + CNTFID0) != freq) {
>   95                 /* Update registers with correct frequency */
>   96                 iowrite32(freq, base + CNTFID0);
>   97                 asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
>   98
>   99                 /* make sure arch timer is started by setting bit 0 of CNTCR */
> 100                 iowrite32(1, base + CNTCR);
> 101         }
>
>
> Regards,
> Biju
>
>>   	void __iomem *base;
>>   	u32 freq;
>>
>> @@ -101,6 +107,7 @@ void __init rcar_gen2_timer_init(void)
>>   	}
>>
>>   	iounmap(base);
>> +#endif /* #if !defined(CONFIG_ARM_PSCI) */
>>
>>   	of_clk_init(NULL);
>>   	timer_probe();
>> --
>> 2.7.4

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Oleksandr]

On 18.04.19 12:32, Julien Grall wrote:
> Hi,

Hi Julien


<snip>

>>
>> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c 
>> b/arch/arm/mach-shmobile/setup-rcar-gen2.c
>> index eea60b2..bac4490 100644
>> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
>> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
>> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[] 
>> __initconst = {
>>       { /* sentinel */ }
>>   };
>>   -static unsigned int __init get_extal_freq(void)
>> +static unsigned int __init __maybe_unused get_extal_freq(void)
>>   {
>>       const struct of_device_id *match;
>>       struct device_node *cpg, *extal;
>> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
>>     void __init rcar_gen2_timer_init(void)
>>   {
>> +/*
>> + * If CONFIG_PSCI is enabled then most likely we are running on 
>> PSCI-enabled
>> + * U-Boot which, we assume, has already taken care of configuring 
>> ARCH timer
>> + * stuff before switching to non-secure mode.
>> + */
>
> I disagree with that statement. CONFIG_ARM_PSCI may be selected in 
> your .config for multiple reasons:
>      - CONFIG_XEN will selected it
>      - you build a .config targeting multiple platforms
>
> Yet you may still want to boot the kernel on older U-boot. Such change 
> would make it impossible.
>
> If there are code that should not be called when PSCI is used, then 
> you need to make sure the function is not called at runtime.

I got your point.


As I answered in a previous email:

 > But "#if !defined(CONFIG_ARM_PSCI)" wraps secure_cntvoff_init() call as
 > well, which resets CNTVOFF,

 > and this is something I tried to avoid the first from being executed on
 > a CPU, which is in SVC mode (when running on top of Xen).

What would be a proper way to avoid calling secure_cntvoff_init() at 
runtime?

Check that we are already in non-secure mode and/or cntvoff is already 0?


>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Julien Grall]

Hi,

On 18/04/2019 12:15, Oleksandr wrote:
> 
> On 18.04.19 12:32, Julien Grall wrote:
>>> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c 
>>> b/arch/arm/mach-shmobile/setup-rcar-gen2.c
>>> index eea60b2..bac4490 100644
>>> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
>>> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
>>> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[] __initconst = {
>>>       { /* sentinel */ }
>>>   };
>>>   -static unsigned int __init get_extal_freq(void)
>>> +static unsigned int __init __maybe_unused get_extal_freq(void)
>>>   {
>>>       const struct of_device_id *match;
>>>       struct device_node *cpg, *extal;
>>> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
>>>     void __init rcar_gen2_timer_init(void)
>>>   {
>>> +/*
>>> + * If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
>>> + * U-Boot which, we assume, has already taken care of configuring ARCH timer
>>> + * stuff before switching to non-secure mode.
>>> + */
>>
>> I disagree with that statement. CONFIG_ARM_PSCI may be selected in your 
>> .config for multiple reasons:
>>      - CONFIG_XEN will selected it
>>      - you build a .config targeting multiple platforms
>>
>> Yet you may still want to boot the kernel on older U-boot. Such change would 
>> make it impossible.
>>
>> If there are code that should not be called when PSCI is used, then you need 
>> to make sure the function is not called at runtime.
> 
> I got your point.
> 
> 
> As I answered in a previous email:
> 
>  > But "#if !defined(CONFIG_ARM_PSCI)" wraps secure_cntvoff_init() call as
>  > well, which resets CNTVOFF,
> 
>  > and this is something I tried to avoid the first from being executed on
>  > a CPU, which is in SVC mode (when running on top of Xen).
> 
> What would be a proper way to avoid calling secure_cntvoff_init() at runtime?
> 
> Check that we are already in non-secure mode and/or cntvoff is already 0?

CNTVOFF is controlled by the hypervisor, so you cannot access it from Dom0.

If none of this code should be necessary when PSCI is enabled, then you can 
check the presence of PSCI (see psci_smp_available()).

Another solution is to check whether we are running on xen (see xen_domain()).

In any case, I would still surround the two checks because the "armgcnt" should 
really not be touched by Dom0.

Cheers,

-- 
Julien Grall

Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Oleksandr]

On 24.04.19 15:31, Julien Grall wrote:
> Hi,

Hi Julien


>
>>
>> As I answered in a previous email:
>>
>>  > But "#if !defined(CONFIG_ARM_PSCI)" wraps secure_cntvoff_init() 
>> call as
>>  > well, which resets CNTVOFF,
>>
>>  > and this is something I tried to avoid the first from being 
>> executed on
>>  > a CPU, which is in SVC mode (when running on top of Xen).
>>
>> What would be a proper way to avoid calling secure_cntvoff_init() at 
>> runtime?
>>
>> Check that we are already in non-secure mode and/or cntvoff is 
>> already 0?
>
> CNTVOFF is controlled by the hypervisor, so you cannot access it from 
> Dom0.
>
> If none of this code should be necessary when PSCI is enabled, then 
> you can check the presence of PSCI (see psci_smp_available()).


If I am not mistaken, U-Boot resets CNTVOFF before leaving monitor mode 
(switching to non-secure state). This happens if "support for booting in 
non-secure mode" is enabled.

This is a mandatory option for PSCI support as well as for support for 
hardware virtualization.

So the presence of PSCI would be an indicator, if CNTVOFF was configured 
in U-Boot and we don't need to.

 From other hand, Gen2 Linux may be booted in non-secure mode, without 
PSCI support...


I was thinking about checking the presence of "enable-method = psci" in 
CPU node, but probably psci_smp_available() is better option.

Also, there is a specific device-tree property 
"arm,cpu-registers-not-fw-configured" [1], but I am not sure whether it 
is appropriate to use here.


>
> Another solution is to check whether we are running on xen (see 
> xen_domain()).
>
> In any case, I would still surround the two checks because the 
> "armgcnt" should really not be touched by Dom0.

Agree.

I will add two checks in V2.


Thank you!


[1]

https://elixir.bootlin.com/linux/v5.1-rc6/source/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml#L68


>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko