Re: [PATCH v2 08/10] ARM: mmu: use reserve mem entries to modify maps

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

Hi,

On 12.09.22 14:01, Sascha Hauer wrote:
> This patch breaks NAND support on my Phytec i.MX6 board. There are some
> problems with this patch, so I ended up reverting it for now.

I wonder why. I see no memory reserves in imx6q-phytec-phycore-som-nand.dts
and the files it includes.

> 
> On Wed, Aug 17, 2022 at 01:42:42PM +0200, Ahmad Fatoum wrote:
>> @@ -468,11 +469,28 @@ void __mmu_init(bool mmu_on)
>>  	vectors_init();
>>  
>>  	for_each_memory_bank(bank) {
>> +		struct resource *rsv;
>> +
>>  		create_sections(ttb, bank->start, bank->start + bank->size - 1,
>>  				PMD_SECT_DEF_CACHED);
>> -		__mmu_cache_flush();
>> +
>> +		for_each_reserved_region(bank, rsv) {
>> +			create_sections(ttb, resource_first_page(rsv),
>> +					resource_count_pages(rsv),
>> +					attrs_uncached_mem());
>> +		}
> 
> This operates on 1MiB sections, so everything requiring a finer
> granularity will fail here. Not sure if we currently need that, but not
> even issuing a warning is not good.

At worst it'd needlessly mark some memory uncached/XN. If users are
affected, they will notice and we can revisit this. I could add a debug
print here.

I need to rework this though, because I see now create_sections differs
between ARM64 and ARM32. On ARM64, it accepts the last address as argument,
while on ARM64, it's the size.. resource_count_pages() is not a nice
name either, because it returns bytes (aligned up to PAGE_SIZE).

> 
>>  	}
>>  
>> +	/*
>> +	 * We could set_ttbr(ttb) here instead and save on the copy, but
>> +	 * for now we play it safe, so we don't mess with the older ARMs.
>> +	 */
>> +	if (oldttb) {
>> +		memcpy(oldttb, ttb, ARM_TTB_SIZE);
>> +		free(ttb);
>> +	}
> 
> in the early MMU case the MMU still uses 'oldttb' as ttb whereas 'ttb'
> now points to invalid memory. Still functions like arm_create_pte()
> still operate on 'ttb'. It looks like a ttb = oldttb is missing here.

Why would ttb point at invalid memory? It's allocated unconditionally
with xmemalign and freed here.

> Also I wonder when we have to map the reserved regions as execute never,
> then the early MMU setup seems broken as well, as that creates a flat
> mapping without honoring the reserved regions. Shouldn't that be fixed?

Yes, see excerpt from cover letter:

"Note that this doesn't yet solve all problems. For example, PPA secure
 monitor installation on Layerscape may happen with CONFIG_MMU_EARLY=y,
 in which case barebox in EL2 may speculate into the secure memory
 before any device tree reserved-memory settings are considered. For this
 reason, both early MMU and normal MMU setup must be aware of the
 reserved memory regions. The original patch set by Rouven used FDT
 parsing in PBL to achieve this, but this is omitted here to limit
 scope of the patch series. Instead we only handle the CONFIG_OPTEE_SIZE
 case out-of-the-box."

My use case is the CONFIG_OPTEE_SIZE one and at least for ARM64, that looks
resolved now IMO.

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 |