[U-Boot] [RFC PATCH] armv8: Extend modification of MMU tables

[york sun]

On 02/10/2017 09:34 AM, Stephen Warren wrote:
> On 02/03/2017 03:24 PM, York Sun wrote:
>> Device memory needs to be set along with PXN and UNX bits. Normal memory
>> must clear these bits. To support modification of PXN, UXN bits, extend
>> existing function mmu_set_region_dcache_behaviour() to accept attributes
>> directly. Also fix parsing d-cache option by removing extra shifting.
>
> I'm not sure that this "extra shifting" refers to; I don't see any shift
> changes in the patch.

Stephen,

This change gets rid of the extra shifting.

-	u64 attrs = PMD_ATTRINDX(option);

+	u64 attrs = option;



>
>> Signed-off-by: York Sun <york.sun at nxp.com>
>> CC: Alexander Graf <agraf at suse.de>
>> ---
>> Looks like original function mmu_set_region_dcache_behaviour() was written
>> to support changing d-cache option. However the PMD_ATTRINDX(option) shifts
>> it further higher. Maybe this function wasn't really used for ARMv8.
>
> It's used in at least a couple of cases:
>
> 1) The Raspberry Pi framebuffer code calls this function, and the rpi_3
> board is ARMv8. (Although I vaguely recall strange cache behaviour
> w.r.t. the framebuffer on the rpi_3 port, so perhaps this is related)
>
> 2) 64-bit Tegra that support PCIe and have an RTL8169 Ethernet card
> attached use noncached_alloc(), which internally calls
> mmu_set_region_dcache_behaviour() to set up the noncached region.
>
> (Those are just the systems I have experience with; there could well be
> other cases)

No issue with the cache?

>
>> I have a need to update existing MMU table with a little bit more than
>> d-cache options. With a recent debug on memory barrier, it came to my
>> attention that code should run on "normal" memory, while "device" memory
>> should have PXN and UXN bits set. A new function mmu_set_region_attr() is
>> hence introduced and mmu_set_region_dcache_behaviour() becomes a wrapper.
>>
>> BTW, if we don't plan to use "read_start" and "real_size" variables, they
>> should be removed.
>
>> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
>
>> @@ -509,8 +509,8 @@ static u64 set_one_region(u64 start, u64 size, u64 attrs, int level)
>>
>>  	/* Can we can just modify the current level block PTE? */
>>  	if (is_aligned(start, size, levelsize)) {
>> -		*pte &= ~PMD_ATTRINDX_MASK;
>> -		*pte |= attrs;
>> +		*pte &= ~PMD_ATTRMASK;
>> +		*pte |= attrs & PMD_ATTRMASK;
>
> This (plus the value chosen for PMD_ATTRMASK) modifies the function so
> that the PXN and UXN bits are over-written with the values take from
> attrs. However, set_on_region() is called from mmu_set_region_attr()
> which is called from mmu_set_region_dcache_behaviour(), which uses enum
> dcache_option values for attrs. However, the values of enum
> dcache_option do not include the PXN/UXN bits, so I believe this change
> will corrupt the page tables if PXN or UXN are already set.
>
> Still, I looked at all the "struct mm_region" tables in U-Boot and
> couldn't find any that set PXN/UXN for normal memory, and I don't
> believe this function will be called for anything other than normal
> memory. As such, this change should be fine in practice. However, I'm
> still a bit hesitant about this change. Can updating PXN/UXN be made
> optional? Related: Why would we need to change PXN/UXN at run-time;
> shouldn't the base struct mm_region table be set up correctly from the
> start?

In a recent debug, I found my MMU table was created _before_ the DDR was 
initialized. Having DDR as "normal memory" is risky. Speculative access 
may try to access DDR, resulting system hang at memory barrier 
instruction. This issue was not easy to reproduce. It depends on the 
code location and compiler. The fix to my issue was to set up DDR MMU 
afterward. I am experimenting both creating new mapping and modifying 
existing ones. That's when I realize current code doesn't let me change 
the PXN/UXN bits.

I am rewriting the code to do it differently. New patch will come out 
once I figure it out.

>
>> +void mmu_set_region_attr(phys_addr_t start, size_t size, u64 attrs)
> ...
>>  	/* We're done modifying page tables, switch back to our primary ones */
>> +	flush_dcache_range(gd->arch.tlb_addr,
>> +			   gd->arch.tlb_addr + gd->arch.tlb_size);
>>  	__asm_switch_ttbr(gd->arch.tlb_addr);
>
> Are the page table walks not coherent with the dcache? If not, I'm
> surprised we haven't had issues with this before. I think this change
> should be in a separate patch since it feels unrelated.
>
I guess it is not. I have found more than once that I had to flush the 
cache in order to load the new table.

Thanks for taking time to review this RFC. I appreciate the discussion 
and will change the code accordingly.

York