From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Date: Thu, 5 Jun 2014 18:41:56 +0100 Subject: [U-Boot] [Patch v4 2/5] ARMv8: Adjust MMU setup In-Reply-To: <539087A5.4090803@freescale.com> References: <1401396548-3353-1-git-send-email-yorksun@freescale.com> <1401396548-3353-2-git-send-email-yorksun@freescale.com> <20140602113450.GD13573@leverpostej> <538CA0F5.2060103@freescale.com> <20140602180100.GA887@leverpostej> <538F48F2.3080408@freescale.com> <20140605100926.GA6430@leverpostej> <539087A5.4090803@freescale.com> Message-ID: <20140605174156.GG31564@leverpostej> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Jun 05, 2014 at 04:07:17PM +0100, York Sun wrote: > On 06/05/2014 03:09 AM, Mark Rutland wrote: > > On Wed, Jun 04, 2014 at 05:27:30PM +0100, York Sun wrote: > >> On 06/02/2014 11:01 AM, Mark Rutland wrote: > >>> On Mon, Jun 02, 2014 at 05:06:13PM +0100, York Sun wrote: > >>>> On 06/02/2014 04:34 AM, Mark Rutland wrote: > >>>>> On Thu, May 29, 2014 at 09:49:05PM +0100, York Sun wrote: > >>>>>> Make MMU functions reusable. Platform code can setup its own MMU tables. > >>>>> > >>>>> What exactly does platform code need to setup its own tables for? > >>>> > >>>> The general ARMv8 MMU table is not detail enough to control memory attribute > >>>> like cache for all addresses. We have devices mapping to addresses with > >>>> different requirement for cache control. > >>> > >>> And there are no APIs for creating device mappings rather than exporting > >>> the raw pagetable accessors and hard-coding them differently in every > >>> board file? > >>> > >> > >> That's a good question. At this point, only two platforms are using ARMv8 code. > >> I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by the > >> file I added. If that's not the case, or more ARMv8 SoCs need special MMU table, > >> we then should introduce such API. Having a full function MMU API may be an > >> overkill for U-boot. We don't need dynamic MMU anyway. > > > > Maybe. It just seems to me that it would be possible to pre-allocate an > > empty table that we could place device (nGnRnE?) mappings in. Then all > > you'd need to call from board code is a function to map a range, rather > > than having to duplicate logic for creating the tables you want. > > It sounds good, but not the case. For the three level tables I am using (level0, > level1, level2), I don't have level2 table for every address, that will be too > many. Instead, I have a lot of blocks for level1. When I need some fine control > within a level1 block range, I have to create a new level2 table. It is doable, > but I will hold on that if I can use static table. While my suggestion might not be the best, I'm not sure I follow, unless you always want to idmap devices? If you don't idmap devices, then you can place all of the disparate physical mappings within a single table unless you have very large peripherals to map? > > > > >> > >>>> > >>>>> > >>>>>> Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c. > >>>>>> > >>>>>> Signed-off-by: York Sun > >>>>>> CC: David Feng > >>>>>> --- > >>>>>> Change log: > >>>>>> v4: new patch, splitted from v3 2/4 > >>>>>> Revise set_pgtable_section() to be reused by platform MMU code > >>>>>> Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code > >>>>>> > >>>>>> arch/arm/cpu/armv8/cache_v8.c | 49 ++++++++++++++++---------------------- > >>>>>> arch/arm/include/asm/armv8/mmu.h | 23 ++++++++++++++++++ > >>>>>> 2 files changed, 43 insertions(+), 29 deletions(-) > >>>>>> > >>>>>> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > >>>>>> index a96ecda..67dbd46 100644 > >>>>>> --- a/arch/arm/cpu/armv8/cache_v8.c > >>>>>> +++ b/arch/arm/cpu/armv8/cache_v8.c > >>>>>> @@ -12,15 +12,14 @@ > >>>>>> DECLARE_GLOBAL_DATA_PTR; > >>>>>> > >>>>>> #ifndef CONFIG_SYS_DCACHE_OFF > >>>>>> - > >>>>>> -static void set_pgtable_section(u64 section, u64 memory_type) > >>>>>> +void set_pgtable_section(u64 *page_table, u64 index, u64 section, > >>>>>> + u64 memory_type) > >>>>>> { > >>>>>> - u64 *page_table = (u64 *)gd->arch.tlb_addr; > >>>>>> u64 value; > >>>>>> > >>>>>> - value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF; > >>>>>> + value = section | PMD_TYPE_SECT | PMD_SECT_AF; > >>>>>> value |= PMD_ATTRINDX(memory_type); > >>>>>> - page_table[section] = value; > >>>>>> + page_table[index] = value; > >>>>>> } > >>>>>> > >>>>>> /* to activate the MMU we need to set up virtual memory */ > >>>>>> @@ -28,10 +27,13 @@ static void mmu_setup(void) > >>>>>> { > >>>>>> int i, j, el; > >>>>>> bd_t *bd = gd->bd; > >>>>>> + u64 *page_table = (u64 *)gd->arch.tlb_addr; > >>>>>> > >>>>>> /* Setup an identity-mapping for all spaces */ > >>>>>> - for (i = 0; i < (PGTABLE_SIZE >> 3); i++) > >>>>>> - set_pgtable_section(i, MT_DEVICE_NGNRNE); > >>>>>> + for (i = 0; i < (PGTABLE_SIZE >> 3); i++) { > >>>>>> + set_pgtable_section(page_table, i, i << SECTION_SHIFT, > >>>>>> + MT_DEVICE_NGNRNE); > >>>>>> + } > >>>>>> > >>>>>> /* Setup an identity-mapping for all RAM space */ > >>>>>> for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { > >>>>>> @@ -39,36 +41,25 @@ static void mmu_setup(void) > >>>>>> ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size; > >>>>>> for (j = start >> SECTION_SHIFT; > >>>>>> j < end >> SECTION_SHIFT; j++) { > >>>>>> - set_pgtable_section(j, MT_NORMAL); > >>>>>> + set_pgtable_section(page_table, j, j << SECTION_SHIFT, > >>>>>> + MT_NORMAL); > >>>>>> } > >>>>>> } > >>>>>> > >>>>>> /* load TTBR0 */ > >>>>>> el = current_el(); > >>>>>> if (el == 1) { > >>>>>> - asm volatile("msr ttbr0_el1, %0" > >>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); > >>>>>> - asm volatile("msr tcr_el1, %0" > >>>>>> - : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS) > >>>>>> - : "memory"); > >>>>>> - asm volatile("msr mair_el1, %0" > >>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); > >>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, > >>>>>> + TCR_FLAGS | TCR_EL1_IPS_BITS, > >>>>>> + MEMORY_ATTRIBUTES); > >>>>>> } else if (el == 2) { > >>>>>> - asm volatile("msr ttbr0_el2, %0" > >>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); > >>>>>> - asm volatile("msr tcr_el2, %0" > >>>>>> - : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS) > >>>>>> - : "memory"); > >>>>>> - asm volatile("msr mair_el2, %0" > >>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); > >>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, > >>>>>> + TCR_FLAGS | TCR_EL2_IPS_BITS, > >>>>>> + MEMORY_ATTRIBUTES); > >>>>>> } else { > >>>>>> - asm volatile("msr ttbr0_el3, %0" > >>>>>> - : : "r" (gd->arch.tlb_addr) : "memory"); > >>>>>> - asm volatile("msr tcr_el3, %0" > >>>>>> - : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS) > >>>>>> - : "memory"); > >>>>>> - asm volatile("msr mair_el3, %0" > >>>>>> - : : "r" (MEMORY_ATTRIBUTES) : "memory"); > >>>>>> + set_ttbr_tcr_mair(el, gd->arch.tlb_addr, > >>>>>> + TCR_FLAGS | TCR_EL3_IPS_BITS, > >>>>>> + MEMORY_ATTRIBUTES); > >>>>>> } > >>>>>> > >>>>>> /* enable the mmu */ > >>>>>> diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h > >>>>>> index 1193e76..7de4ff9 100644 > >>>>>> --- a/arch/arm/include/asm/armv8/mmu.h > >>>>>> +++ b/arch/arm/include/asm/armv8/mmu.h > >>>>>> @@ -108,4 +108,27 @@ > >>>>>> TCR_IRGN_WBWA | \ > >>>>>> TCR_T0SZ(VA_BITS)) > >>>>>> > >>>>>> +#ifndef __ASSEMBLY__ > >>>>>> +void set_pgtable_section(u64 *page_table, u64 index, > >>>>>> + u64 section, u64 memory_type); > >>>>>> +static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr) > >>>>>> +{ > >>>>>> + asm volatile("dsb sy;isb"); > >>>>> > >>>>> Huh? This wasn't anywhere before. Is the isb necessary? > >>>> > >>>> Probably not, but to make sure all previous instructions finish. > >>> > >>> Which instructions do you care about completing? > >>> > >>> You'll certainly want any page table writes to complete, but is anything > >>> else in-flight at this point in time? > >> > >> Before calling this inline function, MMU tables were created. We want the > >> writing completes before setting these registers. > > > > Sure, but the dsb sy guarantees that. > > > > The dsb sy will ensure that all reads and writes before it have become > > visible to all observers in the full system shareability domain > > (including the MMU) before any subsequent instructions (in program > > order) can execute. > > > > I'd only expect to see a "dsb $DOMAIN; isb" sequence is when we're > > changing things that affect the instruction stream (I-cache maintenance, > > changing the mapping underlying the currently executing stream). > > Thanks. I can drop "isb". Please comment on v5 patches. Will do momentarily. > > > > >>> > >>>>> > >>>>> When is this function expected to be called? Is the MMU expected to be > >>>>> on? > >>>> > >>>> The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it > >>>> is called when MMU is off for the first time, but MMU on for the 2nd time. > >>> > >>> Ok. > >>> > >>>> > >>>>> > >>>>>> + if (el == 1) { > >>>>>> + asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory"); > >>>>>> + asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory"); > >>>>>> + asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory"); > >>>>> > >>>>> If the MMU is on, this looks really scary -- what are you expecting to > >>>>> change in a single invocation? > >>>> > >>>> It is not scary for general ARMv8 code. MMU is off then this is called. For FSL > >>>> SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call > >>>> points to a new MMU table. > >>> > >>> Oh but it is ;) > >>> > >>>> > >>>>> > >>>>>> + } else if (el == 2) { > >>>>>> + asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory"); > >>>>>> + asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory"); > >>>>>> + asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory"); > >>>>>> + } else if (el == 3) { > >>>>>> + asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory"); > >>>>>> + asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory"); > >>>>>> + asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory"); > >>>>>> + } else { > >>>>>> + hang(); > >>>>>> + } > >>>>> > >>>>> And no synchronisation to ensure that these writes are complete or even > >>>>> ordered w.r.t. each other? > >>>>> > >>>> > >>>> That's why I added asm volatile("dsb sy;isb") before them. The order of these > >>>> write doesn't matter. See the code before my change > >>>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD > >>> > >>> Ok, so that orders them against everything _before_ them, but not with > >>> respect to each other, or anything _after_ them. > >>> > >>> When the MMU is off, you are correct that the ordering of these > >>> operations w.r.t. each other doesn't matter. With the MMU on that's not > >>> true as the CPU can rerder the operations and can walk the page tables > >>> asynchronously. > >>> > >>> Changing the MAIR at runtime is somewhat scary because you may be > >>> changing attributes for entries which are active in the cache and/or > >>> TLBs. I'm not sure I follow why you need to change it once the MMU is on > >>> -- there are plenty of subfields in the MAIR that you could configure > >>> before turning the MMU on for use later. > >>> > >>> Likewise changing the TCR at runtime is somewhat scary because you can > >>> change attributes of active cache and/or TLB entries, change fields that > >>> affect the way page tables are walked (TG*. T*SZ), all asynchronously > >>> w.r.t. the logic that walsk the page tables. > >>> > >>> Changing the TTBR is fine, except that we didn't invalidate old entries > >>> with a TLBI, so we may even have partial translations cahced in the > >>> TLBs, and we can get odd translations generated from the combination of > >>> the old and new tables. > >> > >> > >> Maybe a more proper way to do so is to change TTBR only. That's actually what I > >> really need. Or if I really need to change all of them, I should turn off MMU first. > > > > In fact, if you had an API for creating device mappings, you wouldn't > > even need to change the TTBR -- you'd just need to replace some existing > > invalid entries with the new mapping, make them visible to the MMU with > > a dsb, and then carry on. The TLBs aren't allowed to cache invalid > > mappings, so on the next access to those mappings, the appropriate page > > table entry will be read into the TLBs. > > > > If you alter multiple levels of a live table then you'll need barriers > > (DMBs) between populating those levels. If you back device mappings by a > > single level of table then you can avoid that. Even better, with an API > > you can do the simple thing today then make it more advanced in future > > if necessary without having to change your board code. > > > > No objection here on the idea. But again this is not the case. My first MMU > table is in SRAM, which is small and will be used for other purpose. The 2nd MMU > table is in DDR. I could copy the table and do the maintenance as you said. For > now, I want to stick with the static table and only create the API when I have to. Sure, if your tables are in SRAM then trying to do a load of dynamic allocation isn't going to work. My fear is that while that sounds OK with a single user to do a quick havk and poke the tables directly, we'll end up with everyone doing that, and no-one will try to unify things. It is very diffifcult to unify such variation after the fact. > >>>> For the 2nd write used by FSL SoC, I need to flush the cache to make sure the > >>>> table is in main DDR and perform TLB invalidation. That's when the loading of > >>>> new MMU table happens. > >>> > >>> I missed the TLB invaliation -- where is that meant to happen? > >>> > >> After flushing the new MMU table into DDR. To your point above, I will send a > >> new version with updating TTBR only. > > > > Unfortunately, that's too early. The MMU is asynchronous w.r.t. the CPU > > and so it can re-read the existing tables into the TLBs _before_ it has > > been programmed with the new table address. The TLBI has to happen > > _after_ the old tables are no longer pointed to by the TTBR. > > > > What you need to do to replace the active set of tables (assuming that > > the new mapping has the instruction stream mapped in an identical way) > > is: > > > > - Write the tables. > > > > - DSB to make them visible to the MMU. > > > > - Write to the appropriate TTBR_*. > > > > - ISB to complete the write to the TTBR_*. > > > > - TLBI to invalidate the old mappings the the TLBs. > > > > - DSB to complete the TLBI. > > > > - If you've changed the instruction stream or system state that will > > affect the instruction stream, ISB to flush the CPU pipeline. > > > > > Here is the flow I have (as of v5 patch) > > Write the tables > > (I removed dsb here in v5, need to add back) > > Write TTBR > > (I missed isb here, need to add) > > Flush dcache (otherwise the table will not be in DDR. Yes, I verified) This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU? > TLBI > > (DSB and ISB TLBI function) Why another TLBI? > > I will need to send v6 patch to add the missing parts. Sure, I'll hold off replies here until then. Cheers, Mark