Re: [PATCH v2 5/6] arm: Use armv8_switch_to_el1 env to switch to EL1

[Andre Przywara <andre.przywara@arm.com>]

On 8/20/21 6:05 PM, Tom Rini wrote:
> On Fri, Aug 20, 2021 at 05:57:51PM +0100, Andre Przywara wrote:
>> On 8/19/21 4:53 PM, Peter Hoyes wrote:

Hi,

>>> From: Peter Hoyes <Peter.Hoyes@arm.com>
>>>
>>> Use the environment variable armv8_switch_to_el1 to determine whether
>>> to switch to EL1 at runtime. This is an alternative to the
>>> CONFIG_ARMV8_SWITCH_TO_EL1 compile-time option.
>>
>> This might be useful outside of the v8-R64 FVP. I cannot find
>> CONFIG_ARMV8_SWITCH_TO_EL1 being set anywhere, which makes me wonder how
>> this is used? Are there certain custom builds which define this somehow?
> 
> Adding Michal ...

Michal, I see that Xilinx Versal and ZynqMP have a commented "define 
CONFIG_ARMV8_SWITCH_TO_EL1" in their include headers. Can you say what 
is the use case here?
 From an architectural point of view it sounds useful to have this 
feature in general and the dynamic switch-ability on top of that for all 
ARM64 platforms, but I would first like to understand how this is used 
with those Xilink platforms.

> asking for a Kconfig migration too :)

That sounds reasonable. I will have a stab once we agree how to address 
this feature in general.

Cheers,
Andre

> 
>>
>> In any case forcing "kernel" entry in either EL1 or EL2, and deciding this
>> at runtime sounds useful for certain scenarios in general, and be it for
>> debugging and testing. So shall we get rid of this compile time option at
>> all, or shall this be retained to avoid extra code?
>>
>>> The environment variable will be ineffective if the ARMV8_MULTIENTRY
>>> config is used.
>>>
>>> This is required by the Armv8r64 architecture, which must be able to
>>> boot at S-EL1 for Linux but may need to boot at other ELs for other
>>> systems.
>>>
>>> Signed-off-by: Peter Hoyes <Peter.Hoyes@arm.com>
>>> ---
>>>    arch/arm/lib/bootm.c | 40 +++++++++++++++++++++++++---------------
>>>    1 file changed, 25 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
>>> index f60ee3a7e6..ea9bfe7570 100644
>>> --- a/arch/arm/lib/bootm.c
>>> +++ b/arch/arm/lib/bootm.c
>>> @@ -317,7 +317,6 @@ __weak void update_os_arch_secondary_cores(uint8_t os_arch)
>>>    {
>>>    }
>>> -#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
>>>    static void switch_to_el1(void)
>>>    {
>>>    	if ((IH_ARCH_DEFAULT == IH_ARCH_ARM64) &&
>>> @@ -332,7 +331,6 @@ static void switch_to_el1(void)
>>>    				    ES_TO_AARCH64);
>>>    }
>>>    #endif
>>> -#endif
>>>    /* Subcommand: GO */
>>>    static void boot_jump_linux(bootm_headers_t *images, int flag)
>>> @@ -359,21 +357,33 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
>>>    		update_os_arch_secondary_cores(images->os.arch);
>>> -#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
>>> -		armv8_switch_to_el2((u64)images->ft_addr, 0, 0, 0,
>>> -				    (u64)switch_to_el1, ES_TO_AARCH64);
>>> +#ifdef CONFIG_ARMV8_MULTIENTRY
>>> +		int armv8_switch_to_el1 = -1;
>>>    #else
>>> -		if ((IH_ARCH_DEFAULT == IH_ARCH_ARM64) &&
>>> -		    (images->os.arch == IH_ARCH_ARM))
>>> -			armv8_switch_to_el2(0, (u64)gd->bd->bi_arch_number,
>>> -					    (u64)images->ft_addr, 0,
>>> -					    (u64)images->ep,
>>> -					    ES_TO_AARCH32);
>>> -		else
>>> -			armv8_switch_to_el2((u64)images->ft_addr, 0, 0, 0,
>>> -					    images->ep,
>>> -					    ES_TO_AARCH64);
>>> +		int armv8_switch_to_el1 = env_get_yesno("armv8_switch_to_el1");
>>>    #endif
>>> +#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
>>> +		if (armv8_switch_to_el1 == -1) {
>>> +			armv8_switch_to_el1 = 1;
>>> +		}
>>> +#endif
>>> +		if (armv8_switch_to_el1 == 1) {
>>
>> This looks confusing. Can't we use CONFIG_IS_ENABLED() and override
>> armv8_switch_to_el1, then use this one variable to trigger the action?
>>
>> Cheers,
>> Andre
>>
>>> +			armv8_switch_to_el2((u64)images->ft_addr, 0, 0, 0,
>>> +					    (u64)switch_to_el1, ES_TO_AARCH64);
>>> +		} else {
>>> +			if ((IH_ARCH_DEFAULT == IH_ARCH_ARM64) &&
>>> +					(images->os.arch == IH_ARCH_ARM))
>>> +				armv8_switch_to_el2(0,
>>> +						    (u64)gd->bd->bi_arch_number,
>>> +						    (u64)images->ft_addr, 0,
>>> +						    (u64)images->ep,
>>> +						    ES_TO_AARCH32);
>>> +			else
>>> +				armv8_switch_to_el2((u64)images->ft_addr,
>>> +						    0, 0, 0,
>>> +						    images->ep,
>>> +						    ES_TO_AARCH64);
>>> +		}
>>>    	}
>>>    #else
>>>    	unsigned long machid = gd->bd->bi_arch_number;
>>>
>>
>