| Message ID | E1YfmXq-0006Wr-M7@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On 04/08/2015 12:45 PM, Russell King wrote: > Make the init_meminfo function return the offset to be applied to the > phys-to-virt translation constants. This allows us to move the update > into generic code, along with the requirements for this update. I have a question (or may be proposition) regarding this patch. Could it be reasonable if .init_meminfo() will return new PHYS offset of RAM (new start address), instead of translation's offset? Additional comments are below, which were done only from Keystone 2 point of view. > > This avoids platforms having to know the details of the phys-to-virt > translation support. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > arch/arm/include/asm/mach/arch.h | 2 +- > arch/arm/mach-keystone/keystone.c | 27 ++++++++++----------------- > arch/arm/mm/mmu.c | 26 ++++++++++++++++++++++---- > 3 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h > index 0406cb3f1af7..e881913f7c3e 100644 > --- a/arch/arm/include/asm/mach/arch.h > +++ b/arch/arm/include/asm/mach/arch.h > @@ -51,7 +51,7 @@ struct machine_desc { > bool (*smp_init)(void); > void (*fixup)(struct tag *, char **); > void (*dt_fixup)(void); > - void (*init_meminfo)(void); > + long long (*init_meminfo)(void); > void (*reserve)(void);/* reserve mem blocks */ > void (*map_io)(void);/* IO mapping function */ > void (*init_early)(void); > diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c > index 3d58a8f4dc7e..baa0fbc9803a 100644 > --- a/arch/arm/mach-keystone/keystone.c > +++ b/arch/arm/mach-keystone/keystone.c > @@ -68,11 +68,9 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x) > return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START; > } > > -static void __init keystone_init_meminfo(void) > +static long long __init keystone_init_meminfo(void) > { > - bool lpae = IS_ENABLED(CONFIG_ARM_LPAE); > - bool pvpatch = IS_ENABLED(CONFIG_ARM_PATCH_PHYS_VIRT); > - phys_addr_t offset = PHYS_OFFSET - KEYSTONE_LOW_PHYS_START; > + long long offset; > phys_addr_t mem_start, mem_end; > > mem_start = memblock_start_of_DRAM(); > @@ -81,29 +79,24 @@ static void __init keystone_init_meminfo(void) > /* nothing to do if we are running out of the <32-bit space */ > if (mem_start >= KEYSTONE_LOW_PHYS_START && > mem_end <= KEYSTONE_LOW_PHYS_END) > - return; > - > - if (!lpae || !pvpatch) { > - pr_crit("Enable %s%s%s to run outside 32-bit space\n", > - !lpae ? __stringify(CONFIG_ARM_LPAE) : "", > - (!lpae && !pvpatch) ? " and " : "", > - !pvpatch ? __stringify(CONFIG_ARM_PATCH_PHYS_VIRT) : ""); > - } > + return 0; > > if (mem_start < KEYSTONE_HIGH_PHYS_START || > mem_end > KEYSTONE_HIGH_PHYS_END) { > pr_crit("Invalid address space for memory (%08llx-%08llx)\n", > - (u64)mem_start, (u64)mem_end); > + (u64)mem_start, (u64)mem_end); > + return 0; > } > > - offset += KEYSTONE_HIGH_PHYS_START; > - __pv_phys_pfn_offset = PFN_DOWN(offset); > - __pv_offset = (offset - PAGE_OFFSET); > + offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START; New PHYS offset of RAM can be returned here offset = KEYSTONE_HIGH_PHYS_START; > > /* Populate the arch idmap hook */ > arch_virt_to_idmap = keystone_virt_to_idmap; > > - pr_info("Switching to high address space at 0x%llx\n", (u64)offset); > + pr_info("Switching to high address space at 0x%llx\n", > + (u64)PHYS_OFFSET + (u64)offset); Change can be dropped > + > + return offset; > } > > static const char *const keystone_match[] __initconst = { > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > index 4e6ef896c619..71563fdf298c 100644 > --- a/arch/arm/mm/mmu.c > +++ b/arch/arm/mm/mmu.c > @@ -1387,7 +1387,7 @@ static void __init map_lowmem(void) > } > } > > -#ifdef CONFIG_ARM_LPAE > +#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_ARM_PATCH_PHYS_VIRT) > /* > * early_paging_init() recreates boot time page table setup, allowing machines > * to switch over to a high (>4G) address space on LPAE systems > @@ -1397,6 +1397,7 @@ void __init early_paging_init(const struct machine_desc *mdesc, > { > pmdval_t pmdprot = procinfo->__cpu_mm_mmu_flags; > unsigned long map_start, map_end; > + long long offset; > pgd_t *pgd0, *pgdk; > pud_t *pud0, *pudk, *pud_start; > pmd_t *pmd0, *pmdk; > @@ -1419,7 +1420,13 @@ void __init early_paging_init(const struct machine_desc *mdesc, > pudk = pud_offset(pgdk, map_start); > pmdk = pmd_offset(pudk, map_start); > If I understand PV patching code right, the PHYS_OFFSET == 0x8000 0000 (32 bit address) at this moment and its value is adjusted by Asm code. So, this value could be considered as old PHYS_OFFSET. PAGE_OFFSET == 0xC000 0000 (defconfig) > - mdesc->init_meminfo(); > + offset = mdesc->init_meminfo(); > + if (offset == 0) > + return; Here now: offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0x8000 0000 (KEYSTONE_LOW_PHYS_START) offset == 0x7 8000 0000 in turn KEYSTONE_LOW_PHYS_START == (old)PHYS_OFFSET > + > + /* Re-set the phys pfn offset, and the pv offset */ > + __pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET; So, here now: __pv_offset = 0x8000 0000 + 0x7 8000 0000 - 0xC000 0000; __pv_offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0xC000 0000 code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START: __pv_offset = offset - PAGE_OFFSET; > + __pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset); Here (old)PHYS_OFFSET + offset == 0x8000 0000 + 0x7 8000 0000 == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) Code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START: __pv_phys_pfn_offset = PFN_DOWN(offset); > > /* Run the patch stub to update the constants */ > fixup_pv_table(&__pv_table_begin, > @@ -1502,8 +1509,19 @@ void __init early_paging_init(const struct machine_desc *mdesc, > void __init early_paging_init(const struct machine_desc *mdesc, > struct proc_info_list *procinfo) > { > - if (mdesc->init_meminfo) > - mdesc->init_meminfo(); > + long long offset; > + > + if (!mdesc->init_meminfo) > + return; > + > + offset = mdesc->init_meminfo(); > + if (offset == 0) > + return; > + > + pr_crit("Physical address space modification is only to support Keystone2.\n"); > + pr_crit("Please enable ARM_LPAE and ARM_PATCH_PHYS_VIRT support to use this\n"); > + pr_crit("feature. Your kernel may crash now, have a good day.\n"); > + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); > } > > #endif >
On Wed, Apr 08, 2015 at 05:56:49PM +0300, Grygorii.Strashko@linaro.org wrote: > Hi Russell, > > On 04/08/2015 12:45 PM, Russell King wrote: > > Make the init_meminfo function return the offset to be applied to the > > phys-to-virt translation constants. This allows us to move the update > > into generic code, along with the requirements for this update. > > > I have a question (or may be proposition) regarding this patch. > > Could it be reasonable if .init_meminfo() will return new PHYS offset > of RAM (new start address), instead of translation's offset? I'm not that interested in such a change, for the simple reason that what we mostly want to know is what the _difference_ between what's already in the page tables, and what we want to update them to. The page table update is a case of merely adding the delta to each page table entry. If we were to want to pass the actual new physical address, we would have to have the fixup code mask out the old address, and insert the new address, keeping track of the offset into the kernel's mapping. We'd also have to do something weirder for the DT mapping too. Using an offset makes the page table update rather trivial and easy. I actually did start off with passing the new physical address initially, and I changed to this way because it simplified the assembly code.
On Wed, Apr 08, 2015 at 05:56:49PM +0300, Grygorii.Strashko@linaro.org wrote: > > + > > + /* Re-set the phys pfn offset, and the pv offset */ > > + __pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET; > > So, here now: > > __pv_offset = 0x8000 0000 + 0x7 8000 0000 - 0xC000 0000; > > __pv_offset == 0x8 0000 0000 (KEYSTONE_HIGH_PHYS_START) - 0xC000 0000 > > code if offset will eq (new)PHYS_OFFSET == KEYSTONE_HIGH_PHYS_START: > > __pv_offset = offset - PAGE_OFFSET; BTW, this could simply be changed to: __pv_offset += offset; keeping the existing "delta" implementation. > > + __pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset); and this to: __pv_phys_pfn_offset += PFN_DOWN(offset);
On 4/8/2015 2:45 AM, Russell King wrote: > Make the init_meminfo function return the offset to be applied to the > phys-to-virt translation constants. This allows us to move the update > into generic code, along with the requirements for this update. > > This avoids platforms having to know the details of the phys-to-virt > translation support. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > arch/arm/include/asm/mach/arch.h | 2 +- > arch/arm/mach-keystone/keystone.c | 27 ++++++++++----------------- > arch/arm/mm/mmu.c | 26 ++++++++++++++++++++++---- > 3 files changed, 33 insertions(+), 22 deletions(-) > You already identified couple of improvements on offset calculations. Apart from that, patch look fine by me. Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h index 0406cb3f1af7..e881913f7c3e 100644 --- a/arch/arm/include/asm/mach/arch.h +++ b/arch/arm/include/asm/mach/arch.h @@ -51,7 +51,7 @@ struct machine_desc { bool (*smp_init)(void); void (*fixup)(struct tag *, char **); void (*dt_fixup)(void); - void (*init_meminfo)(void); + long long (*init_meminfo)(void); void (*reserve)(void);/* reserve mem blocks */ void (*map_io)(void);/* IO mapping function */ void (*init_early)(void); diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c index 3d58a8f4dc7e..baa0fbc9803a 100644 --- a/arch/arm/mach-keystone/keystone.c +++ b/arch/arm/mach-keystone/keystone.c @@ -68,11 +68,9 @@ static phys_addr_t keystone_virt_to_idmap(unsigned long x) return (phys_addr_t)(x) - CONFIG_PAGE_OFFSET + KEYSTONE_LOW_PHYS_START; } -static void __init keystone_init_meminfo(void) +static long long __init keystone_init_meminfo(void) { - bool lpae = IS_ENABLED(CONFIG_ARM_LPAE); - bool pvpatch = IS_ENABLED(CONFIG_ARM_PATCH_PHYS_VIRT); - phys_addr_t offset = PHYS_OFFSET - KEYSTONE_LOW_PHYS_START; + long long offset; phys_addr_t mem_start, mem_end; mem_start = memblock_start_of_DRAM(); @@ -81,29 +79,24 @@ static void __init keystone_init_meminfo(void) /* nothing to do if we are running out of the <32-bit space */ if (mem_start >= KEYSTONE_LOW_PHYS_START && mem_end <= KEYSTONE_LOW_PHYS_END) - return; - - if (!lpae || !pvpatch) { - pr_crit("Enable %s%s%s to run outside 32-bit space\n", - !lpae ? __stringify(CONFIG_ARM_LPAE) : "", - (!lpae && !pvpatch) ? " and " : "", - !pvpatch ? __stringify(CONFIG_ARM_PATCH_PHYS_VIRT) : ""); - } + return 0; if (mem_start < KEYSTONE_HIGH_PHYS_START || mem_end > KEYSTONE_HIGH_PHYS_END) { pr_crit("Invalid address space for memory (%08llx-%08llx)\n", - (u64)mem_start, (u64)mem_end); + (u64)mem_start, (u64)mem_end); + return 0; } - offset += KEYSTONE_HIGH_PHYS_START; - __pv_phys_pfn_offset = PFN_DOWN(offset); - __pv_offset = (offset - PAGE_OFFSET); + offset = KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START; /* Populate the arch idmap hook */ arch_virt_to_idmap = keystone_virt_to_idmap; - pr_info("Switching to high address space at 0x%llx\n", (u64)offset); + pr_info("Switching to high address space at 0x%llx\n", + (u64)PHYS_OFFSET + (u64)offset); + + return offset; } static const char *const keystone_match[] __initconst = { diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 4e6ef896c619..71563fdf298c 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -1387,7 +1387,7 @@ static void __init map_lowmem(void) } } -#ifdef CONFIG_ARM_LPAE +#if defined(CONFIG_ARM_LPAE) && defined(CONFIG_ARM_PATCH_PHYS_VIRT) /* * early_paging_init() recreates boot time page table setup, allowing machines * to switch over to a high (>4G) address space on LPAE systems @@ -1397,6 +1397,7 @@ void __init early_paging_init(const struct machine_desc *mdesc, { pmdval_t pmdprot = procinfo->__cpu_mm_mmu_flags; unsigned long map_start, map_end; + long long offset; pgd_t *pgd0, *pgdk; pud_t *pud0, *pudk, *pud_start; pmd_t *pmd0, *pmdk; @@ -1419,7 +1420,13 @@ void __init early_paging_init(const struct machine_desc *mdesc, pudk = pud_offset(pgdk, map_start); pmdk = pmd_offset(pudk, map_start); - mdesc->init_meminfo(); + offset = mdesc->init_meminfo(); + if (offset == 0) + return; + + /* Re-set the phys pfn offset, and the pv offset */ + __pv_offset = PHYS_OFFSET + offset - PAGE_OFFSET; + __pv_phys_pfn_offset = PFN_DOWN(PHYS_OFFSET + offset); /* Run the patch stub to update the constants */ fixup_pv_table(&__pv_table_begin, @@ -1502,8 +1509,19 @@ void __init early_paging_init(const struct machine_desc *mdesc, void __init early_paging_init(const struct machine_desc *mdesc, struct proc_info_list *procinfo) { - if (mdesc->init_meminfo) - mdesc->init_meminfo(); + long long offset; + + if (!mdesc->init_meminfo) + return; + + offset = mdesc->init_meminfo(); + if (offset == 0) + return; + + pr_crit("Physical address space modification is only to support Keystone2.\n"); + pr_crit("Please enable ARM_LPAE and ARM_PATCH_PHYS_VIRT support to use this\n"); + pr_crit("feature. Your kernel may crash now, have a good day.\n"); + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); } #endif
Make the init_meminfo function return the offset to be applied to the phys-to-virt translation constants. This allows us to move the update into generic code, along with the requirements for this update. This avoids platforms having to know the details of the phys-to-virt translation support. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- arch/arm/include/asm/mach/arch.h | 2 +- arch/arm/mach-keystone/keystone.c | 27 ++++++++++----------------- arch/arm/mm/mmu.c | 26 ++++++++++++++++++++++---- 3 files changed, 33 insertions(+), 22 deletions(-)