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

[Stephen Warren]

On 02/10/2017 10:51 AM, york sun wrote:
> 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;

Ah yes, I hadn't noticed that since the - and + lines weren't quite 
together 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)
>
> No issue with the cache?

As alluded to above, I vaguely recall the framebuffer drawing being 
slower on the RPi 3 than the earlier Pis, which could well be related to 
the dcache bits being incorrectly programmed. On Tegra I haven't noticed 
any cache issues with the current code and use-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?
>
> 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.

Yes, that makes sense. I bet it was hard to debug:-)

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