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@nxp.com CC: Alexander Graf agraf@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);
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.