| Message ID | 1429112064-19952-10-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 7 May 2015 at 18:54, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Wed, Apr 15, 2015 at 05:34:20PM +0200, Ard Biesheuvel wrote: >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> index ceec4def354b..338eaa7bcbfd 100644 >> --- a/arch/arm64/kernel/vmlinux.lds.S >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> @@ -68,6 +68,17 @@ PECOFF_FILE_ALIGNMENT = 0x200; >> #define ALIGN_DEBUG_RO_MIN(min) . = ALIGN(min); >> #endif >> >> +/* >> + * The pgdir region needs to be mappable using a single PMD or PUD sized region, >> + * so it should not cross a 512 MB or 1 GB alignment boundary, respectively >> + * (depending on page size). So align to an upper bound of its size. >> + */ >> +#if CONFIG_ARM64_PGTABLE_LEVELS == 2 >> +#define PGDIR_ALIGN (8 * PAGE_SIZE) >> +#else >> +#define PGDIR_ALIGN (16 * PAGE_SIZE) >> +#endif > > Isn't 8 pages sufficient in both cases? Unless some other patch changes > the idmap and swapper, I can count maximum 7 pages in total. > The preceding patch moves the fixmap page tables to this region as well. But the logic is still incorrect -> we only need 16x for 4 levels (7 + 3 == 10), the remaining ones are all <= 8 >> + >> SECTIONS >> { >> /* >> @@ -160,7 +171,7 @@ SECTIONS >> >> BSS_SECTION(0, 0, 0) >> >> - .pgdir (NOLOAD) : ALIGN(PAGE_SIZE) { >> + .pgdir (NOLOAD) : ALIGN(PGDIR_ALIGN) { >> idmap_pg_dir = .; >> . += IDMAP_DIR_SIZE; >> swapper_pg_dir = .; >> @@ -185,6 +196,11 @@ ASSERT(__idmap_text_end - (__idmap_text_start & ~(SZ_4K - 1)) <= SZ_4K, >> "ID map text too big or misaligned") >> >> /* >> + * Check that the chosen PGDIR_ALIGN value if sufficient. >> + */ >> +ASSERT(SIZEOF(.pgdir) < ALIGNOF(.pgdir), ".pgdir size exceeds its alignment") >> + >> +/* >> * If padding is applied before .head.text, virt<->phys conversions will fail. >> */ >> ASSERT(_text == (PAGE_OFFSET + TEXT_OFFSET), "HEAD is misaligned") >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index c27ab20a5ba9..93e5a2497f01 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -380,26 +380,68 @@ static void __init bootstrap_early_mapping(unsigned long addr, >> } >> } >> >> +static void __init bootstrap_linear_mapping(unsigned long va_offset) >> +{ >> + /* >> + * Bootstrap the linear range that covers swapper_pg_dir so that the >> + * statically allocated page tables as well as newly allocated ones >> + * are accessible via the linear mapping. >> + */ > > Just move the comment outside the function. > OK >> + static struct bootstrap_pgtables linear_bs_pgtables __pgdir; >> + const phys_addr_t swapper_phys = __pa(swapper_pg_dir); >> + unsigned long swapper_virt = __phys_to_virt(swapper_phys) + va_offset; >> + struct memblock_region *reg; >> + >> + bootstrap_early_mapping(swapper_virt, &linear_bs_pgtables, >> + IS_ENABLED(CONFIG_ARM64_64K_PAGES)); >> + >> + /* now find the memblock that covers swapper_pg_dir, and clip */ >> + for_each_memblock(memory, reg) { >> + phys_addr_t start = reg->base; >> + phys_addr_t end = start + reg->size; >> + unsigned long vstart, vend; >> + >> + if (start > swapper_phys || end <= swapper_phys) >> + continue; >> + >> +#ifdef CONFIG_ARM64_64K_PAGES >> + /* clip the region to PMD size */ >> + vstart = max(swapper_virt & PMD_MASK, >> + round_up(__phys_to_virt(start + va_offset), >> + PAGE_SIZE)); >> + vend = min(round_up(swapper_virt, PMD_SIZE), >> + round_down(__phys_to_virt(end + va_offset), >> + PAGE_SIZE)); >> +#else >> + /* clip the region to PUD size */ >> + vstart = max(swapper_virt & PUD_MASK, >> + round_up(__phys_to_virt(start + va_offset), >> + PMD_SIZE)); >> + vend = min(round_up(swapper_virt, PUD_SIZE), >> + round_down(__phys_to_virt(end + va_offset), >> + PMD_SIZE)); >> +#endif >> + >> + create_mapping(__pa(vstart - va_offset), vstart, vend - vstart, >> + PAGE_KERNEL_EXEC); >> + >> + /* >> + * Temporarily limit the memblock range. We need to do this as >> + * create_mapping requires puds, pmds and ptes to be allocated >> + * from memory addressable from the early linear mapping. >> + */ >> + memblock_set_current_limit(__pa(vend - va_offset)); >> + >> + return; >> + } >> + BUG(); >> +} > > I'll probably revisit this function after I see the whole series. But in > the meantime, if the kernel is not loaded in the first memblock (in > address order), isn't there a risk that we allocate memory from the > first memblock which is not mapped yet? > memblock allocates top down, so it should only allocate from this region, unless the remaining room is completely reserved. I think that is a theoretical problem which exists currently as well, i.e., the boot protocol does not mandate that the 512MB/1GB region containing the kernel contains unreserved room.
On 8 May 2015 at 16:44, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, May 07, 2015 at 09:21:28PM +0200, Ard Biesheuvel wrote: >> On 7 May 2015 at 18:54, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Wed, Apr 15, 2015 at 05:34:20PM +0200, Ard Biesheuvel wrote: >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> >> index ceec4def354b..338eaa7bcbfd 100644 >> >> --- a/arch/arm64/kernel/vmlinux.lds.S >> >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> >> @@ -68,6 +68,17 @@ PECOFF_FILE_ALIGNMENT = 0x200; >> >> #define ALIGN_DEBUG_RO_MIN(min) . = ALIGN(min); >> >> #endif >> >> >> >> +/* >> >> + * The pgdir region needs to be mappable using a single PMD or PUD sized region, >> >> + * so it should not cross a 512 MB or 1 GB alignment boundary, respectively >> >> + * (depending on page size). So align to an upper bound of its size. >> >> + */ >> >> +#if CONFIG_ARM64_PGTABLE_LEVELS == 2 >> >> +#define PGDIR_ALIGN (8 * PAGE_SIZE) >> >> +#else >> >> +#define PGDIR_ALIGN (16 * PAGE_SIZE) >> >> +#endif >> > >> > Isn't 8 pages sufficient in both cases? Unless some other patch changes >> > the idmap and swapper, I can count maximum 7 pages in total. >> >> The preceding patch moves the fixmap page tables to this region as well. >> But the logic is still incorrect -> we only need 16x for 4 levels (7 + >> 3 == 10), the remaining ones are all <= 8 > > You should improve the comment here to include the maths, "upper bound > of its size" is not very clear ;). > Yes, you are right, it should read 'power-of-2 upper bound' >> >> + static struct bootstrap_pgtables linear_bs_pgtables __pgdir; >> >> + const phys_addr_t swapper_phys = __pa(swapper_pg_dir); >> >> + unsigned long swapper_virt = __phys_to_virt(swapper_phys) + va_offset; >> >> + struct memblock_region *reg; >> >> + >> >> + bootstrap_early_mapping(swapper_virt, &linear_bs_pgtables, >> >> + IS_ENABLED(CONFIG_ARM64_64K_PAGES)); >> >> + >> >> + /* now find the memblock that covers swapper_pg_dir, and clip */ >> >> + for_each_memblock(memory, reg) { >> >> + phys_addr_t start = reg->base; >> >> + phys_addr_t end = start + reg->size; >> >> + unsigned long vstart, vend; >> >> + >> >> + if (start > swapper_phys || end <= swapper_phys) >> >> + continue; >> >> + >> >> +#ifdef CONFIG_ARM64_64K_PAGES >> >> + /* clip the region to PMD size */ >> >> + vstart = max(swapper_virt & PMD_MASK, >> >> + round_up(__phys_to_virt(start + va_offset), >> >> + PAGE_SIZE)); >> >> + vend = min(round_up(swapper_virt, PMD_SIZE), >> >> + round_down(__phys_to_virt(end + va_offset), >> >> + PAGE_SIZE)); >> >> +#else >> >> + /* clip the region to PUD size */ >> >> + vstart = max(swapper_virt & PUD_MASK, >> >> + round_up(__phys_to_virt(start + va_offset), >> >> + PMD_SIZE)); >> >> + vend = min(round_up(swapper_virt, PUD_SIZE), >> >> + round_down(__phys_to_virt(end + va_offset), >> >> + PMD_SIZE)); >> >> +#endif >> >> + >> >> + create_mapping(__pa(vstart - va_offset), vstart, vend - vstart, >> >> + PAGE_KERNEL_EXEC); >> >> + >> >> + /* >> >> + * Temporarily limit the memblock range. We need to do this as >> >> + * create_mapping requires puds, pmds and ptes to be allocated >> >> + * from memory addressable from the early linear mapping. >> >> + */ >> >> + memblock_set_current_limit(__pa(vend - va_offset)); >> >> + >> >> + return; >> >> + } >> >> + BUG(); >> >> +} >> > >> > I'll probably revisit this function after I see the whole series. But in >> > the meantime, if the kernel is not loaded in the first memblock (in >> > address order), isn't there a risk that we allocate memory from the >> > first memblock which is not mapped yet? >> >> memblock allocates top down, so it should only allocate from this >> region, unless the remaining room is completely reserved. > > I don't like to rely on this, it's not guaranteed behaviour. > Actually, it is. Allocation is always top-down unless you call memblock_set_bottom_up(), which is a NOP unless CONFIG_MOVABLE_NODE is selected. That is why the memblock limit only limits at the top afaict >> I think that is a theoretical problem which exists currently as well, >> i.e., the boot protocol does not mandate that the 512MB/1GB region >> containing the kernel contains unreserved room. > > That's more of a documentation problem, we can make the requirements > clearer. Debugging is probably easier as well, it fails to allocate > memory. But for the other case, not placing the kernel in the first > memblock has high chances of allocating unmapped memory. > The only way we could allocate unmapped memory is if the 512 MB /1 GB sized/aligned intersection with the memblock covering the kernel is completely reserved, either by the kernel or by other reservations. Since UEFI allocates from the top as well, if the kernel is loaded high and may end up such that there is little room between the start of the kernel and the beginning of the intersection. Still quite unlikely imo since it would mean that UEFI using hundreds of megabytes of memory, and it isn't quite /that/ bad [yet :-)] > Can we not have another set of level 2,3(,4) page tables pre-allocated > in swapper for the first block (start of RAM)? It gets hairy, in total > we would need: > > 1) idmap > 2) swapper > 2.a) kernel image outside the linear mapping > 2.b) fixmap > 2.c) start-of-ram > 2.d) swapper mapping in the linear mapping > > Can we avoid accessing 2.d (swapper in linear mapping) until we finished > mapping 2.c? Once we mapped the start of RAM and set the memblock limit, > we can allocate pages to start mapping the rest. > I really don't think any of this is necessary tbh
On 8 May 2015 at 18:43, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Fri, May 08, 2015 at 05:03:37PM +0200, Ard Biesheuvel wrote: >> On 8 May 2015 at 16:44, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Thu, May 07, 2015 at 09:21:28PM +0200, Ard Biesheuvel wrote: >> >> On 7 May 2015 at 18:54, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> > On Wed, Apr 15, 2015 at 05:34:20PM +0200, Ard Biesheuvel wrote: >> >> >> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S >> >> >> index ceec4def354b..338eaa7bcbfd 100644 >> >> >> --- a/arch/arm64/kernel/vmlinux.lds.S >> >> >> +++ b/arch/arm64/kernel/vmlinux.lds.S >> >> >> @@ -68,6 +68,17 @@ PECOFF_FILE_ALIGNMENT = 0x200; >> >> >> #define ALIGN_DEBUG_RO_MIN(min) . = ALIGN(min); >> >> >> #endif >> >> >> >> >> >> +/* >> >> >> + * The pgdir region needs to be mappable using a single PMD or PUD sized region, >> >> >> + * so it should not cross a 512 MB or 1 GB alignment boundary, respectively >> >> >> + * (depending on page size). So align to an upper bound of its size. >> >> >> + */ >> >> >> +#if CONFIG_ARM64_PGTABLE_LEVELS == 2 >> >> >> +#define PGDIR_ALIGN (8 * PAGE_SIZE) >> >> >> +#else >> >> >> +#define PGDIR_ALIGN (16 * PAGE_SIZE) >> >> >> +#endif >> >> > >> >> > Isn't 8 pages sufficient in both cases? Unless some other patch changes >> >> > the idmap and swapper, I can count maximum 7 pages in total. >> >> >> >> The preceding patch moves the fixmap page tables to this region as well. >> >> But the logic is still incorrect -> we only need 16x for 4 levels (7 + >> >> 3 == 10), the remaining ones are all <= 8 >> > >> > You should improve the comment here to include the maths, "upper bound >> > of its size" is not very clear ;). >> >> Yes, you are right, it should read 'power-of-2 upper bound' > > And the number of pages required for the initial page tables (I figured > out it's a power of two already ;)). > Ok >> >> >> + static struct bootstrap_pgtables linear_bs_pgtables __pgdir; >> >> >> + const phys_addr_t swapper_phys = __pa(swapper_pg_dir); >> >> >> + unsigned long swapper_virt = __phys_to_virt(swapper_phys) + va_offset; >> >> >> + struct memblock_region *reg; >> >> >> + >> >> >> + bootstrap_early_mapping(swapper_virt, &linear_bs_pgtables, >> >> >> + IS_ENABLED(CONFIG_ARM64_64K_PAGES)); >> >> >> + >> >> >> + /* now find the memblock that covers swapper_pg_dir, and clip */ >> >> >> + for_each_memblock(memory, reg) { >> >> >> + phys_addr_t start = reg->base; >> >> >> + phys_addr_t end = start + reg->size; >> >> >> + unsigned long vstart, vend; >> >> >> + >> >> >> + if (start > swapper_phys || end <= swapper_phys) >> >> >> + continue; >> >> >> + >> >> >> +#ifdef CONFIG_ARM64_64K_PAGES >> >> >> + /* clip the region to PMD size */ >> >> >> + vstart = max(swapper_virt & PMD_MASK, >> >> >> + round_up(__phys_to_virt(start + va_offset), >> >> >> + PAGE_SIZE)); >> >> >> + vend = min(round_up(swapper_virt, PMD_SIZE), >> >> >> + round_down(__phys_to_virt(end + va_offset), >> >> >> + PAGE_SIZE)); >> >> >> +#else >> >> >> + /* clip the region to PUD size */ >> >> >> + vstart = max(swapper_virt & PUD_MASK, >> >> >> + round_up(__phys_to_virt(start + va_offset), >> >> >> + PMD_SIZE)); >> >> >> + vend = min(round_up(swapper_virt, PUD_SIZE), >> >> >> + round_down(__phys_to_virt(end + va_offset), >> >> >> + PMD_SIZE)); >> >> >> +#endif >> >> >> + >> >> >> + create_mapping(__pa(vstart - va_offset), vstart, vend - vstart, >> >> >> + PAGE_KERNEL_EXEC); >> >> >> + >> >> >> + /* >> >> >> + * Temporarily limit the memblock range. We need to do this as >> >> >> + * create_mapping requires puds, pmds and ptes to be allocated >> >> >> + * from memory addressable from the early linear mapping. >> >> >> + */ >> >> >> + memblock_set_current_limit(__pa(vend - va_offset)); >> >> >> + >> >> >> + return; >> >> >> + } >> >> >> + BUG(); >> >> >> +} >> >> > >> >> > I'll probably revisit this function after I see the whole series. But in >> >> > the meantime, if the kernel is not loaded in the first memblock (in >> >> > address order), isn't there a risk that we allocate memory from the >> >> > first memblock which is not mapped yet? >> >> >> >> memblock allocates top down, so it should only allocate from this >> >> region, unless the remaining room is completely reserved. >> > >> > I don't like to rely on this, it's not guaranteed behaviour. >> >> Actually, it is. Allocation is always top-down unless you call >> memblock_set_bottom_up(), which is a NOP unless CONFIG_MOVABLE_NODE >> is selected. >> That is why the memblock limit only limits at the top afaict > > It currently works like this but I'm not sure it is guaranteed to always > behave this way (e.g. someone "improves" the memblock allocator in the > future). And you never know, we may need memory hotplug on arm64 at some > point in the future (together with CONFIG_MOVABLE_NODE). > I am not it is worth the additional hassle now to take into consideration what someone may or may not implement at some point in the future. I am sure memory hotplug is going to take more effort than just setting the Kconfig option, and I would not be surprised if it imposed additional restrictions on the placement on the kernel. So the question is really if allowing the kernel to be placed at arbitrary offsets in physical memory is worth the hassle in the first place. There is also a rather nasty interaction with the mem= command line option (and that does not work 100% correctly in this version of the series). There is a policy decision to be made there, i.e., if you remove memory to uphold mem=, where do you remove it? Removing from the low end may waste precious <4 GB memory but removing from the top may be impossible if the kernel image is loaded there. Perhaps we should consider adding an early early memblock allocator that allocates statically from the __pgdir region. The only problem is finding a reasonable upper bound for the amount of memory you would need ...
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S index ceec4def354b..338eaa7bcbfd 100644 --- a/arch/arm64/kernel/vmlinux.lds.S +++ b/arch/arm64/kernel/vmlinux.lds.S @@ -68,6 +68,17 @@ PECOFF_FILE_ALIGNMENT = 0x200; #define ALIGN_DEBUG_RO_MIN(min) . = ALIGN(min); #endif +/* + * The pgdir region needs to be mappable using a single PMD or PUD sized region, + * so it should not cross a 512 MB or 1 GB alignment boundary, respectively + * (depending on page size). So align to an upper bound of its size. + */ +#if CONFIG_ARM64_PGTABLE_LEVELS == 2 +#define PGDIR_ALIGN (8 * PAGE_SIZE) +#else +#define PGDIR_ALIGN (16 * PAGE_SIZE) +#endif + SECTIONS { /* @@ -160,7 +171,7 @@ SECTIONS BSS_SECTION(0, 0, 0) - .pgdir (NOLOAD) : ALIGN(PAGE_SIZE) { + .pgdir (NOLOAD) : ALIGN(PGDIR_ALIGN) { idmap_pg_dir = .; . += IDMAP_DIR_SIZE; swapper_pg_dir = .; @@ -185,6 +196,11 @@ ASSERT(__idmap_text_end - (__idmap_text_start & ~(SZ_4K - 1)) <= SZ_4K, "ID map text too big or misaligned") /* + * Check that the chosen PGDIR_ALIGN value if sufficient. + */ +ASSERT(SIZEOF(.pgdir) < ALIGNOF(.pgdir), ".pgdir size exceeds its alignment") + +/* * If padding is applied before .head.text, virt<->phys conversions will fail. */ ASSERT(_text == (PAGE_OFFSET + TEXT_OFFSET), "HEAD is misaligned") diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index c27ab20a5ba9..93e5a2497f01 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -380,26 +380,68 @@ static void __init bootstrap_early_mapping(unsigned long addr, } } +static void __init bootstrap_linear_mapping(unsigned long va_offset) +{ + /* + * Bootstrap the linear range that covers swapper_pg_dir so that the + * statically allocated page tables as well as newly allocated ones + * are accessible via the linear mapping. + */ + static struct bootstrap_pgtables linear_bs_pgtables __pgdir; + const phys_addr_t swapper_phys = __pa(swapper_pg_dir); + unsigned long swapper_virt = __phys_to_virt(swapper_phys) + va_offset; + struct memblock_region *reg; + + bootstrap_early_mapping(swapper_virt, &linear_bs_pgtables, + IS_ENABLED(CONFIG_ARM64_64K_PAGES)); + + /* now find the memblock that covers swapper_pg_dir, and clip */ + for_each_memblock(memory, reg) { + phys_addr_t start = reg->base; + phys_addr_t end = start + reg->size; + unsigned long vstart, vend; + + if (start > swapper_phys || end <= swapper_phys) + continue; + +#ifdef CONFIG_ARM64_64K_PAGES + /* clip the region to PMD size */ + vstart = max(swapper_virt & PMD_MASK, + round_up(__phys_to_virt(start + va_offset), + PAGE_SIZE)); + vend = min(round_up(swapper_virt, PMD_SIZE), + round_down(__phys_to_virt(end + va_offset), + PAGE_SIZE)); +#else + /* clip the region to PUD size */ + vstart = max(swapper_virt & PUD_MASK, + round_up(__phys_to_virt(start + va_offset), + PMD_SIZE)); + vend = min(round_up(swapper_virt, PUD_SIZE), + round_down(__phys_to_virt(end + va_offset), + PMD_SIZE)); +#endif + + create_mapping(__pa(vstart - va_offset), vstart, vend - vstart, + PAGE_KERNEL_EXEC); + + /* + * Temporarily limit the memblock range. We need to do this as + * create_mapping requires puds, pmds and ptes to be allocated + * from memory addressable from the early linear mapping. + */ + memblock_set_current_limit(__pa(vend - va_offset)); + + return; + } + BUG(); +} + static void __init map_mem(void) { struct memblock_region *reg; - phys_addr_t limit; - /* - * Temporarily limit the memblock range. We need to do this as - * create_mapping requires puds, pmds and ptes to be allocated from - * memory addressable from the initial direct kernel mapping. - * - * The initial direct kernel mapping, located at swapper_pg_dir, gives - * us PUD_SIZE (4K pages) or PMD_SIZE (64K pages) memory starting from - * PHYS_OFFSET (which must be aligned to 2MB as per - * Documentation/arm64/booting.txt). - */ - if (IS_ENABLED(CONFIG_ARM64_64K_PAGES)) - limit = PHYS_OFFSET + PMD_SIZE; - else - limit = PHYS_OFFSET + PUD_SIZE; - memblock_set_current_limit(limit); + bootstrap_linear_mapping(0); /* map all the memory banks */ for_each_memblock(memory, reg) { @@ -409,21 +451,6 @@ static void __init map_mem(void) if (start >= end) break; -#ifndef CONFIG_ARM64_64K_PAGES - /* - * For the first memory bank align the start address and - * current memblock limit to prevent create_mapping() from - * allocating pte page tables from unmapped memory. - * When 64K pages are enabled, the pte page table for the - * first PGDIR_SIZE is already present in swapper_pg_dir. - */ - if (start < limit) - start = ALIGN(start, PMD_SIZE); - if (end < limit) { - limit = end & PMD_MASK; - memblock_set_current_limit(limit); - } -#endif __map_memblock(start, end); }
In preparation of moving the kernel text out of the linear mapping, ensure that the part of the kernel Image that contains the statically allocated page tables is made accessible via the linear mapping before performing the actual mapping of all of memory. This is needed by the normal mapping routines, that rely on the linear mapping to walk the page tables while manipulating them. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/vmlinux.lds.S | 18 ++++++++- arch/arm64/mm/mmu.c | 89 +++++++++++++++++++++++++++-------------- 2 files changed, 75 insertions(+), 32 deletions(-)