powerpc: Fix up RTAS invocation for new qemu versions
diff mbox series

Message ID 20191004103844.32590-1-david@gibson.dropbear.id.au
State Accepted
Headers show
Series
  • powerpc: Fix up RTAS invocation for new qemu versions
Related show

Commit Message

David Gibson Oct. 4, 2019, 10:38 a.m. UTC
In order to call RTAS functions on powerpc kvm-unit-tests relies on the
RTAS blob supplied by qemu.  But new versions of qemu don't supply an RTAS
blob: since the normal way for guests to get RTAS is to call the guest
firmware's instantiate-rtas function, we now rely on that guest firmware
to provide the RTAS code itself.

But qemu-kvm-tests bypasses the usual guest firmware to just run itself,
so we can't get the rtas blob from SLOF.

But.. in fact the RTAS blob under qemu is a bit of a sham anyway - it's
a tiny wrapper that forwards the RTAS call to a hypercall.  So, we can
just invoke that hypercall directly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 lib/powerpc/asm/hcall.h |  3 +++
 lib/powerpc/rtas.c      |  6 +++---
 powerpc/cstart64.S      | 20 ++++++++++++++++----
 3 files changed, 22 insertions(+), 7 deletions(-)

So.. "new versions of qemu" in this case means ones that incorporate
the pull request I just sent today.

Comments

Paolo Bonzini Oct. 4, 2019, 11:54 a.m. UTC | #1
On 04/10/19 12:38, David Gibson wrote:
> In order to call RTAS functions on powerpc kvm-unit-tests relies on the
> RTAS blob supplied by qemu.  But new versions of qemu don't supply an RTAS
> blob: since the normal way for guests to get RTAS is to call the guest
> firmware's instantiate-rtas function, we now rely on that guest firmware
> to provide the RTAS code itself.
> 
> But qemu-kvm-tests bypasses the usual guest firmware to just run itself,
> so we can't get the rtas blob from SLOF.
> 
> But.. in fact the RTAS blob under qemu is a bit of a sham anyway - it's
> a tiny wrapper that forwards the RTAS call to a hypercall.  So, we can
> just invoke that hypercall directly.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Would it be hard to call instantiate-rtas?  I'm not sure if you have any
interest in running kvm-unit-tests on hypervisors that might have a
different way to process RTAS calls.

Paolo

> ---
>  lib/powerpc/asm/hcall.h |  3 +++
>  lib/powerpc/rtas.c      |  6 +++---
>  powerpc/cstart64.S      | 20 ++++++++++++++++----
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> So.. "new versions of qemu" in this case means ones that incorporate
> the pull request I just sent today.
> 
> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> index a8bd7e3..1173fea 100644
> --- a/lib/powerpc/asm/hcall.h
> +++ b/lib/powerpc/asm/hcall.h
> @@ -24,6 +24,9 @@
>  #define H_RANDOM		0x300
>  #define H_SET_MODE		0x31C
>  
> +#define KVMPPC_HCALL_BASE	0xf000
> +#define KVMPPC_H_RTAS		(KVMPPC_HCALL_BASE + 0x0)
> +
>  #ifndef __ASSEMBLY__
>  /*
>   * hcall_have_broken_sc1 checks if we're on a host with a broken sc1.
> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
> index 2e7e0da..41c0a24 100644
> --- a/lib/powerpc/rtas.c
> +++ b/lib/powerpc/rtas.c
> @@ -46,9 +46,9 @@ void rtas_init(void)
>  	prop = fdt_get_property(dt_fdt(), node,
>  				"linux,rtas-entry", &len);
>  	if (!prop) {
> -		printf("%s: /rtas/linux,rtas-entry: %s\n",
> -				__func__, fdt_strerror(len));
> -		abort();
> +		/* We don't have a qemu provided RTAS blob, enter_rtas
> +		 * will use H_RTAS directly */
> +		return;
>  	}
>  	data = (u32 *)prop->data;
>  	rtas_entry = (unsigned long)fdt32_to_cpu(*data);
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index ec673b3..972851f 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -121,13 +121,25 @@ halt:
>  
>  .globl enter_rtas
>  enter_rtas:
> +	LOAD_REG_ADDR(r11, rtas_entry)
> +	ld	r10, 0(r11)
> +
> +	cmpdi	r10,0
> +	bne	external_rtas
> +
> +	/* Use H_RTAS directly */
> +	mr	r4,r3
> +	lis	r3,KVMPPC_H_RTAS@h
> +	ori	r3,r3,KVMPPC_H_RTAS@l
> +	b	hcall
> +
> +external_rtas:
> +	/* Use external RTAS blob */
>  	mflr	r0
>  	std	r0, 16(r1)
>  
> -	LOAD_REG_ADDR(r10, rtas_return_loc)
> -	mtlr	r10
> -	LOAD_REG_ADDR(r11, rtas_entry)
> -	ld	r10, 0(r11)
> +	LOAD_REG_ADDR(r11, rtas_return_loc)
> +	mtlr	r11
>  
>  	mfmsr	r11
>  	LOAD_REG_IMMEDIATE(r9, RTAS_MSR_MASK)
>
David Gibson Oct. 5, 2019, 8:11 a.m. UTC | #2
On Fri, Oct 04, 2019 at 01:54:35PM +0200, Paolo Bonzini wrote:
> On 04/10/19 12:38, David Gibson wrote:
> > In order to call RTAS functions on powerpc kvm-unit-tests relies on the
> > RTAS blob supplied by qemu.  But new versions of qemu don't supply an RTAS
> > blob: since the normal way for guests to get RTAS is to call the guest
> > firmware's instantiate-rtas function, we now rely on that guest firmware
> > to provide the RTAS code itself.
> > 
> > But qemu-kvm-tests bypasses the usual guest firmware to just run itself,
> > so we can't get the rtas blob from SLOF.
> > 
> > But.. in fact the RTAS blob under qemu is a bit of a sham anyway - it's
> > a tiny wrapper that forwards the RTAS call to a hypercall.  So, we can
> > just invoke that hypercall directly.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Would it be hard to call instantiate-rtas?

Fairly.  First, we'd have to not use -bios to replace the normal SLOF
firmware - that could significantly slow down tests, since SLOF itself
takes a little while to start up.  Then we'd need to get the unit
tests to boot out of slof which is a different interface from the raw
one we're using now.  Then we'd need to get it to use the OF client
interface to call instantiate-rtas - we currently don't ever call OF
(obviously, since we don't even load it).

> I'm not sure if you have any
> interest in running kvm-unit-tests on hypervisors that might have a
> different way to process RTAS calls.

Probably not..?

> 
> Paolo
> 
> > ---
> >  lib/powerpc/asm/hcall.h |  3 +++
> >  lib/powerpc/rtas.c      |  6 +++---
> >  powerpc/cstart64.S      | 20 ++++++++++++++++----
> >  3 files changed, 22 insertions(+), 7 deletions(-)
> > 
> > So.. "new versions of qemu" in this case means ones that incorporate
> > the pull request I just sent today.
> > 
> > diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> > index a8bd7e3..1173fea 100644
> > --- a/lib/powerpc/asm/hcall.h
> > +++ b/lib/powerpc/asm/hcall.h
> > @@ -24,6 +24,9 @@
> >  #define H_RANDOM		0x300
> >  #define H_SET_MODE		0x31C
> >  
> > +#define KVMPPC_HCALL_BASE	0xf000
> > +#define KVMPPC_H_RTAS		(KVMPPC_HCALL_BASE + 0x0)
> > +
> >  #ifndef __ASSEMBLY__
> >  /*
> >   * hcall_have_broken_sc1 checks if we're on a host with a broken sc1.
> > diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
> > index 2e7e0da..41c0a24 100644
> > --- a/lib/powerpc/rtas.c
> > +++ b/lib/powerpc/rtas.c
> > @@ -46,9 +46,9 @@ void rtas_init(void)
> >  	prop = fdt_get_property(dt_fdt(), node,
> >  				"linux,rtas-entry", &len);
> >  	if (!prop) {
> > -		printf("%s: /rtas/linux,rtas-entry: %s\n",
> > -				__func__, fdt_strerror(len));
> > -		abort();
> > +		/* We don't have a qemu provided RTAS blob, enter_rtas
> > +		 * will use H_RTAS directly */
> > +		return;
> >  	}
> >  	data = (u32 *)prop->data;
> >  	rtas_entry = (unsigned long)fdt32_to_cpu(*data);
> > diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> > index ec673b3..972851f 100644
> > --- a/powerpc/cstart64.S
> > +++ b/powerpc/cstart64.S
> > @@ -121,13 +121,25 @@ halt:
> >  
> >  .globl enter_rtas
> >  enter_rtas:
> > +	LOAD_REG_ADDR(r11, rtas_entry)
> > +	ld	r10, 0(r11)
> > +
> > +	cmpdi	r10,0
> > +	bne	external_rtas
> > +
> > +	/* Use H_RTAS directly */
> > +	mr	r4,r3
> > +	lis	r3,KVMPPC_H_RTAS@h
> > +	ori	r3,r3,KVMPPC_H_RTAS@l
> > +	b	hcall
> > +
> > +external_rtas:
> > +	/* Use external RTAS blob */
> >  	mflr	r0
> >  	std	r0, 16(r1)
> >  
> > -	LOAD_REG_ADDR(r10, rtas_return_loc)
> > -	mtlr	r10
> > -	LOAD_REG_ADDR(r11, rtas_entry)
> > -	ld	r10, 0(r11)
> > +	LOAD_REG_ADDR(r11, rtas_return_loc)
> > +	mtlr	r11
> >  
> >  	mfmsr	r11
> >  	LOAD_REG_IMMEDIATE(r9, RTAS_MSR_MASK)
> > 
>
Paolo Bonzini Oct. 6, 2019, 4:16 p.m. UTC | #3
On 05/10/19 10:11, David Gibson wrote:
> Fairly.  First, we'd have to not use -bios to replace the normal SLOF
> firmware - that could significantly slow down tests, since SLOF itself
> takes a little while to start up.

Oh, I forgot that PPC kvm-unit-tests use -bios.  That makes everything moot.

Paolo
David Gibson Oct. 7, 2019, 5:22 a.m. UTC | #4
On Sun, Oct 06, 2019 at 06:16:17PM +0200, Paolo Bonzini wrote:
> On 05/10/19 10:11, David Gibson wrote:
> > Fairly.  First, we'd have to not use -bios to replace the normal SLOF
> > firmware - that could significantly slow down tests, since SLOF itself
> > takes a little while to start up.
> 
> Oh, I forgot that PPC kvm-unit-tests use -bios.  That makes
> everything moot.

Right.  I did consider putting a closer-to-conventional RTAS blob in
kbm-unit-tests' special "firmware" (which currently consists of a
single jump instruction).  It does help to build it BE which RTAS
expects.  On the other hand, I wasn't about to implement a full OF
client interface, so it would still have the main part of the tests
relying on finding the blob at some well known location.  And finding
a well known location that we can count on the tests proper not
clobbering is a bit of a pain as well.
Thomas Huth Oct. 9, 2019, 10:35 a.m. UTC | #5
On 04/10/2019 12.38, David Gibson wrote:
> In order to call RTAS functions on powerpc kvm-unit-tests relies on the
> RTAS blob supplied by qemu.  But new versions of qemu don't supply an RTAS
> blob: since the normal way for guests to get RTAS is to call the guest
> firmware's instantiate-rtas function, we now rely on that guest firmware
> to provide the RTAS code itself.
> 
> But qemu-kvm-tests bypasses the usual guest firmware to just run itself,

s/qemu-kvm-tests/kvm-unit-tests/

> so we can't get the rtas blob from SLOF.
> 
> But.. in fact the RTAS blob under qemu is a bit of a sham anyway - it's
> a tiny wrapper that forwards the RTAS call to a hypercall.  So, we can
> just invoke that hypercall directly.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  lib/powerpc/asm/hcall.h |  3 +++
>  lib/powerpc/rtas.c      |  6 +++---
>  powerpc/cstart64.S      | 20 ++++++++++++++++----
>  3 files changed, 22 insertions(+), 7 deletions(-)
> 
> So.. "new versions of qemu" in this case means ones that incorporate
> the pull request I just sent today.
> 
> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> index a8bd7e3..1173fea 100644
> --- a/lib/powerpc/asm/hcall.h
> +++ b/lib/powerpc/asm/hcall.h
> @@ -24,6 +24,9 @@
>  #define H_RANDOM		0x300
>  #define H_SET_MODE		0x31C
>  
> +#define KVMPPC_HCALL_BASE	0xf000
> +#define KVMPPC_H_RTAS		(KVMPPC_HCALL_BASE + 0x0)
> +
>  #ifndef __ASSEMBLY__
>  /*
>   * hcall_have_broken_sc1 checks if we're on a host with a broken sc1.
> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
> index 2e7e0da..41c0a24 100644
> --- a/lib/powerpc/rtas.c
> +++ b/lib/powerpc/rtas.c
> @@ -46,9 +46,9 @@ void rtas_init(void)
>  	prop = fdt_get_property(dt_fdt(), node,
>  				"linux,rtas-entry", &len);
>  	if (!prop) {
> -		printf("%s: /rtas/linux,rtas-entry: %s\n",
> -				__func__, fdt_strerror(len));
> -		abort();
> +		/* We don't have a qemu provided RTAS blob, enter_rtas
> +		 * will use H_RTAS directly */
> +		return;
>  	}
>  	data = (u32 *)prop->data;
>  	rtas_entry = (unsigned long)fdt32_to_cpu(*data);
> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
> index ec673b3..972851f 100644
> --- a/powerpc/cstart64.S
> +++ b/powerpc/cstart64.S
> @@ -121,13 +121,25 @@ halt:
>  
>  .globl enter_rtas
>  enter_rtas:
> +	LOAD_REG_ADDR(r11, rtas_entry)
> +	ld	r10, 0(r11)
> +
> +	cmpdi	r10,0
> +	bne	external_rtas
> +
> +	/* Use H_RTAS directly */
> +	mr	r4,r3
> +	lis	r3,KVMPPC_H_RTAS@h
> +	ori	r3,r3,KVMPPC_H_RTAS@l
> +	b	hcall
> +
> +external_rtas:
> +	/* Use external RTAS blob */
>  	mflr	r0
>  	std	r0, 16(r1)
>  
> -	LOAD_REG_ADDR(r10, rtas_return_loc)
> -	mtlr	r10
> -	LOAD_REG_ADDR(r11, rtas_entry)
> -	ld	r10, 0(r11)
> +	LOAD_REG_ADDR(r11, rtas_return_loc)
> +	mtlr	r11
>  
>  	mfmsr	r11
>  	LOAD_REG_IMMEDIATE(r9, RTAS_MSR_MASK)
> 

With the typo in the commit message fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Paolo, I currently don't have any other kvm-unit-test patches pending,
could you pick this up directly, please?
Paolo Bonzini Oct. 9, 2019, 11:12 a.m. UTC | #6
On 09/10/19 12:35, Thomas Huth wrote:
> On 04/10/2019 12.38, David Gibson wrote:
>> In order to call RTAS functions on powerpc kvm-unit-tests relies on the
>> RTAS blob supplied by qemu.  But new versions of qemu don't supply an RTAS
>> blob: since the normal way for guests to get RTAS is to call the guest
>> firmware's instantiate-rtas function, we now rely on that guest firmware
>> to provide the RTAS code itself.
>>
>> But qemu-kvm-tests bypasses the usual guest firmware to just run itself,
> 
> s/qemu-kvm-tests/kvm-unit-tests/
> 
>> so we can't get the rtas blob from SLOF.
>>
>> But.. in fact the RTAS blob under qemu is a bit of a sham anyway - it's
>> a tiny wrapper that forwards the RTAS call to a hypercall.  So, we can
>> just invoke that hypercall directly.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  lib/powerpc/asm/hcall.h |  3 +++
>>  lib/powerpc/rtas.c      |  6 +++---
>>  powerpc/cstart64.S      | 20 ++++++++++++++++----
>>  3 files changed, 22 insertions(+), 7 deletions(-)
>>
>> So.. "new versions of qemu" in this case means ones that incorporate
>> the pull request I just sent today.
>>
>> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
>> index a8bd7e3..1173fea 100644
>> --- a/lib/powerpc/asm/hcall.h
>> +++ b/lib/powerpc/asm/hcall.h
>> @@ -24,6 +24,9 @@
>>  #define H_RANDOM		0x300
>>  #define H_SET_MODE		0x31C
>>  
>> +#define KVMPPC_HCALL_BASE	0xf000
>> +#define KVMPPC_H_RTAS		(KVMPPC_HCALL_BASE + 0x0)
>> +
>>  #ifndef __ASSEMBLY__
>>  /*
>>   * hcall_have_broken_sc1 checks if we're on a host with a broken sc1.
>> diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
>> index 2e7e0da..41c0a24 100644
>> --- a/lib/powerpc/rtas.c
>> +++ b/lib/powerpc/rtas.c
>> @@ -46,9 +46,9 @@ void rtas_init(void)
>>  	prop = fdt_get_property(dt_fdt(), node,
>>  				"linux,rtas-entry", &len);
>>  	if (!prop) {
>> -		printf("%s: /rtas/linux,rtas-entry: %s\n",
>> -				__func__, fdt_strerror(len));
>> -		abort();
>> +		/* We don't have a qemu provided RTAS blob, enter_rtas
>> +		 * will use H_RTAS directly */
>> +		return;
>>  	}
>>  	data = (u32 *)prop->data;
>>  	rtas_entry = (unsigned long)fdt32_to_cpu(*data);
>> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
>> index ec673b3..972851f 100644
>> --- a/powerpc/cstart64.S
>> +++ b/powerpc/cstart64.S
>> @@ -121,13 +121,25 @@ halt:
>>  
>>  .globl enter_rtas
>>  enter_rtas:
>> +	LOAD_REG_ADDR(r11, rtas_entry)
>> +	ld	r10, 0(r11)
>> +
>> +	cmpdi	r10,0
>> +	bne	external_rtas
>> +
>> +	/* Use H_RTAS directly */
>> +	mr	r4,r3
>> +	lis	r3,KVMPPC_H_RTAS@h
>> +	ori	r3,r3,KVMPPC_H_RTAS@l
>> +	b	hcall
>> +
>> +external_rtas:
>> +	/* Use external RTAS blob */
>>  	mflr	r0
>>  	std	r0, 16(r1)
>>  
>> -	LOAD_REG_ADDR(r10, rtas_return_loc)
>> -	mtlr	r10
>> -	LOAD_REG_ADDR(r11, rtas_entry)
>> -	ld	r10, 0(r11)
>> +	LOAD_REG_ADDR(r11, rtas_return_loc)
>> +	mtlr	r11
>>  
>>  	mfmsr	r11
>>  	LOAD_REG_IMMEDIATE(r9, RTAS_MSR_MASK)
>>
> 
> With the typo in the commit message fixed:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Paolo, I currently don't have any other kvm-unit-test patches pending,
> could you pick this up directly, please?

Ok, thanks.

Paolo

Patch
diff mbox series

diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
index a8bd7e3..1173fea 100644
--- a/lib/powerpc/asm/hcall.h
+++ b/lib/powerpc/asm/hcall.h
@@ -24,6 +24,9 @@ 
 #define H_RANDOM		0x300
 #define H_SET_MODE		0x31C
 
+#define KVMPPC_HCALL_BASE	0xf000
+#define KVMPPC_H_RTAS		(KVMPPC_HCALL_BASE + 0x0)
+
 #ifndef __ASSEMBLY__
 /*
  * hcall_have_broken_sc1 checks if we're on a host with a broken sc1.
diff --git a/lib/powerpc/rtas.c b/lib/powerpc/rtas.c
index 2e7e0da..41c0a24 100644
--- a/lib/powerpc/rtas.c
+++ b/lib/powerpc/rtas.c
@@ -46,9 +46,9 @@  void rtas_init(void)
 	prop = fdt_get_property(dt_fdt(), node,
 				"linux,rtas-entry", &len);
 	if (!prop) {
-		printf("%s: /rtas/linux,rtas-entry: %s\n",
-				__func__, fdt_strerror(len));
-		abort();
+		/* We don't have a qemu provided RTAS blob, enter_rtas
+		 * will use H_RTAS directly */
+		return;
 	}
 	data = (u32 *)prop->data;
 	rtas_entry = (unsigned long)fdt32_to_cpu(*data);
diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
index ec673b3..972851f 100644
--- a/powerpc/cstart64.S
+++ b/powerpc/cstart64.S
@@ -121,13 +121,25 @@  halt:
 
 .globl enter_rtas
 enter_rtas:
+	LOAD_REG_ADDR(r11, rtas_entry)
+	ld	r10, 0(r11)
+
+	cmpdi	r10,0
+	bne	external_rtas
+
+	/* Use H_RTAS directly */
+	mr	r4,r3
+	lis	r3,KVMPPC_H_RTAS@h
+	ori	r3,r3,KVMPPC_H_RTAS@l
+	b	hcall
+
+external_rtas:
+	/* Use external RTAS blob */
 	mflr	r0
 	std	r0, 16(r1)
 
-	LOAD_REG_ADDR(r10, rtas_return_loc)
-	mtlr	r10
-	LOAD_REG_ADDR(r11, rtas_entry)
-	ld	r10, 0(r11)
+	LOAD_REG_ADDR(r11, rtas_return_loc)
+	mtlr	r11
 
 	mfmsr	r11
 	LOAD_REG_IMMEDIATE(r9, RTAS_MSR_MASK)