This series is a follow on to the RFC series posted earlier (archived at [1]).
The major change introduced here is the modification to the kernel patching
mechanism for phys_to_virt/virt_to_phys, in order to support LPAE platforms
that require late patching. In addition to these changes, we've updated the
series based on feedback from the earlier posting.
Most of the patches in this series are fixes and extensions to LPAE support on
ARM. The last three patches in this series are specific to the TI Keystone
platform, and are being provided here for the sake of completeness. These
three patches are dependent on the smpops patch set (see [2]), and are not
ready to be merged in as yet.
[1] - https://lkml.org/lkml/2012/7/23/460
[2] - http://permalink.gmane.org/gmane.linux.ports.arm.kernel/171540
Cyril Chemparathy (18):
ARM: add mechanism for late code patching
ARM: use late patch framework for phys-virt patching
ARM: LPAE: use phys_addr_t on virt <--> phys conversion
ARM: LPAE: support 64-bit virt/phys patching
ARM: LPAE: use signed arithmetic for mask definitions
ARM: LPAE: use 64-bit pgd physical address in switch_mm()
ARM: LPAE: use 64-bit accessors for TTBR registers
ARM: LPAE: define ARCH_LOW_ADDRESS_LIMIT for bootmem
ARM: LPAE: factor out T1SZ and TTBR1 computations
ARM: LPAE: allow proc override of TTB setup
ARM: LPAE: accomodate >32-bit addresses for page table base
ARM: mm: use physical addresses in highmem sanity checks
ARM: mm: cleanup checks for membank overlap with vmalloc area
ARM: mm: clean up membank size limit checks
ARM: recreate kernel mappings in early_paging_init()
ARM: keystone: introducing TI Keystone platform
ARM: keystone: enable SMP on Keystone machines
ARM: keystone: add switch over to high physical address range
Vitaly Andrianov (4):
ARM: LPAE: use phys_addr_t in alloc_init_pud()
ARM: LPAE: use phys_addr_t in free_memmap()
ARM: LPAE: use phys_addr_t for initrd location and size
ARM: add virt_to_idmap for interconnect aliasing
arch/arm/Kconfig | 20 +++
arch/arm/Makefile | 1 +
arch/arm/boot/dts/keystone-sim.dts | 77 +++++++++
arch/arm/configs/keystone_defconfig | 23 +++
arch/arm/include/asm/cache.h | 9 +
arch/arm/include/asm/mach/arch.h | 1 +
arch/arm/include/asm/memory.h | 68 +++++---
arch/arm/include/asm/page.h | 2 +-
arch/arm/include/asm/patch.h | 123 +++++++++++++
arch/arm/include/asm/pgtable-3level-hwdef.h | 10 ++
arch/arm/include/asm/pgtable-3level.h | 6 +-
arch/arm/include/asm/proc-fns.h | 28 ++-
arch/arm/kernel/head.S | 119 +++----------
arch/arm/kernel/module.c | 7 +-
arch/arm/kernel/setup.c | 192 +++++++++++++++++++++
arch/arm/kernel/smp.c | 11 +-
arch/arm/kernel/vmlinux.lds.S | 13 +-
arch/arm/mach-keystone/Makefile | 2 +
arch/arm/mach-keystone/Makefile.boot | 1 +
arch/arm/mach-keystone/include/mach/debug-macro.S | 44 +++++
arch/arm/mach-keystone/include/mach/memory.h | 47 +++++
arch/arm/mach-keystone/include/mach/timex.h | 21 +++
arch/arm/mach-keystone/include/mach/uncompress.h | 24 +++
arch/arm/mach-keystone/keystone.c | 122 +++++++++++++
arch/arm/mach-keystone/keystone.h | 23 +++
arch/arm/mach-keystone/platsmp.c | 88 ++++++++++
arch/arm/mm/context.c | 13 +-
arch/arm/mm/idmap.c | 4 +-
arch/arm/mm/init.c | 20 +--
arch/arm/mm/mmu.c | 106 ++++++++----
arch/arm/mm/proc-arm1026.S | 3 +
arch/arm/mm/proc-mohawk.S | 3 +
arch/arm/mm/proc-v6.S | 6 +-
arch/arm/mm/proc-v7-2level.S | 7 +-
arch/arm/mm/proc-v7-3level.S | 29 ++--
arch/arm/mm/proc-v7.S | 2 +
arch/arm/mm/proc-xsc3.S | 3 +
37 files changed, 1065 insertions(+), 213 deletions(-)
create mode 100644 arch/arm/boot/dts/keystone-sim.dts
create mode 100644 arch/arm/configs/keystone_defconfig
create mode 100644 arch/arm/include/asm/patch.h
create mode 100644 arch/arm/mach-keystone/Makefile
create mode 100644 arch/arm/mach-keystone/Makefile.boot
create mode 100644 arch/arm/mach-keystone/include/mach/debug-macro.S
create mode 100644 arch/arm/mach-keystone/include/mach/memory.h
create mode 100644 arch/arm/mach-keystone/include/mach/timex.h
create mode 100644 arch/arm/mach-keystone/include/mach/uncompress.h
create mode 100644 arch/arm/mach-keystone/keystone.c
create mode 100644 arch/arm/mach-keystone/keystone.h
create mode 100644 arch/arm/mach-keystone/platsmp.c
--
1.7.9.5
This patch applies to PAGE_MASK, PMD_MASK, and PGDIR_MASK, where forcing
unsigned long math truncates the mask at the 32-bits. This clearly does bad
things on PAE systems.
This patch fixes this problem by defining these masks as signed quantities.
We then rely on sign extension to do the right thing.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/page.h | 2 +-
arch/arm/include/asm/pgtable-3level.h | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -0,0 +1,74 @@+/*+*Copyright2012TexasInstruments,Inc.+*+*Basedonplatsmp.c,Copyright2010-2011Calxeda,Inc.+*Basedonplatsmp.c,Copyright(C)2002ARMLtd.+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodifyit+*underthetermsandconditionsoftheGNUGeneralPublicLicense,+*version2,aspublishedbytheFreeSoftwareFoundation.+*+*Thisprogramisdistributedinthehopeitwillbeuseful,butWITHOUT+*ANYWARRANTY;withouteventheimpliedwarrantyofMERCHANTABILITYor+*FITNESSFORAPARTICULARPURPOSE.SeetheGNUGeneralPublicLicensefor+*moredetails.+*+*YoushouldhavereceivedacopyoftheGNUGeneralPublicLicensealongwith+*thisprogram.Ifnot,see<http://www.gnu.org/licenses/>.+*/+#include<linux/init.h>+#include<linux/smp.h>+#include<linux/io.h>++#include<asm/smp_plat.h>+#include<asm/smp_ops.h>+#include<asm/hardware/gic.h>+#include<asm/cacheflush.h>+#include<asm/memory.h>++#include"keystone.h"++staticvoid__initkeystone_smp_init_cpus(void)+{+unsignedinti,ncores;++ncores=4;++/* sanity check */+if(ncores>NR_CPUS){+pr_warn("restricted to %d cpus\n",NR_CPUS);+ncores=NR_CPUS;+}++for(i=0;i<ncores;i++)+set_cpu_possible(i,true);++set_smp_cross_call(gic_raise_softirq);+}++staticvoid__initkeystone_smp_prepare_cpus(unsignedintmax_cpus)+{+/* nothing for now */+}++staticvoid__cpuinitkeystone_secondary_init(unsignedintcpu)+{+gic_secondary_init(0);+}++staticint__cpuinit+keystone_boot_secondary(unsignedintcpu,structtask_struct*idle)+{+unsignedlong*ptr;++ptr=phys_to_virt(0x800001f0);+ptr[cpu]=virt_to_idmap(&secondary_startup);+__cpuc_flush_dcache_area(ptr,sizeof(ptr)*4);++return0;+}++structsmp_opskeystone_smp_ops__initdata={+smp_init_ops(keystone)+smp_secondary_ops(keystone)+};
This patch modifies the switch_mm() processor functions to use 64-bit
addresses. We use u64 instead of phys_addr_t, in order to avoid having config
dependent register usage when calling into switch_mm assembly code.
The changes in this patch are primarily adjustments for registers used for
arguments to switch_mm. The few processor definitions that did use the second
argument have been modified accordingly.
Arguments and calling conventions aside, this patch should be a no-op on v6
and non-LPAE v7 processors. On LPAE systems, we now honor the upper 32-bits
of the physical address that is being passed in.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/proc-fns.h | 4 ++--
arch/arm/mm/proc-v6.S | 2 +-
arch/arm/mm/proc-v7-2level.S | 2 +-
arch/arm/mm/proc-v7-3level.S | 5 +++--
4 files changed, 7 insertions(+), 6 deletions(-)
From: Nicolas Pitre <hidden> Date: 2012-08-04 07:05:00
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
This patch modifies the switch_mm() processor functions to use 64-bit
addresses. We use u64 instead of phys_addr_t, in order to avoid having config
dependent register usage when calling into switch_mm assembly code.
The changes in this patch are primarily adjustments for registers used for
arguments to switch_mm. The few processor definitions that did use the second
argument have been modified accordingly.
Arguments and calling conventions aside, this patch should be a no-op on v6
and non-LPAE v7 processors.
NAK.
You just broke all big endian targets, LPAE or not.
Nicolas
This patch modifies the switch_mm() processor functions to use 64-bit
addresses. We use u64 instead of phys_addr_t, in order to avoid having config
dependent register usage when calling into switch_mm assembly code.
The changes in this patch are primarily adjustments for registers used for
arguments to switch_mm. The few processor definitions that did use the second
argument have been modified accordingly.
Arguments and calling conventions aside, this patch should be a no-op on v6
and non-LPAE v7 processors.
NAK.
You just broke all big endian targets, LPAE or not.
Indeed. Thanks.
Would C-land word swappery on BE do? Any other ideas on the best
approach to this?
From: Nicolas Pitre <hidden> Date: 2012-08-06 02:35:38
On Sun, 5 Aug 2012, Cyril Chemparathy wrote:
On 8/4/2012 3:04 AM, Nicolas Pitre wrote:
quoted
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
quoted
This patch modifies the switch_mm() processor functions to use 64-bit
addresses. We use u64 instead of phys_addr_t, in order to avoid having
config
dependent register usage when calling into switch_mm assembly code.
The changes in this patch are primarily adjustments for registers used for
arguments to switch_mm. The few processor definitions that did use the
second
argument have been modified accordingly.
Arguments and calling conventions aside, this patch should be a no-op on
v6
and non-LPAE v7 processors.
NAK.
You just broke all big endian targets, LPAE or not.
Indeed. Thanks.
Would C-land word swappery on BE do? Any other ideas on the best approach to
this?
First, don't use a u64 unconditionally. A phys_addr_t is best for the
same arguments as before. Since this is equivalent to a u64 only when
LPAE is defined, you then only have to care about endian issues in
proc-v7-3level.S. And in there you can deal with the issue with
register aliases just as it is done in lib/div64.S.
Nicolas
This patch adds TTBR accessor macros, and modifies cpu_get_pgd() and
the LPAE version of cpu_set_reserved_ttbr0() to use these instead.
In the process, we also fix these functions to correctly handle cases
where the physical address lies beyond the 4G limit of 32-bit addressing.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/proc-fns.h | 24 +++++++++++++++++++-----
arch/arm/mm/context.c | 13 ++-----------
2 files changed, 21 insertions(+), 16 deletions(-)
This patch cleans up the highmem sanity check code by simplifying the range
checks with a pre-calculated size_limit. This patch should otherwise have no
functional impact on behavior.
This patch also removes a redundant (bank->start < vmalloc_limit) check, since
this is already covered by the !highmem condition.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/mm/mmu.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
This patch allows ARM processor setup functions (*_setup in proc-*.S) to
indicate that the page table has already been programmed. This is
done by setting r4 (page table pointer) to -1 before returning from the
processor setup handler.
This capability is particularly needed on LPAE systems, where the translation
table base needs to be programmed differently with 64-bit control
register operations.
Further, a few of the processors (arm1026, mohawk, xsc3) were programming the
TTB twice. This patch prevents the main head.S code from programming TTB the
second time on these machines.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/kernel/head.S | 10 +++++-----
arch/arm/mm/proc-arm1026.S | 1 +
arch/arm/mm/proc-mohawk.S | 1 +
arch/arm/mm/proc-v6.S | 2 ++
arch/arm/mm/proc-v7-2level.S | 3 ++-
arch/arm/mm/proc-v7-3level.S | 1 +
arch/arm/mm/proc-v7.S | 1 +
arch/arm/mm/proc-xsc3.S | 1 +
8 files changed, 14 insertions(+), 6 deletions(-)
@@ -390,6 +390,7 @@ __mohawk_setup:mcrp15,0,r0,c8,c7@invalidateI,DTLBsorrr4,r4,#0x18 @ cache the page table in L2mcrp15,0,r4,c2,c0,0@loadpagetablepointer+mvnr4,#0 @ do not set page table pointermovr0,#0 @ don't allow CP accessmcrp15,0,r0,c15,c1,0@writeCPaccessregister
@@ -210,7 +210,9 @@ __v6_setup:ALT_UP(orrr4,r4,#TTB_FLAGS_UP)ALT_SMP(orrr8,r8,#TTB_FLAGS_SMP)ALT_UP(orrr8,r8,#TTB_FLAGS_UP)+mcrp15,0,r4,c2,c0,0@loadTTB0mcrp15,0,r8,c2,c0,1@loadTTB1+mvnr4,#0 @ do not set page table pointer#endif /* CONFIG_MMU */adrr5,v6_crvalldmiar5,{r5,r6}
@@ -250,6 +250,7 @@ __v7_setup:#ifdef CONFIG_MMUmcrp15,0,r10,c8,c7,0@invalidateI+DTLBsv7_ttb_setupr10,r4,r8,r5@TTBCR,TTBRxsetup+mvnr4,#0 @ do not set page table pointerldrr5,=PRRR@PRRRldrr6,=NMRR@NMRRmcrp15,0,r5,c10,c2,0@writePRRR
@@ -455,6 +455,7 @@ __xsc3_setup:mcrp15,0,ip,c8,c7,0@invalidateIandDTLBsorrr4,r4,#0x18 @ cache the page table in L2mcrp15,0,r4,c2,c0,0@loadpagetablepointer+mvnr4,#0 @ do not set page table pointermovr0,#1 << 6 @ cp6 access for early sched_clockmcrp15,0,r0,c15,c1,0@writeCPaccessregister
This patch redefines the early boot time use of the R4 register to steal a few
low order bits (ARCH_PGD_SHIFT bits), allowing for up to 38-bit physical
addresses.
This is probably not the best means to the end, and a better alternative may
be to modify the head.S register allocations to fit in full register pairs for
pgdir and swapper_pg_dir. However, squeezing out these extra registers seemed
to be a far greater pain than squeezing out a few low order bits from the page
table addresses.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/cache.h | 9 +++++++++
arch/arm/kernel/head.S | 7 +++++--
arch/arm/kernel/smp.c | 11 +++++++++--
arch/arm/mm/proc-arm1026.S | 2 ++
arch/arm/mm/proc-mohawk.S | 2 ++
arch/arm/mm/proc-v6.S | 2 ++
arch/arm/mm/proc-v7-2level.S | 2 ++
arch/arm/mm/proc-v7-3level.S | 7 +++++++
arch/arm/mm/proc-v7.S | 1 +
arch/arm/mm/proc-xsc3.S | 2 ++
10 files changed, 41 insertions(+), 4 deletions(-)
@@ -323,6 +324,7 @@ __create_page_tables:#ifdef CONFIG_ARM_LPAEsubr4,r4,#0x1000 @ point to the PGD table#endif+movr4,r4,lsr#ARCH_PGD_SHIFTmovpc,lrENDPROC(__create_page_tables).ltorg
@@ -395,7 +397,7 @@ __secondary_data:*r0=cp#15 control register*r1=machineID*r2=atagsordtbpointer-*r4=pagetablepointer+*r4=pagetable (seeARCH_PGD_SHIFTinasm/cache.h)*r9=processorID*r13=*virtual*addresstojumptouponcompletion*/
@@ -403,6 +404,7 @@ __arm1026_setup:mcrp15,0,r0,c7,c10,4@drainwritebufferonv4#ifdef CONFIG_MMUmcrp15,0,r0,c8,c7@invalidateI,DTLBsonv4+movr4,r4,lsl#ARCH_PGD_SHIFTmcrp15,0,r4,c2,c0@loadpagetablepointermvnr4,#0 @ do not set page table pointer#endif
@@ -388,6 +389,7 @@ __mohawk_setup:mcrp15,0,r0,c7,c7@invalidateI,Dcachesmcrp15,0,r0,c7,c10,4@drainwritebuffermcrp15,0,r0,c8,c7@invalidateI,DTLBs+movr4,r4,lsl#ARCH_PGD_SHIFTorrr4,r4,#0x18 @ cache the page table in L2mcrp15,0,r4,c2,c0,0@loadpagetablepointermvnr4,#0 @ do not set page table pointer
@@ -453,6 +454,7 @@ __xsc3_setup:mcrp15,0,ip,c7,c10,4@datawritebarriermcrp15,0,ip,c7,c5,4@prefetchflushmcrp15,0,ip,c8,c7,0@invalidateIandDTLBs+movr4,r4,lsl#ARCH_PGD_SHIFTorrr4,r4,#0x18 @ cache the page table in L2mcrp15,0,r4,c2,c0,0@loadpagetablepointermvnr4,#0 @ do not set page table pointer
This patch adds a step in the init sequence, in order to recreate the kernel
code/data page table mappings prior to full paging initialization. This is
necessary on LPAE systems that run out of a physical address space outside the
4G limit. On these systems, this implementation provides a machine descriptor
hook that allows the PHYS_OFFSET to be overridden in a machine specific
fashion.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/mach/arch.h | 1 +
arch/arm/kernel/setup.c | 3 ++
arch/arm/mm/mmu.c | 57 ++++++++++++++++++++++++++++++++++++++
3 files changed, 61 insertions(+)
This patch adds support for 64-bit physical addresses in virt_to_phys
patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.
In addition to adding 64-bit support, this patch also adds a set_phys_offset()
helper that is needed on architectures that need to modify PHYS_OFFSET during
initialization.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
arch/arm/kernel/head.S | 6 ++++++
arch/arm/kernel/setup.c | 14 ++++++++++++++
3 files changed, 35 insertions(+), 7 deletions(-)
From: Nicolas Pitre <hidden> Date: 2012-08-04 06:49:15
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
This patch adds support for 64-bit physical addresses in virt_to_phys
patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.
You should explain _why_ you do not a real aadd/sub. I did deduce it
but that might not be obvious to everyone. Also this subtlety should be
commented in the code as well.
quoted hunk
In addition to adding 64-bit support, this patch also adds a set_phys_offset()
helper that is needed on architectures that need to modify PHYS_OFFSET during
initialization.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
arch/arm/kernel/head.S | 6 ++++++
arch/arm/kernel/setup.c | 14 ++++++++++++++
3 files changed, 35 insertions(+), 7 deletions(-)
As mentioned previously, this is just too ugly. Please make
__pv_phys_offset into a phys_addr_t instead and mask the low/high parts
as needed in __virt_to_phys().
Given the high part is always the same, isn't there a better way than an
add with 0 that could be done here? The add will force a load of 0 in a
register needlessly just to add a constant value to it. Your new
patching framework ought to be able to patch a mov (or a mvn)
instruction directly.
Nicolas
Hi Nicolas,
On 8/4/2012 2:49 AM, Nicolas Pitre wrote:
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
quoted
This patch adds support for 64-bit physical addresses in virt_to_phys
patching. This does not do real 64-bit add/sub, but instead patches in the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.
You should explain _why_ you do not a real aadd/sub. I did deduce it
but that might not be obvious to everyone. Also this subtlety should be
commented in the code as well.
We could not do an ADDS + ADC here because the carry is not guaranteed
to be retained and passed into the ADC. This is because the compiler is
free to insert all kinds of stuff between the two non-volatile asm blocks.
Is there another subtlety here that I have missed out on entirely?
quoted
In addition to adding 64-bit support, this patch also adds a set_phys_offset()
helper that is needed on architectures that need to modify PHYS_OFFSET during
initialization.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
arch/arm/kernel/head.S | 6 ++++++
arch/arm/kernel/setup.c | 14 ++++++++++++++
3 files changed, 35 insertions(+), 7 deletions(-)
As mentioned previously, this is just too ugly. Please make
__pv_phys_offset into a phys_addr_t instead and mask the low/high parts
as needed in __virt_to_phys().
Maybe u64 instead of phys_addr_t to keep the sizing non-variable?
Given the high part is always the same, isn't there a better way than an
add with 0 that could be done here? The add will force a load of 0 in a
register needlessly just to add a constant value to it. Your new
patching framework ought to be able to patch a mov (or a mvn)
instruction directly.
True. I'll try and figure out a better way of doing this.
Nicolas
Once again, thanks for the excellent feedback.
--
Thanks
- Cyril
From: Nicolas Pitre <hidden> Date: 2012-08-06 02:19:09
On Sun, 5 Aug 2012, Cyril Chemparathy wrote:
Hi Nicolas,
On 8/4/2012 2:49 AM, Nicolas Pitre wrote:
quoted
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
quoted
This patch adds support for 64-bit physical addresses in virt_to_phys
patching. This does not do real 64-bit add/sub, but instead patches in
the
upper 32-bits of the phys_offset directly into the output of virt_to_phys.
You should explain _why_ you do not a real aadd/sub. I did deduce it
but that might not be obvious to everyone. Also this subtlety should be
commented in the code as well.
We could not do an ADDS + ADC here because the carry is not guaranteed to be
retained and passed into the ADC. This is because the compiler is free to
insert all kinds of stuff between the two non-volatile asm blocks.
Is there another subtlety here that I have missed out on entirely?
The high bits for the valid physical memory address range for which
virt_to_phys and phys_to_virt can be used are always the same.
Therefore no aadition at all is needed, fake or real. Only providing
those bits in the top word for the value returned by virt_to_phys is
needed.
quoted
quoted
In addition to adding 64-bit support, this patch also adds a
set_phys_offset()
helper that is needed on architectures that need to modify PHYS_OFFSET
during
initialization.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 22 +++++++++++++++-------
arch/arm/kernel/head.S | 6 ++++++
arch/arm/kernel/setup.c | 14 ++++++++++++++
3 files changed, 35 insertions(+), 7 deletions(-)
As mentioned previously, this is just too ugly. Please make
__pv_phys_offset into a phys_addr_t instead and mask the low/high parts
as needed in __virt_to_phys().
Maybe u64 instead of phys_addr_t to keep the sizing non-variable?
No. When not using LPAE, we don't have to pay the price of a u64 value.
That's why the phys_addr_t type is conditionally defined. You already
do extra processing in virt_to_phys when sizeof(phys_addr_t) > 4 which
is perfect for dealing with this issue.
Nicolas
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
From: Nicolas Pitre <hidden> Date: 2012-08-04 06:24:09
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
Did you verify that this didn't introduce any compilation warning when
compiling for non LPAE? If so and there were no warnings then...
Acked-by: Nicolas Pitre <redacted>
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
Did you verify that this didn't introduce any compilation warning when
compiling for non LPAE? If so and there were no warnings then...
Yes. This series has been tested on vanilla ARMv7 Cortex-A8 non-LPAE
hardware as well.
From: Russell King - ARM Linux <hidden> Date: 2012-08-06 11:14:25
On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote:
quoted hunk
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
This as a whole does not fill me with a great amount of enthusiasm,
because it breaks some of the typechecking that we have here.
The whole point of __phys_to_virt() and __virt_to_phys() is that they work
on integer types, and warn if you dare to use them with pointers. Adding
a cast into them breaks that.
The whole point is that the typecasting is explicitly inside phys_to_virt()
and virt_to_phys() and not their macro counterparts.
Secondly, are you sure that this patch is correct on its own? You're
passing a u64 into assembly only expecting a 32-bit register. Have you
checked it does the right thing with a 64-bit phys_addr_t on both LE
and BE?
On 8/6/2012 7:14 AM, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote:
quoted
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
This as a whole does not fill me with a great amount of enthusiasm,
because it breaks some of the typechecking that we have here.
The whole point of __phys_to_virt() and __virt_to_phys() is that they work
on integer types, and warn if you dare to use them with pointers. Adding
a cast into them breaks that.
Understood. Thanks. The casts were needed to upgrade to 64-bit before
arithmetic. We should convert the non-patch __phys_to_virt and
__virt_to_phys to inlines to keep the typechecking intact.
The whole point is that the typecasting is explicitly inside phys_to_virt()
and virt_to_phys() and not their macro counterparts.
Secondly, are you sure that this patch is correct on its own? You're
passing a u64 into assembly only expecting a 32-bit register. Have you
checked it does the right thing with a 64-bit phys_addr_t on both LE
and BE?
We should explicitly pass in the lower order bits here, at least until
the next patch in the series fixes things up for 64-bit. Thanks.
We've tested with 64-bit and 32-bit phys_addr_t, but only on LE. Thanks
for pointing this out, we'll figure out a way to run BE as well.
--
Thanks
- Cyril
Hi Russell,
On 8/6/2012 7:14 AM, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2012 at 07:04:39PM -0400, Cyril Chemparathy wrote:
quoted
This patch fixes up the types used when converting back and forth between
physical and virtual addresses.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
This as a whole does not fill me with a great amount of enthusiasm,
because it breaks some of the typechecking that we have here.
The whole point of __phys_to_virt() and __virt_to_phys() is that they work
on integer types, and warn if you dare to use them with pointers. Adding
a cast into them breaks that.
The whole point is that the typecasting is explicitly inside phys_to_virt()
and virt_to_phys() and not their macro counterparts.
The casts in __phys_to_virt() and __virt_to_phys() were necessary to
widen integer types in case of LPAE without phys/virt patching.
I assume that this specifically is the typecasting that you are
concerned about. Would it be better then to convert these to inlines
then? That way we could get the typechecking, with proper widening as
needed.
--
Thanks
- Cyril
From: Vitaly Andrianov <redacted>
This patch fixes the initrd setup code to use phys_addr_t instead of assuming
32-bit addressing. Without this we cannot boot on systems where initrd is
located above the 4G physical address limit.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/mm/init.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
@@ -347,14 +347,14 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)#ifdef CONFIG_BLK_DEV_INITRDif(phys_initrd_size&&!memblock_is_region_memory(phys_initrd_start,phys_initrd_size)){-pr_err("INITRD: 0x%08lx+0x%08lx is not a memory region - disabling initrd\n",-phys_initrd_start,phys_initrd_size);+pr_err("INITRD: 0x%08llx+0x%08llx is not a memory region - disabling initrd\n",+(u64)phys_initrd_start,(u64)phys_initrd_size);phys_initrd_start=phys_initrd_size=0;}if(phys_initrd_size&&memblock_is_region_reserved(phys_initrd_start,phys_initrd_size)){-pr_err("INITRD: 0x%08lx+0x%08lx overlaps in-use memory region - disabling initrd\n",-phys_initrd_start,phys_initrd_size);+pr_err("INITRD: 0x%08llx+0x%08llx overlaps in-use memory region - disabling initrd\n",+(u64)phys_initrd_start,(u64)phys_initrd_size);phys_initrd_start=phys_initrd_size=0;}if(phys_initrd_size){
From: Nicolas Pitre <hidden> Date: 2012-08-04 06:57:29
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
quoted hunk
From: Vitaly Andrianov <redacted>
This patch fixes the initrd setup code to use phys_addr_t instead of assuming
32-bit addressing. Without this we cannot boot on systems where initrd is
located above the 4G physical address limit.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/mm/init.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Vitaly Andrianov <redacted>
This patch fixes the initrd setup code to use phys_addr_t instead of assuming
32-bit addressing. Without this we cannot boot on systems where initrd is
located above the 4G physical address limit.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/mm/init.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
@@ -0,0 +1,20 @@+CONFIG_EXPERIMENTAL=y+CONFIG_BLK_DEV_INITRD=y+CONFIG_ARCH_KEYSTONE=y+CONFIG_ARM_ARCH_TIMER=y+CONFIG_AEABI=y+CONFIG_HIGHMEM=y+CONFIG_VFP=y+CONFIG_NEON=y+# CONFIG_SUSPEND is not set+CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"+CONFIG_BLK_DEV_RAM=y+CONFIG_SERIAL_8250=y+CONFIG_SERIAL_8250_CONSOLE=y+CONFIG_SERIAL_OF_PLATFORM=y+CONFIG_PRINTK_TIME=y+CONFIG_DEBUG_KERNEL=y+CONFIG_DEBUG_INFO=y+CONFIG_DEBUG_USER=y+CONFIG_DEBUG_LL=y+CONFIG_EARLY_PRINTK=y
Texas Instruments Keystone family of multicore devices now includes an
upcoming slew of Cortex A15 based devices. This patch adds basic definitions
for a new Keystone sub-architecture in ARM.
Subsequent patches in this series will extend support to include SMP and take
advantage of the large physical memory addressing capabilities via LPAE.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
And some nitpicking:
This command line should not really be here. Most of what you put in it is not
generic to the platform at all.
In order to select the console, use an alias for the serial device.
Texas Instruments Keystone family of multicore devices now includes an
upcoming slew of Cortex A15 based devices. This patch adds basic definitions
for a new Keystone sub-architecture in ARM.
Subsequent patches in this series will extend support to include SMP and take
advantage of the large physical memory addressing capabilities via LPAE.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Thanks for taking a look, Arnd.
Any inputs on the other patches in this series? I'd ideally like to
have the LPAE fixes, and code patching changes merged in sooner than the
Keystone machine specific stuff.
This command line should not really be here. Most of what you put in it is not
generic to the platform at all.
In order to select the console, use an alias for the serial device.
Agreed. The DTS in general needs quite a bit of work.
On Wednesday 01 August 2012 11:41:08 Cyril Chemparathy wrote:
On 7/31/2012 7:16 PM, Arnd Bergmann wrote:
quoted
On Tuesday 31 July 2012, Cyril Chemparathy wrote:
quoted
Texas Instruments Keystone family of multicore devices now includes an
upcoming slew of Cortex A15 based devices. This patch adds basic definitions
for a new Keystone sub-architecture in ARM.
Subsequent patches in this series will extend support to include SMP and take
advantage of the large physical memory addressing capabilities via LPAE.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Thanks for taking a look, Arnd.
Any inputs on the other patches in this series?
From: Vitaly Andrianov <redacted>
The free_memmap() was mistakenly using unsigned long type to represent
physical addresses. This breaks on PAE systems where memory could be placed
above the 32-bit addressible limit.
This patch fixes this function to properly use phys_addr_t instead.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/mm/init.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -469,8 +469,8 @@ free_memmap(unsigned long start_pfn, unsigned long end_pfn)*Converttophysicaladdresses,and*roundstartupwardsandenddownwards.*/-pg=(unsignedlong)PAGE_ALIGN(__pa(start_pg));-pgend=(unsignedlong)__pa(end_pg)&PAGE_MASK;+pg=PAGE_ALIGN(__pa(start_pg));+pgend=__pa(end_pg)&PAGE_MASK;/**Iftherearefreepagesbetweenthese,
From: Nicolas Pitre <hidden> Date: 2012-08-04 06:54:19
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
From: Vitaly Andrianov <redacted>
The free_memmap() was mistakenly using unsigned long type to represent
physical addresses. This breaks on PAE systems where memory could be placed
above the 32-bit addressible limit.
This patch fixes this function to properly use phys_addr_t instead.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
@@ -469,8 +469,8 @@ free_memmap(unsigned long start_pfn, unsigned long end_pfn)*Converttophysicaladdresses,and*roundstartupwardsandenddownwards.*/-pg=(unsignedlong)PAGE_ALIGN(__pa(start_pg));-pgend=(unsignedlong)__pa(end_pg)&PAGE_MASK;+pg=PAGE_ALIGN(__pa(start_pg));+pgend=__pa(end_pg)&PAGE_MASK;/**Iftherearefreepagesbetweenthese,
The original phys_to_virt/virt_to_phys patching implementation relied on early
patching prior to MMU initialization. On PAE systems running out of >4G
address space, this would have entailed an additional round of patching after
switching over to the high address space.
The approach implemented here conceptually extends the original PHYS_OFFSET
patching implementation with the introduction of "early" patch stubs. Early
patch code is required to be functional out of the box, even before the patch
is applied. This is implemented by inserting functional (but inefficient)
load code into the .patch.code init section. Having functional code out of
the box then allows us to defer the init time patch application until later
in the init sequence.
In addition to fitting better with our need for physical address-space
switch-over, this implementation should be somewhat more extensible by virtue
of its more readable (and hackable) C implementation. This should prove
useful for other similar init time specialization needs, especially in light
of our multi-platform kernel initiative.
This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7
(Cortex-A8) device.
Note: the obtuse use of stringified symbols in patch_stub() and
early_patch_stub() is intentional. Theoretically this should have been
accomplished with formal operands passed into the asm block, but this requires
the use of the 'c' modifier for instantiating the long (e.g. .long %c0).
However, the 'c' modifier has been found to ICE certain versions of GCC, and
therefore we resort to stringified symbols here.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/patch.h | 123 +++++++++++++++++++++++++++++
arch/arm/kernel/module.c | 4 +
arch/arm/kernel/setup.c | 175 +++++++++++++++++++++++++++++++++++++++++
arch/arm/kernel/vmlinux.lds.S | 10 +++
4 files changed, 312 insertions(+)
create mode 100644 arch/arm/include/asm/patch.h
@@ -937,6 +938,178 @@ static int __init meminfo_cmp(const void *_a, const void *_b)returncmp<0?-1:cmp>0?1:0;}+staticintapply_patch_imm8_arm(conststructpatch_info*p)+{+u32insn,ninsn,op,*insn_ptr=p->insn_start;+u32imm,rot,val;+intsize=p->insn_end-p->insn_start;++if(size!=4){+pr_err("patch: bad template size %d\n",size);+return-EINVAL;+}++insn=__mem_to_opcode_arm(p->patch_data[1]);++/* disallow special unconditional instructions+*1111xxxxxxxxxxxxxxxxxxxxxxxxxxxx*/+if((insn>>24)==0xf){+pr_err("patch: unconditional insn %08x\n",insn);+return-EINVAL;+}++/* allow only data processing (immediate)+*xxxx001xxxxxxxxxxxxxxxxxxxxxxxxx*/+if(((insn>>25)&0x3)!=1){+pr_err("patch: unknown insn %08x\n",insn);+return-EINVAL;+}++/* extract op code */+op=(insn>>20)&0x1f;++/* disallow unsupported 10xxx op codes */+if(((op>>3)&0x3)==2){+pr_err("patch: unsupported opcode %08x\n",insn);+return-EINVAL;+}++/* disallow Rn == PC and Rd == PC */+if(((insn>>16)&0xf)==0xf||((insn>>12)&0xf)==0xf){+pr_err("patch: unsupported register %08x\n",insn);+return-EINVAL;+}++imm=*(u32*)p->patch_data[0];++rot=imm?__ffs(imm)/2:0;+val=imm>>(rot*2);+rot=(-rot)&0xf;++/* does this fit in 8-bit? */+if(val>0xff){+pr_err("patch: constant overflow %08x\n",imm);+return-EINVAL;+}++/* patch in new immediate and rotation */+ninsn=(insn&~0xfff)|(rot<<8)|val;++*insn_ptr=__opcode_to_mem_arm(ninsn);++return0;+}++staticintapply_patch_imm8_thumb(conststructpatch_info*p)+{+u32insn,ninsn,op,*insn_ptr=p->insn_start;+u32imm,rot,val;+intsize=p->insn_end-p->insn_start;+constu32supported_ops=(BIT(0)|/* and */+BIT(1)|/* bic */+BIT(2)|/* orr/mov */+BIT(3)|/* orn/mvn */+BIT(4)|/* eor */+BIT(8)|/* add */+BIT(10)|/* adc */+BIT(11)|/* sbc */+BIT(12)|/* sub */+BIT(13));/* rsb */++if(size!=4){+pr_err("patch: bad template size %d\n",size);+return-EINVAL;+}++insn=__mem_to_opcode_thumb32(p->patch_data[1]);+if(!__opcode_is_thumb32(insn)){+pr_err("patch: invalid thumb2 insn %08x\n",insn);+return-EINVAL;+}++/* allow only data processing (immediate)+*11110x0xxxx0xxxx0xxxxxxxxxxxxxxx*/+if((insn&0xfa008000)!=0xf0000000){+pr_err("patch: unknown insn %08x\n",insn);+return-EINVAL;+}++/* disallow Rn == PC and Rd == PC */+if(((insn>>8)&0xf)==0xf||((insn>>16)&0xf)==0xf){+pr_err("patch: unsupported register %08x\n",insn);+return-EINVAL;+}++/* extract op code */+op=(insn>>21)&0xf;++/* disallow unsupported opcodes */+if((supported_ops&BIT(op))==0){+pr_err("patch: unsupported opcode %x\n",op);+return-EINVAL;+}++imm=*(u32*)p->patch_data[0];++if(imm<=0xff){+rot=0;+val=imm;+}else{+rot=32-fls(imm);/* clz */+if(imm&~(0xff000000>>rot)){+pr_err("patch: constant overflow %08x\n",imm);+return-EINVAL;+}+val=(imm>>(24-rot))&0x7f;+rot+=8;/* encoded i:imm3:a */++/* pack least-sig rot bit into most-sig val bit */+val|=(rot&1)<<7;+rot>>=1;+}++ninsn=insn&~(BIT(26)|0x7<<12|0xff);+ninsn|=(rot>>3)<<26;/* field "i" */+ninsn|=(rot&0x7)<<12;/* field "imm3" */+ninsn|=val;++*insn_ptr=__opcode_to_mem_thumb32(ninsn);++return0;+}++intpatch_kernel(constvoid*table,unsignedsize)+{+conststructpatch_info*p=table,*end=(table+size);+boolthumb2=IS_ENABLED(CONFIG_THUMB2_KERNEL);++for(p=table;p<end;p=patch_next(p)){+inttype=p->type&PATCH_TYPE_MASK;+intret;++if(type==PATCH_IMM8){+ret=(thumb2?apply_patch_imm8_thumb(p):+apply_patch_imm8_arm(p));+}else{+pr_err("invalid patch type %d\n",type);+ret=-EINVAL;+}++if(ret<0)+returnret;+}+return0;+}++staticvoid__initinit_patch_kernel(void)+{+constvoid*start=&__patch_table_begin;+constvoid*end=&__patch_table_end;++BUG_ON(patch_kernel(start,end-start));+flush_icache_range(init_mm.start_code,init_mm.end_code);+}+void__initsetup_arch(char**cmdline_p){structmachine_desc*mdesc;
From: Nicolas Pitre <hidden> Date: 2012-08-04 05:38:37
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
The original phys_to_virt/virt_to_phys patching implementation relied on early
patching prior to MMU initialization. On PAE systems running out of >4G
address space, this would have entailed an additional round of patching after
switching over to the high address space.
The approach implemented here conceptually extends the original PHYS_OFFSET
patching implementation with the introduction of "early" patch stubs. Early
patch code is required to be functional out of the box, even before the patch
is applied. This is implemented by inserting functional (but inefficient)
load code into the .patch.code init section. Having functional code out of
the box then allows us to defer the init time patch application until later
in the init sequence.
In addition to fitting better with our need for physical address-space
switch-over, this implementation should be somewhat more extensible by virtue
of its more readable (and hackable) C implementation. This should prove
useful for other similar init time specialization needs, especially in light
of our multi-platform kernel initiative.
This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7
(Cortex-A8) device.
Note: the obtuse use of stringified symbols in patch_stub() and
early_patch_stub() is intentional. Theoretically this should have been
accomplished with formal operands passed into the asm block, but this requires
the use of the 'c' modifier for instantiating the long (e.g. .long %c0).
However, the 'c' modifier has been found to ICE certain versions of GCC, and
therefore we resort to stringified symbols here.
Signed-off-by: Cyril Chemparathy <redacted>
Please find a better name for this file. "patch" is way too generic and
commonly referring to something different. "runtime-patching" or similar
would be more descriptive.
This is complex enough to waarrant aa separate source file. Please move
those additions out from setup.c. Given a good name for the header file
above, the c file could share the same name.
You could use "exttern void __patch_table_begin" so those symbols don't
get any type that could be misused by mistake, while you still can take
their addresses.
+
+struct patch_info {
+ u32 type;
+ u32 size;
Given the possibly large number of table entries, some effort at making
those entries as compact as possible should be considered. For instance,
the type and size fields could be u8's and insn_end pointer replaced
with another size expressed as an u8. By placing all the u8's together
they would occupy a single word by themselves. The assembly stub would
only need a .align statement to reflect the c structure's padding.
[...]
Did you verify with some test program that your patching routines do
produce the same opcodes as the assembled equivalent for all possible
shift values? Especially for Thumb2 code which isn't as trivial to get
right as the ARM one.
Nicolas
Hi Nicolas,
On 8/4/2012 1:38 AM, Nicolas Pitre wrote:
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
quoted
The original phys_to_virt/virt_to_phys patching implementation relied on early
patching prior to MMU initialization. On PAE systems running out of >4G
address space, this would have entailed an additional round of patching after
switching over to the high address space.
The approach implemented here conceptually extends the original PHYS_OFFSET
patching implementation with the introduction of "early" patch stubs. Early
patch code is required to be functional out of the box, even before the patch
is applied. This is implemented by inserting functional (but inefficient)
load code into the .patch.code init section. Having functional code out of
the box then allows us to defer the init time patch application until later
in the init sequence.
In addition to fitting better with our need for physical address-space
switch-over, this implementation should be somewhat more extensible by virtue
of its more readable (and hackable) C implementation. This should prove
useful for other similar init time specialization needs, especially in light
of our multi-platform kernel initiative.
This code has been boot tested in both ARM and Thumb-2 modes on an ARMv7
(Cortex-A8) device.
Note: the obtuse use of stringified symbols in patch_stub() and
early_patch_stub() is intentional. Theoretically this should have been
accomplished with formal operands passed into the asm block, but this requires
the use of the 'c' modifier for instantiating the long (e.g. .long %c0).
However, the 'c' modifier has been found to ICE certain versions of GCC, and
therefore we resort to stringified symbols here.
Signed-off-by: Cyril Chemparathy <redacted>
Please find a better name for this file. "patch" is way too generic and
commonly referring to something different. "runtime-patching" or similar
would be more descriptive.
Sure. Does init-patch sound about right? We need to reflect the fact
that this is intended for init-time patching only.
This is complex enough to waarrant aa separate source file. Please move
those additions out from setup.c. Given a good name for the header file
above, the c file could share the same name.
You could use "exttern void __patch_table_begin" so those symbols don't
get any type that could be misused by mistake, while you still can take
their addresses.
Sure.
quoted
+
+struct patch_info {
+ u32 type;
+ u32 size;
Given the possibly large number of table entries, some effort at making
those entries as compact as possible should be considered. For instance,
the type and size fields could be u8's and insn_end pointer replaced
with another size expressed as an u8. By placing all the u8's together
they would occupy a single word by themselves. The assembly stub would
only need a .align statement to reflect the c structure's padding.
Thanks, will try and pack this struct up.
[...]
Did you verify with some test program that your patching routines do
produce the same opcodes as the assembled equivalent for all possible
shift values? Especially for Thumb2 code which isn't as trivial to get
right as the ARM one.
Not quite all, but I'm sure I can conjure up an off-line test harness to
do so.
Much appreciated feedback. Thanks for taking a look.
--
Thanks
- Cyril
You could use "exttern void __patch_table_begin" so those symbols don't
get any type that could be misused by mistake, while you still can take
their addresses.
Looks like we'll have to stick with a non-void type here. The compiler
throws a warning when we try to take the address of a void.
[...]
Did you verify with some test program that your patching routines do
produce the same opcodes as the assembled equivalent for all possible
shift values? Especially for Thumb2 code which isn't as trivial to get
right as the ARM one.
We've refactored the patching code into separate functions as:
static int do_patch_imm8_arm(u32 insn, u32 imm, u32 *ninsn);
static int do_patch_imm8_thumb(u32 insn, u32 imm, u32 *ninsn);
With this, the following test code has been used to verify the generated
instruction encoding:
u32 arm_check[] = {
0xe2810041, 0xe2810082, 0xe2810f41, 0xe2810f82, 0xe2810e41,
0xe2810e82, 0xe2810d41, 0xe2810d82, 0xe2810c41, 0xe2810c82,
0xe2810b41, 0xe2810b82, 0xe2810a41, 0xe2810a82, 0xe2810941,
0xe2810982, 0xe2810841, 0xe2810882, 0xe2810741, 0xe2810782,
0xe2810641, 0xe2810682, 0xe2810541, 0xe2810582, 0xe2810441,
};
u32 thumb_check[] = {
0xf1010081, 0xf5017081, 0xf5017001, 0xf5016081, 0xf5016001,
0xf5015081, 0xf5015001, 0xf5014081, 0xf5014001, 0xf5013081,
0xf5013001, 0xf5012081, 0xf5012001, 0xf5011081, 0xf5011001,
0xf5010081, 0xf5010001, 0xf1017081, 0xf1017001, 0xf1016081,
0xf1016001, 0xf1015081, 0xf1015001, 0xf1014081, 0xf1014001,
};
int do_test(void)
{
int i, ret;
u32 ninsn, insn;
insn = arm_check[0];
for (i = 0; i < ARRAY_SIZE(arm_check); i++) {
ret = do_patch_imm8_arm(insn, 0x41 << i, &ninsn);
if (ret < 0)
pr_err("patch failed at shift %d\n", i);
if (ninsn != arm_check[i])
pr_err("mismatch at %d, expect %x, got %x\n",
i, arm_check[i], ninsn);
}
insn = thumb_check[0];
for (i = 0; i < ARRAY_SIZE(thumb_check); i++) {
ret = do_patch_imm8_thumb(insn, 0x81 << i, &ninsn);
if (ret < 0)
pr_err("patch failed at shift %d\n", i);
if (ninsn != thumb_check[i])
pr_err("mismatch@%d, expect %x, got %x\n",
i, thumb_check[i], ninsn);
}
}
Any ideas on improving these tests?
--
Thanks
- Cyril
You could use "exttern void __patch_table_begin" so those symbols don't
get any type that could be misused by mistake, while you still can take
their addresses.
Looks like we'll have to stick with a non-void type here. The compiler throws
a warning when we try to take the address of a void.
Ah, I see. Bummer. This used not to be the case with older gcc
versions.
[...]
quoted
Did you verify with some test program that your patching routines do
produce the same opcodes as the assembled equivalent for all possible
shift values? Especially for Thumb2 code which isn't as trivial to get
right as the ARM one.
We've refactored the patching code into separate functions as:
static int do_patch_imm8_arm(u32 insn, u32 imm, u32 *ninsn);
static int do_patch_imm8_thumb(u32 insn, u32 imm, u32 *ninsn);
With this, the following test code has been used to verify the generated
instruction encoding:
u32 arm_check[] = {
0xe2810041, 0xe2810082, 0xe2810f41, 0xe2810f82, 0xe2810e41,
0xe2810e82, 0xe2810d41, 0xe2810d82, 0xe2810c41, 0xe2810c82,
0xe2810b41, 0xe2810b82, 0xe2810a41, 0xe2810a82, 0xe2810941,
0xe2810982, 0xe2810841, 0xe2810882, 0xe2810741, 0xe2810782,
0xe2810641, 0xe2810682, 0xe2810541, 0xe2810582, 0xe2810441,
};
Instead of using this array you could let the assembler do it for you
like this:
asm (" \n\
.arm \n\
arm_check: \n\
.set shft, 0 \n\
.rep 12 \n\
add r1, r2, #0x81 << \shft \n\
.set shft, \shft + 2 \n\
.endr \n\
");
Instead of using this array you could let the assembler do it for you
like this:
asm (" \n\
.arm \n\
arm_check: \n\
.set shft, 0 \n\
.rep 12 \n\
add r1, r2, #0x81 << \shft \n\
.set shft, \shft + 2 \n\
.endr \n\
");
Neat macro magic. Are you thinking that we build this in as a self test
in the code?
Thanks
-- Cyril.
Instead of using this array you could let the assembler do it for you
like this:
asm (" \n\
.arm \n\
arm_check: \n\
.set shft, 0 \n\
.rep 12 \n\
add r1, r2, #0x81 << \shft \n\
.set shft, \shft + 2 \n\
.endr \n\
");
Neat macro magic. Are you thinking that we build this in as a self test in
the code?
For such things, this is never a bad idea to have some test alongside
with the main code, especially if this is extended to more cases in the
future. It is too easy to break it in subtle ways.
See arch/arm/kernel/kprobes-test*.c for a precedent.
Nicolas
From: Russell King - ARM Linux <hidden> Date: 2012-08-08 16:06:37
On Wed, Aug 08, 2012 at 09:55:12AM -0400, Nicolas Pitre wrote:
On Wed, 8 Aug 2012, Cyril Chemparathy wrote:
quoted
Neat macro magic. Are you thinking that we build this in as a self test in
the code?
For such things, this is never a bad idea to have some test alongside
with the main code, especially if this is extended to more cases in the
future. It is too easy to break it in subtle ways.
See arch/arm/kernel/kprobes-test*.c for a precedent.
Done correctly, it shouldn't be a problem, but I wouldn't say that
arch/arm/kernel/kprobes-test*.c is done correctly. It's seen quite
a number of patching attempts since it was introduced for various
problems, and I've seen quite a number of builds fail for various
reasons in this file (none which I could be bothered to investigate.)
When the test code ends up causing more problems than the code it's
testing, something is definitely wrong.
From: Nicolas Pitre <hidden> Date: 2012-08-08 16:56:59
On Wed, 8 Aug 2012, Russell King - ARM Linux wrote:
On Wed, Aug 08, 2012 at 09:55:12AM -0400, Nicolas Pitre wrote:
quoted
On Wed, 8 Aug 2012, Cyril Chemparathy wrote:
quoted
Neat macro magic. Are you thinking that we build this in as a self test in
the code?
For such things, this is never a bad idea to have some test alongside
with the main code, especially if this is extended to more cases in the
future. It is too easy to break it in subtle ways.
See arch/arm/kernel/kprobes-test*.c for a precedent.
Done correctly, it shouldn't be a problem, but I wouldn't say that
arch/arm/kernel/kprobes-test*.c is done correctly. It's seen quite
a number of patching attempts since it was introduced for various
problems, and I've seen quite a number of builds fail for various
reasons in this file (none which I could be bothered to investigate.)
When the test code ends up causing more problems than the code it's
testing, something is definitely wrong.
I think we shouldn't compare the complexity of test code for kprobes and
test code for runtime patching code. The former, while more difficult
to keep compiling, has found loads of issues in the former kprobes code.
So it certainly paid back many times its cost in maintenance.
My mention of it wasn't about the actual test code implementation, but
rather about the fact that we do have test code in the tree which can be
enabled with a config option.
As for build failures with that test code, I'd suggest you simply drop a
note to Tixy who is normally very responsive. I randomly enable it
myself and didn't run into any issues yet.
Nicolas
On Wed, 2012-08-08 at 12:56 -0400, Nicolas Pitre wrote:
On Wed, 8 Aug 2012, Russell King - ARM Linux wrote:
quoted
Done correctly, it shouldn't be a problem, but I wouldn't say that
arch/arm/kernel/kprobes-test*.c is done correctly. It's seen quite
a number of patching attempts since it was introduced for various
problems, and I've seen quite a number of builds fail for various
reasons in this file (none which I could be bothered to investigate.)
<snip>
quoted
As for build failures with that test code, I'd suggest you simply drop a
note to Tixy who is normally very responsive.
Indeed. If there are build failures, I'm happy to investigate and fix.
--
Tixy
Err. You are asking the kernel to flush every single cache line
manually throughout the kernel code. That's a flush every 32-bytes
over maybe a few megabytes of address space.
This is one of the reasons we do the patching in assembly code before
the caches are enabled - so we don't have to worry about the interaction
with the CPU caches, which for this kind of application would be very
expensive.
Err. You are asking the kernel to flush every single cache line
manually throughout the kernel code. That's a flush every 32-bytes
over maybe a few megabytes of address space.
With a flush_cache_all(), we could avoid having to operate a cacheline
at a time, but that clobbers way more than necessary.
Maybe the better answer is to flush only the patched cachelines.
This is one of the reasons we do the patching in assembly code before
the caches are enabled - so we don't have to worry about the interaction
with the CPU caches, which for this kind of application would be very
expensive.
Sure, flushing caches is expensive. But then, so is running the
patching code with caches disabled. I guess memory access latencies
drive the performance trade off here.
--
Thanks
- Cyril
From: Russell King - ARM Linux <hidden> Date: 2012-08-06 13:26:55
On Mon, Aug 06, 2012 at 09:19:10AM -0400, Cyril Chemparathy wrote:
With a flush_cache_all(), we could avoid having to operate a cacheline
at a time, but that clobbers way more than necessary.
You can't do that, because flush_cache_all() on some CPUs requires the
proper MMU mappings to be in place, and you can't get those mappings
in place because you don't have the V:P offsets fixed up in the kernel.
Welcome to the chicken and egg problem.
Sure, flushing caches is expensive. But then, so is running the
patching code with caches disabled. I guess memory access latencies
drive the performance trade off here.
There we disagree on a few orders of magnitude. There are relatively
few places that need updating. According to the kernel I have here:
text data bss dec hex filename
7644346 454320 212984 8311650 7ed362 vmlinux
Idx Name Size VMA LMA File off Algn
1 .text 004cd170 c00081c0 c00081c0 000081c0 2**5
16 .init.pv_table 00000300 c0753a24 c0753a24 00753a24 2**0
That's about 7MB of text, and only 192 points in that code which need
patching. Even if we did this with caches on, that's still 192 places,
and only 192 places we'd need to flush a cache line.
Alternatively, with your approach and 7MB of text, you need to flush
238885 cache lines to cover the entire kernel.
It would be far _cheaper_ with your approach to flush the individual
cache lines as you go.
On 8/6/2012 9:26 AM, Russell King - ARM Linux wrote:
On Mon, Aug 06, 2012 at 09:19:10AM -0400, Cyril Chemparathy wrote:
quoted
With a flush_cache_all(), we could avoid having to operate a cacheline
at a time, but that clobbers way more than necessary.
You can't do that, because flush_cache_all() on some CPUs requires the
proper MMU mappings to be in place, and you can't get those mappings
in place because you don't have the V:P offsets fixed up in the kernel.
Welcome to the chicken and egg problem.
quoted
Sure, flushing caches is expensive. But then, so is running the
patching code with caches disabled. I guess memory access latencies
drive the performance trade off here.
There we disagree on a few orders of magnitude. There are relatively
few places that need updating. According to the kernel I have here:
text data bss dec hex filename
7644346 454320 212984 8311650 7ed362 vmlinux
Idx Name Size VMA LMA File off Algn
1 .text 004cd170 c00081c0 c00081c0 000081c0 2**5
16 .init.pv_table 00000300 c0753a24 c0753a24 00753a24 2**0
That's about 7MB of text, and only 192 points in that code which need
patching. Even if we did this with caches on, that's still 192 places,
and only 192 places we'd need to flush a cache line.
Alternatively, with your approach and 7MB of text, you need to flush
238885 cache lines to cover the entire kernel.
It would be far _cheaper_ with your approach to flush the individual
cache lines as you go.
From: Nicolas Pitre <hidden> Date: 2012-08-06 18:02:24
On Mon, 6 Aug 2012, Russell King - ARM Linux wrote:
On Mon, Aug 06, 2012 at 09:19:10AM -0400, Cyril Chemparathy wrote:
quoted
With a flush_cache_all(), we could avoid having to operate a cacheline
at a time, but that clobbers way more than necessary.
You can't do that, because flush_cache_all() on some CPUs requires the
proper MMU mappings to be in place, and you can't get those mappings
in place because you don't have the V:P offsets fixed up in the kernel.
Welcome to the chicken and egg problem.
This problem is fixed in this case by having the p2v and v2p code sites
using an out-of-line non optimized computation until those sites are
runtime patched with the inlined optimized computation we have today.
Nicolas
This patch moves the TTBR1 offset calculation and the T1SZ calculation out
of the TTB setup assembly code. This should not affect functionality in
any way, but improves code readability as well as readability of subsequent
patches in this series.
Signed-off-by: Cyril Chemparathy <redacted>
Signed-off-by: Vitaly Andrianov <redacted>
---
arch/arm/include/asm/pgtable-3level-hwdef.h | 10 ++++++++++
arch/arm/mm/proc-v7-3level.S | 16 ++++------------
2 files changed, 14 insertions(+), 12 deletions(-)
Keystone platforms have their physical memory mapped at an address outside the
32-bit physical range. A Keystone machine with 16G of RAM would find its
memory at 0x0800000000 - 0x0bffffffff.
For boot purposes, the interconnect supports a limited alias of some of this
memory within the 32-bit addressable space (0x80000000 - 0xffffffff). This
aliasing is implemented in hardware, and is not intended to be used much
beyond boot. For instance, DMA coherence does not work when running out of
this aliased address space.
Therefore, we've taken the approach of booting out of the low physical address
range, and subsequently we switch over to the high range once we're safely
inside machine specific territory. This patch implements this switch over
mechanism, which involves rewiring the TTBRs and page tables to point to the
new physical address space.
Signed-off-by: Vitaly Andrianov <redacted>
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/Kconfig | 1 +
arch/arm/boot/dts/keystone-sim.dts | 8 +++---
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/mach-keystone/include/mach/memory.h | 25 +++++++++++++++++
arch/arm/mach-keystone/keystone.c | 37 ++++++++++++++++++++++++++
arch/arm/mach-keystone/platsmp.c | 18 +++++++++++--
6 files changed, 84 insertions(+), 6 deletions(-)
@@ -74,6 +74,39 @@ static const char *keystone_match[] __initconst = {NULL,};+staticvoid__initkeystone_init_meminfo(void)+{+boollpae=IS_ENABLED(CONFIG_ARM_LPAE);+boolpvpatch=IS_ENABLED(CONFIG_ARM_PATCH_PHYS_VIRT);+phys_addr_tmem_start,mem_end;++BUG_ON(meminfo.nr_banks<1);++mem_start=meminfo.bank[0].start;+mem_end=mem_start+meminfo.bank[0].size-1;++/* 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){+panic("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):"");+}++if(mem_start<KEYSTONE_HIGH_PHYS_START||+mem_end>KEYSTONE_HIGH_PHYS_END){+panic("Invalid address space for memory (%08llx-%08llx)\n",+(u64)KEYSTONE_HIGH_PHYS_START,+(u64)KEYSTONE_HIGH_PHYS_END);+}++set_phys_offset(KEYSTONE_HIGH_PHYS_START);+}+DT_MACHINE_START(KEYSTONE,"Keystone")smp_ops(keystone_smp_ops).map_io=keystone_map_io,
@@ -51,17 +52,30 @@ static void __init keystone_smp_prepare_cpus(unsigned int max_cpus)/* nothing for now */}+staticvoid__cpuinitkeystone_secondary_initmem(void)+{+#ifdef CONFIG_ARM_LPAE+pgd_t*pgd0=pgd_offset_k(0);+cpu_set_ttbr(1,__pa(pgd0)+TTBR1_OFFSET);+local_flush_tlb_all();+#endif+}+staticvoid__cpuinitkeystone_secondary_init(unsignedintcpu){gic_secondary_init(0);+keystone_secondary_initmem();}staticint__cpuinitkeystone_boot_secondary(unsignedintcpu,structtask_struct*idle){unsignedlong*ptr;--ptr=phys_to_virt(0x800001f0);++ptr=IS_ENABLED(CONFIG_ARM_LPAE)?+phys_to_virt(KEYSTONE_HIGH_PHYS_START+0x1f0):+phys_to_virt(KEYSTONE_LOW_PHYS_START+0x1f0);+ptr[cpu]=virt_to_idmap(&secondary_startup);__cpuc_flush_dcache_area(ptr,sizeof(ptr)*4);
This patch replaces the original physical offset patching implementation
with one that uses the newly added patching framework. In the process, we now
unconditionally initialize the __pv_phys_offset and __pv_offset globals in the
head.S code.
Signed-off-by: Cyril Chemparathy <redacted>
---
arch/arm/include/asm/memory.h | 20 ++-------
arch/arm/kernel/head.S | 96 +++++------------------------------------
arch/arm/kernel/module.c | 5 ---
arch/arm/kernel/vmlinux.lds.S | 5 ---
4 files changed, 15 insertions(+), 111 deletions(-)
@@ -522,94 +525,17 @@ ENTRY(fixup_smp)ldmfdsp!,{r4-r6,pc}ENDPROC(fixup_smp)-#ifdef CONFIG_ARM_PATCH_PHYS_VIRT--/*__fixup_pv_table-patchthestubinstructionswiththedeltabetween-*PHYS_OFFSETandPAGE_OFFSET,whichisassumedtobe16MiBalignedand-*canbeexpressedbyanimmediateshifteroperand.Thestubinstruction-*hasaformof'(add|sub) rd, rn, #imm'.-*/-__HEAD-__fixup_pv_table:-adrr0,1f-ldmiar0,{r3-r5,r7}-subr3,r0,r3@PHYS_OFFSET-PAGE_OFFSET-addr4,r4,r3@adjusttablestartaddress-addr5,r5,r3@adjusttableendaddress-addr7,r7,r3@adjust__pv_phys_offsetaddress-strr8,[r7]@savecomputedPHYS_OFFSETto__pv_phys_offset-movr6,r3,lsr#24 @ constant for add/sub instructions-teqr3,r6,lsl#24 @ must be 16MiB aligned-THUMB(itne@crosssectionbranch)-bne__error-strr6,[r7,#4] @ save to __pv_offset-b__fixup_a_pv_table-ENDPROC(__fixup_pv_table)--.align-1:.long.-.long__pv_table_begin-.long__pv_table_end-2:.long__pv_phys_offset--.text-__fixup_a_pv_table:-#ifdef CONFIG_THUMB2_KERNEL-lslsr6,#24-beq2f-clzr7,r6-lsrr6,#24-lslr6,r7-bicr6,#0x0080-lsrsr7,#1-orrcsr6,#0x0080-orrr6,r6,r7,lsl#12-orrr6,#0x4000-b2f-1:addr7,r3-ldrhip,[r7,#2]-andip,0x8f00-orrip,r6@maskinoffsetbits31-24-strhip,[r7,#2]-2:cmpr4,r5-ldrccr7,[r4],#4 @ use branch for delay slot-bcc1b-bxlr-#else-b2f-1:ldrip,[r7,r3]-bicip,ip,#0x000000ff-orrip,ip,r6@maskinoffsetbits31-24-strip,[r7,r3]-2:cmpr4,r5-ldrccr7,[r4],#4 @ use branch for delay slot-bcc1b-movpc,lr-#endif-ENDPROC(__fixup_a_pv_table)--ENTRY(fixup_pv_table)-stmfdsp!,{r4-r7,lr}-ldrr2,2f@getaddressof__pv_phys_offset-movr3,#0 @ no offset-movr4,r0@r0=tablestart-addr5,r0,r1@r1=tablesize-ldrr6,[r2,#4] @ get __pv_offset-bl__fixup_a_pv_table-ldmfdsp!,{r4-r7,pc}-ENDPROC(fixup_pv_table)--.align-2:.long__pv_phys_offset-.data.globl__pv_phys_offset.type__pv_phys_offset,%object __pv_phys_offset:.long0.size__pv_phys_offset,.-__pv_phys_offset++.globl__pv_offset+.type__pv_offset,%object __pv_offset:.long0-#endif+.size__pv_offset,.-__pv_offset#include "head-common.S"
From: Nicolas Pitre <hidden> Date: 2012-08-04 06:16:01
On Tue, 31 Jul 2012, Cyril Chemparathy wrote:
This patch replaces the original physical offset patching implementation
with one that uses the newly added patching framework. In the process, we now
unconditionally initialize the __pv_phys_offset and __pv_offset globals in the
head.S code.
Why unconditionally initializing those? There is no reason for that.
Please move those to C code. They aren't of much use in this file
anymore. This will allow you to use pphys_addr_t for them as well in
your subsequent patch. And more importantly get rid of that ugly
pv_offset_high that you introduced iin another patch.
The patch_kernel code and its invokation should still be conditional on
CONFIG_ARM_PATCH_PHYS_VIRT. This ability may still be configured out
irrespective of the implementation used.
Since you're changing the module ABI,it is important to also modify the
module vermagic string in asm/module.h to prevent the loading of
incompatible kernel modules.
Nicolas