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

[Stephen Warren]

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.

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

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

> +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.