[U-Boot] [RFC PATCH] armv8: Extend modification of MMU tables
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.
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. 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.
arch/arm/cpu/armv8/cache_v8.c | 28 +++++++++++++++++++--------- arch/arm/include/asm/armv8/mmu.h | 1 + arch/arm/include/asm/system.h | 1 + 3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index afa76c1..3b2f3f8 100644 --- 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; debug("Set attrs=%llx pte=%p level=%d\n", attrs, pte, level);
return levelsize; @@ -532,13 +532,8 @@ static u64 set_one_region(u64 start, u64 size, u64 attrs, int level) return 0; }
-void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, - enum dcache_option option) +void mmu_set_region_attr(phys_addr_t start, size_t size, u64 attrs) { - u64 attrs = PMD_ATTRINDX(option); - u64 real_start = start; - u64 real_size = size; - debug("start=%lx size=%lx\n", (ulong)start, (ulong)size);
if (!gd->arch.tlb_emerg) @@ -572,7 +567,19 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, }
/* 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); +} + +void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, + enum dcache_option option) +{ + u64 attrs = option; + u64 real_start = start; + u64 real_size = size; + + mmu_set_region_attr(start, size, attrs);
/* * Make sure there's nothing stale in dcache for a region that might @@ -580,7 +587,6 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, */ flush_dcache_range(real_start, real_start + real_size); } - #else /* CONFIG_SYS_DCACHE_OFF */
/* @@ -613,6 +619,10 @@ int dcache_status(void) return 0; }
+void mmu_set_region_attr(phys_addr_t start, size_t size, u64 attrs) +{ +} + void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, enum dcache_option option) { diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h index 58aecb9..c1cff23 100644 --- a/arch/arm/include/asm/armv8/mmu.h +++ b/arch/arm/include/asm/armv8/mmu.h @@ -86,6 +86,7 @@ */ #define PMD_ATTRINDX(t) ((t) << 2) #define PMD_ATTRINDX_MASK (7 << 2) +#define PMD_ATTRMASK (PTE_BLOCK_PXN | PTE_BLOCK_UXN | PMD_ATTRINDX_MASK)
/* * TCR flags. diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index dc4c991..81709ec 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -224,6 +224,7 @@ void protect_secure_region(void); void smp_kick_all_cpus(void);
void flush_l3_cache(void); +void mmu_set_region_attr(phys_addr_t start, size_t size, u64 attrs);
/* *Issue a secure monitor call in accordance with ARM "SMC Calling convention",
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.
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@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:
- 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)
- 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);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
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@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:
- 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)
- 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);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
On 02/03/2017 02: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.
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. 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.
Let's skip this patch for now. I have sent another set to address my issue with MMU update. We will need another patch to address the mistaken index.
York
participants (3)
-
Stephen Warren -
York Sun -
york sun