diff mbox

[kvm-unit-tests,1/2] powerpc: use well known kernel start address

Message ID 1461086788-3102-2-git-send-email-lvivier@redhat.com
State Accepted
Headers show

Commit Message

Laurent Vivier April 19, 2016, 5:26 p.m. UTC 
As we know where QEMU will load the kernel, it seems an useless
pain to try to compute it instead of using the well known value.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 powerpc/boot_rom.S | 3 ++-
 powerpc/cstart64.S | 6 +++---
 powerpc/spapr.h    | 6 ++++++
 3 files changed, 11 insertions(+), 4 deletions(-)
 create mode 100644 powerpc/spapr.h

Comments

David Gibson April 20, 2016, 2:01 a.m. UTC | #1 
On Tue, 19 Apr 2016 19:26:27 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> As we know where QEMU will load the kernel, it seems an useless
> pain to try to compute it instead of using the well known value.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Hmm.. I'm a little dubious about whether it's a good idea to require
that kernel load address to remain fixed in qemu.  I don't think it's
something we really have an interface guarantee for.

It might be ok given that we're just talking about the test shim here,
though.

> ---
>  powerpc/boot_rom.S | 3 ++-
>  powerpc/cstart64.S | 6 +++---
>  powerpc/spapr.h    | 6 ++++++
>  3 files changed, 11 insertions(+), 4 deletions(-)
>  create mode 100644 powerpc/spapr.h
> 
> diff --git a/powerpc/boot_rom.S b/powerpc/boot_rom.S
> index ae2c08d..266d61f 100644
> --- a/powerpc/boot_rom.S
> +++ b/powerpc/boot_rom.S
> @@ -1,4 +1,5 @@
> -#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> +#include "spapr.h"
> +
>  .text
>  .globl start
>  start:
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index ceb6397..c47b67d 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -11,6 +11,8 @@
>  #include <asm/rtas.h>
>  #include <asm/ptrace.h>
>  
> +#include "spapr.h"
> +
>  .section .init
>  
>  /*
> @@ -25,9 +27,7 @@ start:
>  	 * so we just linked at zero. This means the first thing to do is
>  	 * to find our stack and toc, and then do a relocate.
>  	 */
> -	bl	0f
> -0:	mflr	r31
> -	subi	r31, r31, 0b - start    /* QEMU's kernel load address */
> +	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
>  	ld	r1, (p_stack - start)(r31)
>  	ld	r2, (p_toc - start)(r31)
>  	add	r1, r1, r31
> diff --git a/powerpc/spapr.h b/powerpc/spapr.h
> new file mode 100644
> index 0000000..b41aece
> --- /dev/null
> +++ b/powerpc/spapr.h
> @@ -0,0 +1,6 @@
> +#ifndef _ASMPOWERPC_SPAPR_H_
> +#define _ASMPOWERPC_SPAPR_H_
> +
> +#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> +
> +#endif /* _ASMPOWERPC_SPAPR_H_ */
> -- 
> 2.5.5
>
Laurent Vivier April 20, 2016, 5:48 a.m. UTC | #2 
On 20/04/2016 04:01, David Gibson wrote:
> On Tue, 19 Apr 2016 19:26:27 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> As we know where QEMU will load the kernel, it seems an useless
>> pain to try to compute it instead of using the well known value.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Hmm.. I'm a little dubious about whether it's a good idea to require
> that kernel load address to remain fixed in qemu.  I don't think it's
> something we really have an interface guarantee for.
> 
> It might be ok given that we're just talking about the test shim here,
> though.

As it is hardcoded into boot_rom.S I see no obvious reason to not
hardcode it in cstart64.S.

Laurent

>> ---
>>  powerpc/boot_rom.S | 3 ++-
>>  powerpc/cstart64.S | 6 +++---
>>  powerpc/spapr.h    | 6 ++++++
>>  3 files changed, 11 insertions(+), 4 deletions(-)
>>  create mode 100644 powerpc/spapr.h
>>
>> diff --git a/powerpc/boot_rom.S b/powerpc/boot_rom.S
>> index ae2c08d..266d61f 100644
>> --- a/powerpc/boot_rom.S
>> +++ b/powerpc/boot_rom.S
>> @@ -1,4 +1,5 @@
>> -#define SPAPR_KERNEL_LOAD_ADDR 0x400000
>> +#include "spapr.h"
>> +
>>  .text
>>  .globl start
>>  start:
>> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
>> index ceb6397..c47b67d 100644
>> --- a/powerpc/cstart64.S
>> +++ b/powerpc/cstart64.S
>> @@ -11,6 +11,8 @@
>>  #include <asm/rtas.h>
>>  #include <asm/ptrace.h>
>>  
>> +#include "spapr.h"
>> +
>>  .section .init
>>  
>>  /*
>> @@ -25,9 +27,7 @@ start:
>>  	 * so we just linked at zero. This means the first thing to do is
>>  	 * to find our stack and toc, and then do a relocate.
>>  	 */
>> -	bl	0f
>> -0:	mflr	r31
>> -	subi	r31, r31, 0b - start    /* QEMU's kernel load address */
>> +	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
>>  	ld	r1, (p_stack - start)(r31)
>>  	ld	r2, (p_toc - start)(r31)
>>  	add	r1, r1, r31
>> diff --git a/powerpc/spapr.h b/powerpc/spapr.h
>> new file mode 100644
>> index 0000000..b41aece
>> --- /dev/null
>> +++ b/powerpc/spapr.h
>> @@ -0,0 +1,6 @@
>> +#ifndef _ASMPOWERPC_SPAPR_H_
>> +#define _ASMPOWERPC_SPAPR_H_
>> +
>> +#define SPAPR_KERNEL_LOAD_ADDR 0x400000
>> +
>> +#endif /* _ASMPOWERPC_SPAPR_H_ */
>> -- 
>> 2.5.5
>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson April 20, 2016, 5:55 a.m. UTC | #3 
On Wed, 20 Apr 2016 07:48:49 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 20/04/2016 04:01, David Gibson wrote:
> > On Tue, 19 Apr 2016 19:26:27 +0200
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> As we know where QEMU will load the kernel, it seems an useless
> >> pain to try to compute it instead of using the well known value.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>  
> > 
> > Hmm.. I'm a little dubious about whether it's a good idea to require
> > that kernel load address to remain fixed in qemu.  I don't think it's
> > something we really have an interface guarantee for.
> > 
> > It might be ok given that we're just talking about the test shim here,
> > though.  
> 
> As it is hardcoded into boot_rom.S I see no obvious reason to not
> hardcode it in cstart64.S.

True enough.

Reviewed-by: David Gibson <dgibson@redhat.com>

> 
> Laurent
> 
> >> ---
> >>  powerpc/boot_rom.S | 3 ++-
> >>  powerpc/cstart64.S | 6 +++---
> >>  powerpc/spapr.h    | 6 ++++++
> >>  3 files changed, 11 insertions(+), 4 deletions(-)
> >>  create mode 100644 powerpc/spapr.h
> >>
> >> diff --git a/powerpc/boot_rom.S b/powerpc/boot_rom.S
> >> index ae2c08d..266d61f 100644
> >> --- a/powerpc/boot_rom.S
> >> +++ b/powerpc/boot_rom.S
> >> @@ -1,4 +1,5 @@
> >> -#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> >> +#include "spapr.h"
> >> +
> >>  .text
> >>  .globl start
> >>  start:
> >> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> >> index ceb6397..c47b67d 100644
> >> --- a/powerpc/cstart64.S
> >> +++ b/powerpc/cstart64.S
> >> @@ -11,6 +11,8 @@
> >>  #include <asm/rtas.h>
> >>  #include <asm/ptrace.h>
> >>  
> >> +#include "spapr.h"
> >> +
> >>  .section .init
> >>  
> >>  /*
> >> @@ -25,9 +27,7 @@ start:
> >>  	 * so we just linked at zero. This means the first thing to do is
> >>  	 * to find our stack and toc, and then do a relocate.
> >>  	 */
> >> -	bl	0f
> >> -0:	mflr	r31
> >> -	subi	r31, r31, 0b - start    /* QEMU's kernel load address */
> >> +	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
> >>  	ld	r1, (p_stack - start)(r31)
> >>  	ld	r2, (p_toc - start)(r31)
> >>  	add	r1, r1, r31
> >> diff --git a/powerpc/spapr.h b/powerpc/spapr.h
> >> new file mode 100644
> >> index 0000000..b41aece
> >> --- /dev/null
> >> +++ b/powerpc/spapr.h
> >> @@ -0,0 +1,6 @@
> >> +#ifndef _ASMPOWERPC_SPAPR_H_
> >> +#define _ASMPOWERPC_SPAPR_H_
> >> +
> >> +#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> >> +
> >> +#endif /* _ASMPOWERPC_SPAPR_H_ */
> >> -- 
> >> 2.5.5
> >>  
> > 
> >
Thomas Huth April 20, 2016, 10:15 a.m. UTC | #4 
On 19.04.2016 19:26, Laurent Vivier wrote:
> As we know where QEMU will load the kernel, it seems an useless
> pain to try to compute it instead of using the well known value.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  powerpc/boot_rom.S | 3 ++-
>  powerpc/cstart64.S | 6 +++---
>  powerpc/spapr.h    | 6 ++++++
>  3 files changed, 11 insertions(+), 4 deletions(-)
>  create mode 100644 powerpc/spapr.h
> 
> diff --git a/powerpc/boot_rom.S b/powerpc/boot_rom.S
> index ae2c08d..266d61f 100644
> --- a/powerpc/boot_rom.S
> +++ b/powerpc/boot_rom.S
> @@ -1,4 +1,5 @@
> -#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> +#include "spapr.h"
> +
>  .text
>  .globl start
>  start:
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index ceb6397..c47b67d 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -11,6 +11,8 @@
>  #include <asm/rtas.h>
>  #include <asm/ptrace.h>
>  
> +#include "spapr.h"
> +
>  .section .init
>  
>  /*
> @@ -25,9 +27,7 @@ start:
>  	 * so we just linked at zero. This means the first thing to do is
>  	 * to find our stack and toc, and then do a relocate.
>  	 */
> -	bl	0f
> -0:	mflr	r31
> -	subi	r31, r31, 0b - start    /* QEMU's kernel load address */
> +	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
>  	ld	r1, (p_stack - start)(r31)
>  	ld	r2, (p_toc - start)(r31)
>  	add	r1, r1, r31
> diff --git a/powerpc/spapr.h b/powerpc/spapr.h
> new file mode 100644
> index 0000000..b41aece
> --- /dev/null
> +++ b/powerpc/spapr.h
> @@ -0,0 +1,6 @@
> +#ifndef _ASMPOWERPC_SPAPR_H_
> +#define _ASMPOWERPC_SPAPR_H_
> +
> +#define SPAPR_KERNEL_LOAD_ADDR 0x400000
> +
> +#endif /* _ASMPOWERPC_SPAPR_H_ */

Should be fine for now (and in case we ever want to run from SLOF
instead, for example, we still can rework this again), so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/powerpc/boot_rom.S b/powerpc/boot_rom.S
index ae2c08d..266d61f 100644
--- a/powerpc/boot_rom.S
+++ b/powerpc/boot_rom.S
@@ -1,4 +1,5 @@ 
-#define SPAPR_KERNEL_LOAD_ADDR 0x400000
+#include "spapr.h"
+
 .text
 .globl start
 start:
diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index ceb6397..c47b67d 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -11,6 +11,8 @@ 
 #include <asm/rtas.h>
 #include <asm/ptrace.h>
 
+#include "spapr.h"
+
 .section .init
 
 /*
@@ -25,9 +27,7 @@  start:
 	 * so we just linked at zero. This means the first thing to do is
 	 * to find our stack and toc, and then do a relocate.
 	 */
-	bl	0f
-0:	mflr	r31
-	subi	r31, r31, 0b - start    /* QEMU's kernel load address */
+	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
 	ld	r1, (p_stack - start)(r31)
 	ld	r2, (p_toc - start)(r31)
 	add	r1, r1, r31
diff --git a/powerpc/spapr.h b/powerpc/spapr.h
new file mode 100644
index 0000000..b41aece
--- /dev/null
+++ b/powerpc/spapr.h
@@ -0,0 +1,6 @@ 
+#ifndef _ASMPOWERPC_SPAPR_H_
+#define _ASMPOWERPC_SPAPR_H_
+
+#define SPAPR_KERNEL_LOAD_ADDR 0x400000
+
+#endif /* _ASMPOWERPC_SPAPR_H_ */