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@xxxxxxxxxxxxxx>
>>> ---
>>> 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 |
