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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3925 bytes --]

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 and asking for a Kconfig migration too :)

> 
> 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;
> > 
> 

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]