[kvm-unit-tests PATCH 0/2] Fix TOC value in exception handler

[kvm-unit-tests PATCH 0/2] Fix TOC value in exception handler

[Laurent Vivier]

As noticed by Thomas, if the TOC pointer (r2) is corrupted,
the code from the exception table cannot call the common exception handler
function.

This series tries to fix that by calling call_handler without using
the TOC pointer but a pointer to the function stored at an absolute
address in memory (as SLOF does), and then by restoring the value of
r2 before calling the user registered exception handler.

An easy way to to compute the TOC address seems to use directly the value
of the load address used by QEMU instead of computing it from the PC.

Laurent Vivier (2):
  powerpc: use well known kernel start address
  powerpc: restore TOC pointer

 powerpc/boot_rom.S |  3 ++-
 powerpc/cstart64.S | 23 +++++++++++++++++++----
 powerpc/spapr.h    |  6 ++++++
 3 files changed, 27 insertions(+), 5 deletions(-)
 create mode 100644 powerpc/spapr.h

-- 
2.5.5

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

[Laurent Vivier]

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_ */
-- 
2.5.5

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

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
> 


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Laurent Vivier]

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
>>
> 
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2711 bytes --]

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
> >>  
> > 
> >   


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

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>

[kvm-unit-tests PATCH 2/2] powerpc: restore TOC pointer

[Laurent Vivier]

As the TOC pointer can be corrupted by the main program,
we must restore it in the exception handler.

As we know where we are loaded, we can now compute it easily.

To compute it only in the common part of the exception handler
(call_handler), store the address of call_handler at an absolute
address in memory to be able to call the handler from the exception
table (as SLOF does).

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 powerpc/cstart64.S | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index c47b67d..1ddfa13 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -13,6 +13,8 @@
 
 #include "spapr.h"
 
+#define P_HANDLER	0x2ff8
+
 .section .init
 
 /*
@@ -46,6 +48,11 @@ start:
 	add	r4, r4, r31
 	bl	relocate
 
+	/* compute address of call_handler */
+
+	LOAD_REG_ADDR(r4, call_handler)
+	std	r4, P_HANDLER(0)
+
 	/* relocate vector table to base address 0x0 (MSR_IP = 0) */
 
 	/* source: r4, dest end: r5, destination: r6 */
@@ -166,6 +173,12 @@ call_handler:
 	mfsrr1	r0
 	std	r0, _MSR(r1)
 
+	/* restore TOC pointer */
+
+	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
+	ld	r2, (p_toc - start)(r31)
+	add	r2, r2, r31
+
 	/* FIXME: build stack frame */
 
 	/* call generic handler */
@@ -221,7 +234,7 @@ call_handler:
 	mfctr	r0
 	std	r0,_CTR(r1)
 
-	LOAD_REG_ADDR(r0, call_handler)
+	ld	r0, P_HANDLER(0)
 	mtctr	r0
 
 	li	r0,\vec
@@ -245,3 +258,5 @@ VECTOR(0x900)
 	.align 7
 	.globl __end_interrupts
 __end_interrupts:
+	.org	P_HANDLER
+	.llong	0
-- 
2.5.5

Re: [kvm-unit-tests PATCH 2/2] powerpc: restore TOC pointer

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

On Tue, 19 Apr 2016 19:26:28 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> As the TOC pointer can be corrupted by the main program,
> we must restore it in the exception handler.
> 
> As we know where we are loaded, we can now compute it easily.
> 
> To compute it only in the common part of the exception handler
> (call_handler), store the address of call_handler at an absolute
> address in memory to be able to call the handler from the exception
> table (as SLOF does).
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

So, this looks ok as long as the unit tests are built with a single
TOC.  The more stricly correct approach would be to actually load the
correct TOC pointer for the specific handler function you're going to
jump to.

That would need different paths for ABIv1 and v2, though (in practice
ppc64 vs. ppc64le).  With ABIv1 (ppc64) you'd need to load the TOC
pointer from the procedure descriptor into r2 before jumping to the
code address.  With ABIv2 (ppc64le), I believe there's both a "far" and
"near" entry point to the function - the "far" one has a few
instructions to load the correct TOC before continuing onto the "near"
version.  I forget which entry point the plain symbol references.  If
it's the "far" one, the current code is probably already correct.

> ---
>  powerpc/cstart64.S | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index c47b67d..1ddfa13 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -13,6 +13,8 @@
>  
>  #include "spapr.h"
>  
> +#define P_HANDLER	0x2ff8
> +
>  .section .init
>  
>  /*
> @@ -46,6 +48,11 @@ start:
>  	add	r4, r4, r31
>  	bl	relocate
>  
> +	/* compute address of call_handler */
> +
> +	LOAD_REG_ADDR(r4, call_handler)
> +	std	r4, P_HANDLER(0)
> +
>  	/* relocate vector table to base address 0x0 (MSR_IP = 0) */
>  
>  	/* source: r4, dest end: r5, destination: r6 */
> @@ -166,6 +173,12 @@ call_handler:
>  	mfsrr1	r0
>  	std	r0, _MSR(r1)
>  
> +	/* restore TOC pointer */
> +
> +	LOAD_REG_IMMEDIATE(r31, SPAPR_KERNEL_LOAD_ADDR)
> +	ld	r2, (p_toc - start)(r31)
> +	add	r2, r2, r31
> +
>  	/* FIXME: build stack frame */
>  
>  	/* call generic handler */
> @@ -221,7 +234,7 @@ call_handler:
>  	mfctr	r0
>  	std	r0,_CTR(r1)
>  
> -	LOAD_REG_ADDR(r0, call_handler)
> +	ld	r0, P_HANDLER(0)
>  	mtctr	r0
>  
>  	li	r0,\vec
> @@ -245,3 +258,5 @@ VECTOR(0x900)
>  	.align 7
>  	.globl __end_interrupts
>  __end_interrupts:
> +	.org	P_HANDLER
> +	.llong	0
> -- 
> 2.5.5
> 


-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [kvm-unit-tests PATCH 2/2] powerpc: restore TOC pointer

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On 20.04.2016 07:59, David Gibson wrote:
> On Tue, 19 Apr 2016 19:26:28 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> As the TOC pointer can be corrupted by the main program,
>> we must restore it in the exception handler.
>>
>> As we know where we are loaded, we can now compute it easily.
>>
>> To compute it only in the common part of the exception handler
>> (call_handler), store the address of call_handler at an absolute
>> address in memory to be able to call the handler from the exception
>> table (as SLOF does).
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> So, this looks ok as long as the unit tests are built with a single
> TOC.

In case there would be multiple TOCs, the previous exception handler
would not work at all anymore, since it does not set up r2 at all.

So for the current scope of kvm-unit-tests, I think this here is already
enough - we can still extend it later in case we ever need to support
multiple TOCs. So to me, this patch looks fine:

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]