| Message ID | 1486160665-1485-1-git-send-email-york.sun@nxp.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
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); > __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.
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: > > 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? > >> 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); >> __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
On 02/10/2017 09:51 AM, york.sun@nxp.com wrote: > On 02/10/2017 09:34 AM, Stephen Warren wrote: >> >> 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? > I forgot to add here. If the DDR mapping is created _before_ DDR is initialized, it has to be device memory, not normal memory to avoid speculative access. I got from ARM support that the PXN/UXN must be set for device memory, otherwise it is architecturally undefined. I would like to skipping the mapping for DDR to begin with and add it back later. However, current code counts the highest address in predefined mapping and decides the TCR. So I have to have something for my highest mapping to hold the place. The only safe way is to map it as device memory with PXN/UXN to avoid hitting the speculative access issue. Now you see why I have to update the PXN/UXN bits. One more point, I have to avoid using the backup MMU table as current mmu_set_region_dcache_behaviour() does. For my early MMU table (long story why I have to use MMU early, in short, for performance on emulators), the space is very limited and I cannot afford extra space. I will follow ARM's suggestion to do break-before-make to update MMU. 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: >> >> 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 >
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
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",
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(-)