[PATCH 0/5] a few small fixups for arm64

[PATCH 0/5] a few small fixups for arm64

[Geoff Levand]

Hi Catalin,

Here are a few small fixups for arm64 that I need for my kexec work.

For PATCH #2 (arm64: Fix the soft_restart routine) I put the jump to
the identity mapped physical address in the caller of cpu_reset, soft_restart.
Another way is to put the jump into cpu_reset.  cpu_reset could also have a
conditional to check if running from a virtual address.  Let me know what you
prefer. 

-Geoff

The following changes since commit db4ed53cfe9f5a00355891a631d47dfa3fd4541f:

  arm64: mm: Fix PMD_SECT_PROT_NONE definition (2013-12-06 17:22:44 +0000)

are available in the git repository at:

  git://git.linaro.org/people/geoff.levand/linux-kexec.git for-arm64

for you to fetch changes up to 92660d1b81279aa6784a33eff2f102e78d883931:

  arm64: Add LOAD_OFFSET symbol for linker scripts (2013-12-12 12:13:04 -0800)

----------------------------------------------------------------
Geoff Levand (5):
      arm64: Fix __data_loc value
      arm64: Fix the soft_restart routine
      arm64: Fix include header order in vmlinux.lds.S
      arm64: Add missing AT() macros to vmlinux.lds.S
      arm64: Add LOAD_OFFSET symbol for linker scripts

 arch/arm64/include/asm/memory.h |  2 ++
 arch/arm64/kernel/process.c     |  8 +++++++-
 arch/arm64/kernel/vmlinux.lds.S | 14 +++++++-------
 3 files changed, 16 insertions(+), 8 deletions(-)

-- 
1.8.1.2

[PATCH 1/5] arm64: Fix __data_loc value

[Geoff Levand]

The __data_loc variable should be set to the VMA of the data section, not
the LMA.

At present LOAD_OFFSET is not set and defaults to zero, so this bug
does not cause any problems.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 5161ad9..3072c41 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -99,7 +99,7 @@ SECTIONS
 
 	. = ALIGN(PAGE_SIZE);
 	_data = .;
-	__data_loc = _data - LOAD_OFFSET;
+	__data_loc = _data;
 	_sdata = .;
 	RW_DATA_SECTION(64, PAGE_SIZE, THREAD_SIZE)
 	_edata = .;
-- 
1.8.1.2

[PATCH 1/5] arm64: Fix __data_loc value

[Russell King - ARM Linux]

On Thu, Dec 12, 2013 at 08:39:45PM +0000, Geoff Levand wrote:
> The __data_loc variable should be set to the VMA of the data section, not
> the LMA.
> 
> At present LOAD_OFFSET is not set and defaults to zero, so this bug
> does not cause any problems.

Why does ARM64 have this in the first place?  __data_loc is used for
XIP support on ARM, does ARM64 have XIP support?

[PATCH 1/5] arm64: Fix __data_loc value

[Catalin Marinas]

On Fri, Dec 13, 2013 at 05:46:47PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 12, 2013 at 08:39:45PM +0000, Geoff Levand wrote:
> > The __data_loc variable should be set to the VMA of the data section, not
> > the LMA.
> > 
> > At present LOAD_OFFSET is not set and defaults to zero, so this bug
> > does not cause any problems.
> 
> Why does ARM64 have this in the first place?  __data_loc is used for
> XIP support on ARM, does ARM64 have XIP support?

Not a good reason, copy-paste leftover.

-- 
Catalin

[PATCH 1/5] arm64: Fix __data_loc value

[Geoff Levand]

Hi Catalin,

On Fri, 2013-12-13 at 18:17 +0000, Catalin Marinas wrote:
> On Fri, Dec 13, 2013 at 05:46:47PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Dec 12, 2013 at 08:39:45PM +0000, Geoff Levand wrote:
> > > The __data_loc variable should be set to the VMA of the data section, not
> > > the LMA.
> > > 
> > > At present LOAD_OFFSET is not set and defaults to zero, so this bug
> > > does not cause any problems.
> > 
> > Why does ARM64 have this in the first place?  __data_loc is used for
> > XIP support on ARM, does ARM64 have XIP support?
> 
> Not a good reason, copy-paste leftover.

I didn't check how __data_loc was used.  I assumed it was of some use
and just did what was needed to suppress a link error in my work.

I'll post a patch that removes it.

-Geoff

[PATCH 4/5] arm64: Add missing AT() macros to vmlinux.lds.S

[Geoff Levand]

Add missing AT() macros to vmlinux.lds to generate the sections
properly.  Some elf file tools expect correct paddr values in
vmlinux.

Fixes bad paddr values written to vmlinux like these:

  Program Header:
    LOAD off 0x0000000000010000 vaddr 0xffffffc000080000 paddr 0xffffffc000080000 align 2**16

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/kernel/vmlinux.lds.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 6563b64..065fe40d 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -48,11 +48,11 @@ SECTIONS
 
 	. = PAGE_OFFSET + TEXT_OFFSET;
 
-	.head.text : {
+	.head.text : AT(ADDR(.head.text) - LOAD_OFFSET) {
 		_text = .;
 		HEAD_TEXT
 	}
-	.text : {			/* Real text segment		*/
+	.text : AT(ADDR(.text) - LOAD_OFFSET) { /* Real text segment	*/
 		_stext = .;		/* Text and read-only data	*/
 			__exception_text_start = .;
 			*(.exception.text)
@@ -77,11 +77,11 @@ SECTIONS
 	__init_begin = .;
 
 	INIT_TEXT_SECTION(8)
-	.exit.text : {
+	.exit.text : AT(ADDR(.exit.text) - LOAD_OFFSET) {
 		ARM_EXIT_KEEP(EXIT_TEXT)
 	}
 	. = ALIGN(16);
-	.init.data : {
+	.init.data : AT(ADDR(.init.data) - LOAD_OFFSET) {
 		INIT_DATA
 		INIT_SETUP(16)
 		INIT_CALLS
@@ -89,7 +89,7 @@ SECTIONS
 		SECURITY_INITCALL
 		INIT_RAM_FS
 	}
-	.exit.data : {
+	.exit.data : AT(ADDR(.exit.data) - LOAD_OFFSET){
 		ARM_EXIT_KEEP(EXIT_DATA)
 	}
 
-- 
1.8.1.2

[PATCH 4/5] arm64: Add missing AT() macros to vmlinux.lds.S

[Will Deacon]

On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> Add missing AT() macros to vmlinux.lds to generate the sections
> properly.  Some elf file tools expect correct paddr values in
> vmlinux.
> 
> Fixes bad paddr values written to vmlinux like these:
> 
>   Program Header:
>     LOAD off 0x0000000000010000 vaddr 0xffffffc000080000 paddr 0xffffffc000080000 align 2**16
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> ---
>  arch/arm64/kernel/vmlinux.lds.S | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 6563b64..065fe40d 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -48,11 +48,11 @@ SECTIONS
>  
>  	. = PAGE_OFFSET + TEXT_OFFSET;
>  
> -	.head.text : {
> +	.head.text : AT(ADDR(.head.text) - LOAD_OFFSET) {

Since LOAD_OFFSET is PAGE_OFFSET, does this assume that physical memory
starts at 0x0?

Will

[PATCH 4/5] arm64: Add missing AT() macros to vmlinux.lds.S

[Geoff Levand]

Hi Will,

On Fri, 2013-12-13 at 16:49 +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > Add missing AT() macros to vmlinux.lds to generate the sections
> > properly.  Some elf file tools expect correct paddr values in
> > vmlinux.
> > 
> > Fixes bad paddr values written to vmlinux like these:
> > 
> >   Program Header:
> >     LOAD off 0x0000000000010000 vaddr 0xffffffc000080000 paddr 0xffffffc000080000 align 2**16
> > 
> > Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> > ---
> >  arch/arm64/kernel/vmlinux.lds.S | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 6563b64..065fe40d 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -48,11 +48,11 @@ SECTIONS
> >  
> >  	. = PAGE_OFFSET + TEXT_OFFSET;
> >  
> > -	.head.text : {
> > +	.head.text : AT(ADDR(.head.text) - LOAD_OFFSET) {
> 
> Since LOAD_OFFSET is PAGE_OFFSET, does this assume that physical memory
> starts at 0x0?

Well, LOAD_OFFSET defaults to zero, so as of this patch, no, but
please see my comments to patch 5/5 (arm64: Add LOAD_OFFSET
symbol for linker scripts) where I do set LOAD_OFFSET...

The pre-defined macros from include/asm-generic/vmlinux.lds.h that we
use in our arm64 vmlinux.lds.S, like INIT_TEXT_SECTION() and BSS_SECTION(),
are all constructed using AT().  This patch is really just making all
the sections in the arm64 linker script consistent.  As I mentioned, when
LOAD_OFFSET is zero not having AT() macros on some sections doesn't cause
problems, but if LOAD_OFFSET is non-zero then some sections are not located
correctly resulting in a link error.

-Geoff

[PATCH 2/5] arm64: Fix the soft_restart routine

[Geoff Levand]

Change the soft_restart() routine to call cpu_reset() at its identity mapped
physical address.

The cpu_reset() routine must be called at its identity mapped physical address
so that when the MMU is turned off the instruction pointer will be at the correct
location in physical memory.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/kernel/process.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index de17c89..985eb42 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -72,7 +72,13 @@ static void setup_restart(void)
 void soft_restart(unsigned long addr)
 {
 	setup_restart();
-	cpu_reset(addr);
+
+	/*
+	 * cpu_reset turns the MMU off, so must be called at its identity
+	 * mapped physical address.
+	 */
+
+	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);
 }
 
 /*
-- 
1.8.1.2

[PATCH 2/5] arm64: Fix the soft_restart routine

[Will Deacon]

On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> Change the soft_restart() routine to call cpu_reset() at its identity mapped
> physical address.
> 
> The cpu_reset() routine must be called at its identity mapped physical address
> so that when the MMU is turned off the instruction pointer will be at the correct
> location in physical memory.
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> ---
>  arch/arm64/kernel/process.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index de17c89..985eb42 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -72,7 +72,13 @@ static void setup_restart(void)
>  void soft_restart(unsigned long addr)
>  {
>  	setup_restart();
> -	cpu_reset(addr);
> +
> +	/*
> +	 * cpu_reset turns the MMU off, so must be called at its identity
> +	 * mapped physical address.
> +	 */
> +
> +	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);

This isn't right; although cpu_reset *does* need to run from the idmap,
actually the idmap only includes __turn_mmu_on. You just get lucky because
its section-mapped and happens to include the code you want.

The cast is also cleaner if you define a phys_reset_t type, like we do for
arch/arm/.

Will

[PATCH 2/5] arm64: Fix the soft_restart routine

[Geoff Levand]

Hi Will,

On Fri, 2013-12-13 at 16:46 +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > +	/*
> > +	 * cpu_reset turns the MMU off, so must be called at its identity
> > +	 * mapped physical address.
> > +	 */
> > +
> > +	(*(void(*)(unsigned long))virt_to_phys(cpu_reset))(addr);
> 
> This isn't right; although cpu_reset *does* need to run from the idmap,
> actually the idmap only includes __turn_mmu_on. You just get lucky because
> its section-mapped and happens to include the code you want.

OK, I think this is different from fixing the call in soft_restart(), so
I'll make a separate patch for this.

I can see a few ways to do it but am not sure of the ramifications
of each.  Should we put cpu_reset into the idmap at startup when we add
__turn_mmu_on, or add cpu_reset on demand from say setup_mm_for_reboot()?

If the former, is all we need something like this?

@@ -438,6 +438,9 @@ __create_page_tables:
 	adr	x3, __turn_mmu_on		// virtual/physical address
 	create_pgd_entry x25, x0, x3, x5, x6
 	create_block_map x0, x7, x3, x5, x5, idmap=1
+	adr	x3, cpu_reset			// virtual/physical address
+	create_pgd_entry x25, x0, x3, x5, x6
+	create_block_map x0, x7, x3, x5, x5, idmap=1

> The cast is also cleaner if you define a phys_reset_t type, like we do for
> arch/arm/.

I'll send out an updated fix.

-Geoff

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Geoff Levand]

Add a definition for the LOAD_OFFSET symbol used in the linker
scripts.  LOAD_OFFSET is used to generate the load address of
the section.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/include/asm/memory.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 3776217..1994c56 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -52,6 +52,8 @@
 #define EARLYCON_IOBASE		(MODULES_VADDR - SZ_4M)
 #define TASK_SIZE_64		(UL(1) << VA_BITS)
 
+#define LOAD_OFFSET		(PAGE_OFFSET)
+
 #ifdef CONFIG_COMPAT
 #define TASK_SIZE_32		UL(0x100000000)
 #define TASK_SIZE		(test_thread_flag(TIF_32BIT) ? \
-- 
1.8.1.2

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Will Deacon]

On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> Add a definition for the LOAD_OFFSET symbol used in the linker
> scripts.  LOAD_OFFSET is used to generate the load address of
> the section.
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro

This isn't a standard SoB line. Please choose an email address and lose the
non-standard suffix.

> ---
>  arch/arm64/include/asm/memory.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 3776217..1994c56 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -52,6 +52,8 @@
>  #define EARLYCON_IOBASE		(MODULES_VADDR - SZ_4M)
>  #define TASK_SIZE_64		(UL(1) << VA_BITS)
>  
> +#define LOAD_OFFSET		(PAGE_OFFSET)

Can you be more specific about why we need this please? We don't seem to use
this on ARM, and I can't really think of a sensible value to define it as,
either. PAGE_OFFSET is a virtual address, which doesn't make much sense to
me, but perhaps I'm missing something.

Will

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Geoff Levand]

Hi Will,

On Fri, 2013-12-13 at 16:45 +0000, Will Deacon wrote:
> On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > Add a definition for the LOAD_OFFSET symbol used in the linker
> > scripts.  LOAD_OFFSET is used to generate the load address of
> > the section.
> > 
> > Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> 
> This isn't a standard SoB line. Please choose an email address and lose the
> non-standard suffix.

It took me a little while to find it, but here are the comments
regarding sign-off tags from the 2004 discussion:

  https://lkml.org/lkml/2004/5/25/108

I put on the extra tag for the benefit of those who generate patch
submission statistics.

> > ---
> >  arch/arm64/include/asm/memory.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index 3776217..1994c56 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -52,6 +52,8 @@
> >  #define EARLYCON_IOBASE		(MODULES_VADDR - SZ_4M)
> >  #define TASK_SIZE_64		(UL(1) << VA_BITS)
> >  
> > +#define LOAD_OFFSET		(PAGE_OFFSET)
> 
> Can you be more specific about why we need this please? We don't seem to use
> this on ARM, and I can't really think of a sensible value to define it as,
> either. PAGE_OFFSET is a virtual address, which doesn't make much sense to
> me, but perhaps I'm missing something.

As I mentioned before, LOAD_OFFSET defaults to zero if it is not defined by the
arch, so our arm64 vmlinux files currently have their paddr's equal to their
vaddr's, so something like 0xffffffc000080000.

kexec-tools uses the paddr from the elf file in its generic elf file loader.
Those kexec-tools routines do some sanity checks on the elf headers and one of
those checks is if the paddr seems sane.  A paddr of 0xffffffc000080000 fails
the test, and rightfully so I think.

I could make a special arm64 hack in kexec-tools to handle this, but I think
the proper thing to do is to get some sane paddr values in our vmlinux files.

I agree that PAGE_OFFSET isn't quite right, since zero is not really where the
kernel is located, but it is a handy base, as the proper location is an offset
from there.  My plan is to have the arm64 vmlinux loader routines in kexec-tools
either take a load offset from the kexec command line, or dig out the value from
the device tree.  I'm still working on that part and am not sure what will work.

I haven't looked into it yet, but we shouldn't have this problem with the boot
wrapper files, as we know where the kernel is located at from the device tree.

Sorry for such a long explanation.  Does it make sense?  Any suggestions would
be appreciated.

-Geoff

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Jason Gunthorpe]

On Fri, Dec 13, 2013 at 04:20:30PM -0800, Geoff Levand wrote:
> > Can you be more specific about why we need this please? We don't seem to use
> > this on ARM, and I can't really think of a sensible value to define it as,
> > either. PAGE_OFFSET is a virtual address, which doesn't make much sense to
> > me, but perhaps I'm missing something.
> 
> As I mentioned before, LOAD_OFFSET defaults to zero if it is not defined by the
> arch, so our arm64 vmlinux files currently have their paddr's equal to their
> vaddr's, so something like 0xffffffc000080000.

FWIW, I posted a similar patch for ARM32 last year:

https://lkml.org/lkml/2012/9/30/145

We are using it here to convey to desired load address to the boot
loader. My patch makes LOAD_OFFSET equal to PLAT_PHYS_OFFSET for
non-relocatable kernels, 0 otherwise.

If ARM64 takes Geoffs changes I can resend my patch for ARM32.

Regards,
Jason

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Will Deacon]

On Sat, Dec 14, 2013 at 12:20:30AM +0000, Geoff Levand wrote:
> On Fri, 2013-12-13 at 16:45 +0000, Will Deacon wrote:
> > On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > > Add a definition for the LOAD_OFFSET symbol used in the linker
> > > scripts.  LOAD_OFFSET is used to generate the load address of
> > > the section.
> > > 
> > > Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> > 
> > This isn't a standard SoB line. Please choose an email address and lose the
> > non-standard suffix.
> 
> It took me a little while to find it, but here are the comments
> regarding sign-off tags from the 2004 discussion:
> 
>   https://lkml.org/lkml/2004/5/25/108
> 
> I put on the extra tag for the benefit of those who generate patch
> submission statistics.

Hmm, there still doesn't appear to be a *single* occurrence of this in the
git log, even nearly a decade later. I'm also not very interested in patch
submission statistics, so I'd rather we stick with the status-quo.

> > > ---
> > >  arch/arm64/include/asm/memory.h | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > index 3776217..1994c56 100644
> > > --- a/arch/arm64/include/asm/memory.h
> > > +++ b/arch/arm64/include/asm/memory.h
> > > @@ -52,6 +52,8 @@
> > >  #define EARLYCON_IOBASE		(MODULES_VADDR - SZ_4M)
> > >  #define TASK_SIZE_64		(UL(1) << VA_BITS)
> > >  
> > > +#define LOAD_OFFSET		(PAGE_OFFSET)
> > 
> > Can you be more specific about why we need this please? We don't seem to use
> > this on ARM, and I can't really think of a sensible value to define it as,
> > either. PAGE_OFFSET is a virtual address, which doesn't make much sense to
> > me, but perhaps I'm missing something.
> 
> As I mentioned before, LOAD_OFFSET defaults to zero if it is not defined by the
> arch, so our arm64 vmlinux files currently have their paddr's equal to their
> vaddr's, so something like 0xffffffc000080000.
> 
> kexec-tools uses the paddr from the elf file in its generic elf file loader.
> Those kexec-tools routines do some sanity checks on the elf headers and one of
> those checks is if the paddr seems sane.  A paddr of 0xffffffc000080000 fails
> the test, and rightfully so I think.
> 
> I could make a special arm64 hack in kexec-tools to handle this, but I think
> the proper thing to do is to get some sane paddr values in our vmlinux files.

But how? There isn't an architected physical address map, so it's impossible
to pick a correct physical address base at link time.

> I agree that PAGE_OFFSET isn't quite right, since zero is not really where the
> kernel is located, but it is a handy base, as the proper location is an offset
> from there.  My plan is to have the arm64 vmlinux loader routines in kexec-tools
> either take a load offset from the kexec command line, or dig out the value from
> the device tree.  I'm still working on that part and am not sure what will work.

Using PAGE_OFFSET is worse than `isn't quite right'. In fact, it's only
right in exactly one case: where physical memory begins at 0x0!

> I haven't looked into it yet, but we shouldn't have this problem with the boot
> wrapper files, as we know where the kernel is located at from the device tree.
> 
> Sorry for such a long explanation.  Does it make sense?  Any suggestions would
> be appreciated.

I think kexec-tools needs to drop the expectation that the kernel is linked
to run at a particular physical address, since this isn't the case.

Will

[PATCH 5/5] arm64: Add LOAD_OFFSET symbol for linker scripts

[Geoff Levand]

Hi Will,

On Mon, 2013-12-16 at 17:29 +0000, Will Deacon wrote:
> On Sat, Dec 14, 2013 at 12:20:30AM +0000, Geoff Levand wrote:
> > On Fri, 2013-12-13 at 16:45 +0000, Will Deacon wrote:
> > > On Thu, Dec 12, 2013 at 08:39:46PM +0000, Geoff Levand wrote:
> > > > Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
> > > 
> > > This isn't a standard SoB line. Please choose an email address and lose the
> > > non-standard suffix.
> > 
> > It took me a little while to find it, but here are the comments
> > regarding sign-off tags from the 2004 discussion:
> > 
> >   https://lkml.org/lkml/2004/5/25/108
> > 
> > I put on the extra tag for the benefit of those who generate patch
> > submission statistics.
> 
> Hmm, there still doesn't appear to be a *single* occurrence of this in the
> git log, even nearly a decade later. I'm also not very interested in patch
> submission statistics, so I'd rather we stick with the status-quo.

One is at commit 534afb90a9cd0b9643f62d660c164e1d924f39cf (ppc32: fix
ppc440 pagetable attributes).  You can find others in the git history,
and some in the bitkeeper history also, so it is not unprecedented.
Linus says 'I think this is great. In general, I think people who want
to add their own extra tags after their email address should be able to
do so.'.  Can you still deny me this?

> > > > ---
> > > >  arch/arm64/include/asm/memory.h | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > > > index 3776217..1994c56 100644
> > > > --- a/arch/arm64/include/asm/memory.h
> > > > +++ b/arch/arm64/include/asm/memory.h
> > > > @@ -52,6 +52,8 @@
> > > >  #define EARLYCON_IOBASE		(MODULES_VADDR - SZ_4M)
> > > >  #define TASK_SIZE_64		(UL(1) << VA_BITS)
> > > >  
> > > > +#define LOAD_OFFSET		(PAGE_OFFSET)
> > > 
> > > Can you be more specific about why we need this please? We don't seem to use
> > > this on ARM, and I can't really think of a sensible value to define it as,
> > > either. PAGE_OFFSET is a virtual address, which doesn't make much sense to
> > > me, but perhaps I'm missing something.
> > 
> > As I mentioned before, LOAD_OFFSET defaults to zero if it is not defined by the
> > arch, so our arm64 vmlinux files currently have their paddr's equal to their
> > vaddr's, so something like 0xffffffc000080000.
> > 
> > kexec-tools uses the paddr from the elf file in its generic elf file loader.
> > Those kexec-tools routines do some sanity checks on the elf headers and one of
> > those checks is if the paddr seems sane.  A paddr of 0xffffffc000080000 fails
> > the test, and rightfully so I think.
> > 
> > I could make a special arm64 hack in kexec-tools to handle this, but I think
> > the proper thing to do is to get some sane paddr values in our vmlinux files.
> 
> But how? There isn't an architected physical address map, so it's impossible
> to pick a correct physical address base at link time.

Yes, and so just locate it at zero to make the tools happy.  It doesn't
change the behavior of the kernel in any way, it just sets the elf file
header values.

> > I agree that PAGE_OFFSET isn't quite right, since zero is not really where the
> > kernel is located, but it is a handy base, as the proper location is an offset
> > from there.  My plan is to have the arm64 vmlinux loader routines in kexec-tools
> > either take a load offset from the kexec command line, or dig out the value from
> > the device tree.  I'm still working on that part and am not sure what will work.
> 
> Using PAGE_OFFSET is worse than `isn't quite right'. In fact, it's only
> right in exactly one case: where physical memory begins at 0x0!
> 
> > I haven't looked into it yet, but we shouldn't have this problem with the boot
> > wrapper files, as we know where the kernel is located at from the device tree.
> > 
> > Sorry for such a long explanation.  Does it make sense?  Any suggestions would
> > be appreciated.
> 
> I think kexec-tools needs to drop the expectation that the kernel is linked
> to run at a particular physical address, since this isn't the case.

Sure, I can fixup kexec-tools.  It doesn't really matter that much to
me, but I suspect there are other loaders out there (Jason mentions his
arm32 loader) that will not like such paddr values in the elf header,
and is why I thought this change was of use.

-Geoff

[PATCH 3/5] arm64: Fix include header order in vmlinux.lds.S

[Geoff Levand]

asm-generic/vmlinux.lds.h should be included after the arch
specific headers so that the arch headers can override the
generic macro defs in asm-generic/vmlinux.lds.h.

Fixes preprosessor redefined warnings when adding arch specific
macros.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
 arch/arm64/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 3072c41..6563b64 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -4,10 +4,10 @@
  * Written by Martin Mares <mj@atrey.karlin.mff.cuni.cz>
  */
 
-#include <asm-generic/vmlinux.lds.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/page.h>
+#include <asm-generic/vmlinux.lds.h>
 
 #define ARM_EXIT_KEEP(x)
 #define ARM_EXIT_DISCARD(x)	x
-- 
1.8.1.2

[PATCH v2 1/5] arm64: Remove unused __data_loc variable

[Geoff Levand]

The __data_loc variable is an unused left over from the 32 bit arm implementation.
Remove that variable and adjust the __mmap_switched startup routine accordingly.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
v2: Change from fixing up the linker script to removing the __data_loc variable
    altogether.

Catalin,

As discussed, I removed all reference to __data_loc and _edata_loc, and also
the use of _data in the __mmap_switched routine, as is was just used in a
compare.  

I guess the data segment copy I removed is to support XIP.  This change
seems to work OK for me in a casual boot test.

Please consider.

-Geoff

 arch/arm64/kernel/head.S        | 10 ----------
 arch/arm64/kernel/vmlinux.lds.S |  2 --
 2 files changed, 12 deletions(-)

diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index c68cca5..0b281ff 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -482,8 +482,6 @@ ENDPROC(__create_page_tables)
 	.type	__switch_data, %object
 __switch_data:
 	.quad	__mmap_switched
-	.quad	__data_loc			// x4
-	.quad	_data				// x5
 	.quad	__bss_start			// x6
 	.quad	_end				// x7
 	.quad	processor_id			// x4
@@ -498,15 +496,7 @@ __switch_data:
 __mmap_switched:
 	adr	x3, __switch_data + 8
 
-	ldp	x4, x5, [x3], #16
 	ldp	x6, x7, [x3], #16
-	cmp	x4, x5				// Copy data segment if needed
-1:	ccmp	x5, x6, #4, ne
-	b.eq	2f
-	ldr	x16, [x4], #8
-	str	x16, [x5], #8
-	b	1b
-2:
 1:	cmp	x6, x7
 	b.hs	2f
 	str	xzr, [x6], #8			// Clear BSS
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 5161ad9..8a75386 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -99,11 +99,9 @@ SECTIONS
 
 	. = ALIGN(PAGE_SIZE);
 	_data = .;
-	__data_loc = _data - LOAD_OFFSET;
 	_sdata = .;
 	RW_DATA_SECTION(64, PAGE_SIZE, THREAD_SIZE)
 	_edata = .;
-	_edata_loc = __data_loc + SIZEOF(.data);
 
 	BSS_SECTION(0, 0, 0)
 	_end = .;
-- 
1.8.1.2

[PATCH v2 1/5] arm64: Remove unused __data_loc variable

[Catalin Marinas]

On Sat, Dec 14, 2013 at 12:20:13AM +0000, Geoff Levand wrote:
> The __data_loc variable is an unused left over from the 32 bit arm implementation.
> Remove that variable and adjust the __mmap_switched startup routine accordingly.
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro

Applied. Thanks.

-- 
Catalin

[PATCH v2 2/5] arm64: Fix the soft_restart routine

[Geoff Levand]

Change the soft_restart() routine to call cpu_reset() at its identity mapped
physical address.

The cpu_reset() routine must be called at its identity mapped physical address
so that when the MMU is turned off the instruction pointer will be at the correct
location in physical memory.

Signed-off-by: Geoff Levand <geoff@infradead.org> for Huawei, Linaro
---
v2: Use typecast and variable for call as in arm code.

 arch/arm64/kernel/process.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index de17c89..88e50fc 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -71,8 +71,14 @@ static void setup_restart(void)
 
 void soft_restart(unsigned long addr)
 {
+	typedef void (*phys_reset_t)(unsigned long);
+	phys_reset_t phys_reset;
+
 	setup_restart();
-	cpu_reset(addr);
+
+	/* Switch to the identity mapping. */
+	phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
+	phys_reset(addr);
 }
 
 /*
-- 
1.8.1.2