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....@nxp.com>
CC: Alexander Graf <ag...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
