Re: [PATCH 2/2] ARM: layerscape: ppa: Fix starting PPA

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 20.11.23 16:12, Sascha Hauer wrote:
> On Mon, Nov 20, 2023 at 03:54:40PM +0100, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 20.11.23 15:44, Sascha Hauer wrote:
>>> With dba1c26f70 we replaced request_sdram_region() for the PPA with
>>> reserve_sdram_region(). The effect is that the region is marked as
>>> reserved and mapped non executable. While this is desired for EL2, it
>>> also has the effect that we can't start the PPA anymore from EL3.
>>>
>>> Map the region cached/executable to start the PPA, then map it
>>> uncached/non executable once we are in EL2.
>>>
>>> Fixes: dba1c26f70 ("arm: layerscape: ppa: reserve SDRAM region for PPA")
>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>> ---
>>>  arch/arm/mach-layerscape/ppa.c | 18 +++++++++---------
>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
>>> index 521e6b89da..43145a7ece 100644
>>> --- a/arch/arm/mach-layerscape/ppa.c
>>> +++ b/arch/arm/mach-layerscape/ppa.c
>>> @@ -4,6 +4,7 @@
>>>  
>>>  #include <common.h>
>>>  #include <init.h>
>>> +#include <mmu.h>
>>>  #include <firmware.h>
>>>  #include <memory.h>
>>>  #include <linux/sizes.h>
>>> @@ -54,17 +55,11 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
>>>  	int ret;
>>>  	u32 *boot_loc_ptr_l, *boot_loc_ptr_h;
>>>  	struct ccsr_scfg __iomem *scfg = (void *)(LSCH2_SCFG_ADDR);
>>> -	int el = current_el();
>>>  	struct fit_handle *fit;
>>>  	void *conf;
>>>  	const void *buf;
>>>  	unsigned long firmware_size;
>>>  
>>> -	if (el < 3) {
>>> -		printf("EL%d, skip ppa init\n", el);
>>> -		return 0;
>>> -	}
>>> -
>>>  	boot_loc_ptr_l = &scfg->scratchrw[1];
>>>  	boot_loc_ptr_h = &scfg->scratchrw[0];
>>>  
>>> @@ -115,6 +110,7 @@ int ls1046a_ppa_init(resource_size_t ppa_start, resource_size_t ppa_size)
>>>  	struct resource *res;
>>>  	void *ppa_fw;
>>>  	size_t ppa_fw_size;
>>> +	int el = current_el();
>>>  	int ret;
>>>  
>>>  	res = reserve_sdram_region("ppa", ppa_start, ppa_size);
>>> @@ -126,9 +122,13 @@ int ls1046a_ppa_init(resource_size_t ppa_start, resource_size_t ppa_size)
>>>  
>>>  	get_builtin_firmware(ppa_ls1046a_bin, &ppa_fw, &ppa_fw_size);
>>>  
>>> -	ret = ppa_init(ppa_fw, ppa_fw_size, (void *)ppa_start);
>>> -	if (ret)
>>> -		return ret;
>>> +	if (el == 3) {
>>> +		remap_range((void *)ppa_start, ppa_size, MAP_CACHED);
>>> +		ret = ppa_init(ppa_fw, ppa_fw_size, (void *)ppa_start);
>>> +		remap_range((void *)ppa_start, ppa_size, MAP_UNCACHED);
>>> +		if (ret)
>>> +			return ret;
>>
>> Isn't this still racy? I think the EL2 mapping should already have the
>> PPA area uncached _before_ jumping into PPA.
> 
> It likely is, but to fix that we would have to create a second set of
> pagetables and use one for EL3 with PPA executable and another one for EL2
> with PPA non executable.
> 
> As of now this is purely academic anyway as the PPA doesn't protect its
> SDRAM range in any way, so speculating into it doesn't do any harm.

Fair enough. As long as speculation doesn't break anything, I guess we can
leave it like this for now. A comment would be nice though.

Cheers,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |