rtas: Integrate RTAS blob
diff mbox series

Message ID 20190716051131.65756-1-aik@ozlabs.ru
State Superseded
Headers show
Series
  • rtas: Integrate RTAS blob
Related show

Commit Message

Alexey Kardashevskiy July 16, 2019, 5:11 a.m. UTC
We implement RTAS as a simple binary blob which calls directly into QEMU
via a custom hcall. So far we were relying on QEMU putting the RTAS blob
to the guest memory with its location in linux,rtas-base/rtas-size.

The problems with this are:
1. we need to peek a location in the guest ram in addition to slof, FDT
and sometime kernel and init ram disk; having one less image makes QEMU's
life easier.
2. for secure VMs, it is yet another image which needs to be signed and
verified.

This implements "instantiate-rtas" completely in SLOF, including KVM PR
support ("broken sc1").

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 lib/libhvcall/libhvcall.h |  4 ++++
 lib/libhvcall/brokensc1.c |  2 +-
 board-qemu/slof/rtas.fs   | 44 +++++++++++++--------------------------
 lib/libhvcall/hvcall.S    | 19 +++++++++++++++++
 lib/libhvcall/hvcall.code | 22 ++++++++++++++++++++
 lib/libhvcall/hvcall.in   |  2 ++
 6 files changed, 62 insertions(+), 31 deletions(-)

Comments

Thomas Huth July 16, 2019, 8:40 a.m. UTC | #1
Hi Alexey!

On 16/07/2019 07.11, Alexey Kardashevskiy wrote:
> We implement RTAS as a simple binary blob which calls directly into QEMU
> via a custom hcall. So far we were relying on QEMU putting the RTAS blob
> to the guest memory with its location in linux,rtas-base/rtas-size.

Likely a left-over from the time when it was possible to start a Linux
kernel directly, without SLOF. But since this is not possible anymore, I
think your patch is a nice clean-up, indeed.

[...]
> diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs
> index 092f5b1131f0..1b791b538d92 100644
> --- a/board-qemu/slof/rtas.fs
> +++ b/board-qemu/slof/rtas.fs
> @@ -40,38 +40,10 @@ rtas-cb /rtas-control-block erase
>  0 VALUE rtas-entry
>  0 VALUE rtas-node
>  
> -\ Locate qemu RTAS, remove the linux,... properties we really don't
> -\ want them to stick around
> -
>  372 cp
>  
>  : find-qemu-rtas ( -- )
>      " /rtas" find-device get-node to rtas-node
> -
> -    " linux,rtas-base" rtas-node get-package-property IF
> -         device-end EXIT THEN
> -    drop l@ to rtas-base
> -    " linux,rtas-base" delete-property
> -
> -    " rtas-size" rtas-node get-package-property IF
> -         device-end EXIT THEN
> -    drop l@ to rtas-size
> -
> -    " linux,rtas-entry" rtas-node get-package-property IF
> -        rtas-base to rtas-entry
> -    ELSE
> -        drop l@ to rtas-entry
> -        " linux,rtas-entry" delete-property
> -    THEN
> -
> -\    ." RTAS found, base=" rtas-base . ."  size=" rtas-size . cr
> -
> -    \ Patch the RTAS blob with our sc1 patcher if necessary
> -    0
> -    rtas-base
> -    dup rtas-size +
> -    check-and-patch-sc1
> -
>      device-end
>  ;
>  find-qemu-rtas

I think I'd rather simplify that whole function now to:

s" /rtas" find-node to rtas-node

(no need to put this code into a function, and no need for device-end
anymore)

> @@ -176,6 +148,14 @@ rtas-node set-node
>  : open true ;
>  : close ;
>  
> +: store-hv-rtas-size (  )
> +    hv-rtas-size
> +    dup to rtas-size
> +    encode-int s" rtas-size" s" /rtas" find-node set-property
> +;
> +
> +store-hv-rtas-size
> +
>  : store-rtas-loc ( adr )
>      s" /rtas" find-node >r
>      encode-int s" slof,rtas-base" r@ set-property
> @@ -184,8 +164,12 @@ rtas-node set-node
>  
>  : instantiate-rtas ( adr -- entry )
>      dup store-rtas-loc
> -    dup rtas-base swap rtas-size move
> -    rtas-entry rtas-base - +
> +    dup to rtas-base
> +    dup to rtas-entry
> +    s" /rtas" find-node >r
> +    dup encode-int s" linux,rtas-base" r@ set-property
> +    dup encode-int s" linux,rtas-entry" r> set-property

Why do you create the "linux,..." properties here? SLOF should normally
not create anything with the "linux," prefix.

> +    dup hv-instantiate-rtas
>  ;
>  
>  device-end
> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S
> index 92cf22e4c22c..19163f619be4 100644
> --- a/lib/libhvcall/hvcall.S
> +++ b/lib/libhvcall/hvcall.S
> @@ -127,6 +127,25 @@ ENTRY(hv_cas)
>  	HVCALL
>  	blr
>  
> +/* This is the actual RTAS blob copied to the OS at instantiate-rtas */
> +ENTRY(hv_rtas)
> +	mr	r4,r3
> +	lis	r3,KVMPPC_H_RTAS@h
> +	ori	r3,r3,KVMPPC_H_RTAS@l
> +	HVCALL
> +	blr
> +	.globl hv_rtas_end
> +hv_rtas_end = .;

you could calculate the size here instead already, a la:

hv_rtas_code_size = . - hv_rtas;

...

> +ENTRY(hv_rtas_broken_sc1)
> +	mr	r4,r3
> +	lis	r3,KVMPPC_H_RTAS@h
> +	ori	r3,r3,KVMPPC_H_RTAS@l
> +	.long	0x7c000268
> +	blr
> +	.globl hv_rtas_broken_sc1_end
> +hv_rtas_broken_sc1_end = .;

... and:

hv_rtas_broken_sc1_size = . - hv_rtas_broken_sc1;

...

>  	.section ".bss"
>  inbuf:	.space	16
>  inlen:	.space	4
> diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
> index 5918c901252e..1e47e55a5cc2 100644
> --- a/lib/libhvcall/hvcall.code
> +++ b/lib/libhvcall/hvcall.code
> @@ -128,3 +128,25 @@ PRIM(hv_X2d_update_X2d_dt)
>  	unsigned long dt = TOS.u;
>  	TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt);
>  MIRP
> +
> +PRIM(hv_X2d_instantiate_X2d_rtas)
> +	unsigned long start = TOS.u; POP;
> +    if (check_broken_sc1()) {
> +        memcpy((void *) start, &hv_rtas_broken_sc1,
> +                (unsigned long) &hv_rtas_broken_sc1_end -
> +                (unsigned long) &hv_rtas_broken_sc1);
> +    } else {
> +        memcpy((void *) start, &hv_rtas,
> +                (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas);
> +    }

... then you can use the size here directly and don't have to deal with
the ugly casting to (unsigned long) all over the place.

> +MIRP
> +
> +PRIM(hv_X2d_rtas_X2d_size)
> +    PUSH;
> +    if (check_broken_sc1()) {
> +        TOS.u = (unsigned long) &hv_rtas_broken_sc1_end -
> +                (unsigned long) &hv_rtas_broken_sc1;
> +    } else {
> +        TOS.u = (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas;
> +    }
> +MIRP

Please indent C code with tabs, not with spaces.

> diff --git a/lib/libhvcall/hvcall.in b/lib/libhvcall/hvcall.in
> index 9193162dd8ce..4165c6bcab58 100644
> --- a/lib/libhvcall/hvcall.in
> +++ b/lib/libhvcall/hvcall.in
> @@ -18,6 +18,8 @@ cod(hv-free-crq)
>  cod(hv-send-crq)
>  cod(hv-put-tce)
>  cod(check-and-patch-sc1)
> +cod(hv-instantiate-rtas)
> +cod(hv-rtas-size)
>  
>  cod(RB@)
>  cod(RB!)
> 

 Thomas
Alexey Kardashevskiy July 16, 2019, 10:21 a.m. UTC | #2
On 16/07/2019 18:40, Thomas Huth wrote:
>   Hi Alexey!
> 
> On 16/07/2019 07.11, Alexey Kardashevskiy wrote:
>> We implement RTAS as a simple binary blob which calls directly into QEMU
>> via a custom hcall. So far we were relying on QEMU putting the RTAS blob
>> to the guest memory with its location in linux,rtas-base/rtas-size.
> 
> Likely a left-over from the time when it was possible to start a Linux
> kernel directly, without SLOF. But since this is not possible anymore, I
> think your patch is a nice clean-up, indeed.


With a small patch to call rtas directly via H_RTAS from linux, it is 
possible :)


> [...]
>> diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs
>> index 092f5b1131f0..1b791b538d92 100644
>> --- a/board-qemu/slof/rtas.fs
>> +++ b/board-qemu/slof/rtas.fs
>> @@ -40,38 +40,10 @@ rtas-cb /rtas-control-block erase
>>   0 VALUE rtas-entry
>>   0 VALUE rtas-node
>>   
>> -\ Locate qemu RTAS, remove the linux,... properties we really don't
>> -\ want them to stick around
>> -
>>   372 cp
>>   
>>   : find-qemu-rtas ( -- )
>>       " /rtas" find-device get-node to rtas-node
>> -
>> -    " linux,rtas-base" rtas-node get-package-property IF
>> -         device-end EXIT THEN
>> -    drop l@ to rtas-base
>> -    " linux,rtas-base" delete-property
>> -
>> -    " rtas-size" rtas-node get-package-property IF
>> -         device-end EXIT THEN
>> -    drop l@ to rtas-size
>> -
>> -    " linux,rtas-entry" rtas-node get-package-property IF
>> -        rtas-base to rtas-entry
>> -    ELSE
>> -        drop l@ to rtas-entry
>> -        " linux,rtas-entry" delete-property
>> -    THEN
>> -
>> -\    ." RTAS found, base=" rtas-base . ."  size=" rtas-size . cr
>> -
>> -    \ Patch the RTAS blob with our sc1 patcher if necessary
>> -    0
>> -    rtas-base
>> -    dup rtas-size +
>> -    check-and-patch-sc1
>> -
>>       device-end
>>   ;
>>   find-qemu-rtas
> 
> I think I'd rather simplify that whole function now to:
> 
> s" /rtas" find-node to rtas-node
> 
> (no need to put this code into a function, and no need for device-end
> anymore)
> 
>> @@ -176,6 +148,14 @@ rtas-node set-node
>>   : open true ;
>>   : close ;
>>   
>> +: store-hv-rtas-size (  )
>> +    hv-rtas-size
>> +    dup to rtas-size
>> +    encode-int s" rtas-size" s" /rtas" find-node set-property
>> +;
>> +
>> +store-hv-rtas-size
>> +
>>   : store-rtas-loc ( adr )
>>       s" /rtas" find-node >r
>>       encode-int s" slof,rtas-base" r@ set-property
>> @@ -184,8 +164,12 @@ rtas-node set-node
>>   
>>   : instantiate-rtas ( adr -- entry )
>>       dup store-rtas-loc
>> -    dup rtas-base swap rtas-size move
>> -    rtas-entry rtas-base - +
>> +    dup to rtas-base
>> +    dup to rtas-entry
>> +    s" /rtas" find-node >r
>> +    dup encode-int s" linux,rtas-base" r@ set-property
>> +    dup encode-int s" linux,rtas-entry" r> set-property
> 
> Why do you create the "linux,..." properties here? SLOF should normally
> not create anything with the "linux," prefix.


Well, I am removing those from qemu, that's why but I do not need those 
anyway as linux creates them anyway.


> 
>> +    dup hv-instantiate-rtas
>>   ;
>>   
>>   device-end
>> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S
>> index 92cf22e4c22c..19163f619be4 100644
>> --- a/lib/libhvcall/hvcall.S
>> +++ b/lib/libhvcall/hvcall.S
>> @@ -127,6 +127,25 @@ ENTRY(hv_cas)
>>   	HVCALL
>>   	blr
>>   
>> +/* This is the actual RTAS blob copied to the OS at instantiate-rtas */
>> +ENTRY(hv_rtas)
>> +	mr	r4,r3
>> +	lis	r3,KVMPPC_H_RTAS@h
>> +	ori	r3,r3,KVMPPC_H_RTAS@l
>> +	HVCALL
>> +	blr
>> +	.globl hv_rtas_end
>> +hv_rtas_end = .;
> 
> you could calculate the size here instead already, a la:
> 
> hv_rtas_code_size = . - hv_rtas;
> 
> ...
> 
>> +ENTRY(hv_rtas_broken_sc1)
>> +	mr	r4,r3
>> +	lis	r3,KVMPPC_H_RTAS@h
>> +	ori	r3,r3,KVMPPC_H_RTAS@l
>> +	.long	0x7c000268
>> +	blr
>> +	.globl hv_rtas_broken_sc1_end
>> +hv_rtas_broken_sc1_end = .;
> 
> ... and:
> 
> hv_rtas_broken_sc1_size = . - hv_rtas_broken_sc1;
> 
> ...
> 
>>   	.section ".bss"
>>   inbuf:	.space	16
>>   inlen:	.space	4
>> diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
>> index 5918c901252e..1e47e55a5cc2 100644
>> --- a/lib/libhvcall/hvcall.code
>> +++ b/lib/libhvcall/hvcall.code
>> @@ -128,3 +128,25 @@ PRIM(hv_X2d_update_X2d_dt)
>>   	unsigned long dt = TOS.u;
>>   	TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt);
>>   MIRP
>> +
>> +PRIM(hv_X2d_instantiate_X2d_rtas)
>> +	unsigned long start = TOS.u; POP;
>> +    if (check_broken_sc1()) {
>> +        memcpy((void *) start, &hv_rtas_broken_sc1,
>> +                (unsigned long) &hv_rtas_broken_sc1_end -
>> +                (unsigned long) &hv_rtas_broken_sc1);
>> +    } else {
>> +        memcpy((void *) start, &hv_rtas,
>> +                (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas);
>> +    }
> 
> ... then you can use the size here directly and don't have to deal with
> the ugly casting to (unsigned long) all over the place.


If I do as you say, I'll have to take address of hv_rtas_size as 
hv_rtas_size is not a variable but address and for some reason it is 
0x6faf0014 instead of 0x14. Although it seems to be correct in objdump:

[fstn1-p1 slof]$ objdump -t ./board-qemu/slof/paflof | grep hv_rtas_size
0000000000000014 g       *ABS*  0000000000000000 hv_rtas_size

This produces nice "extern unsigned int hv_rtas_size" with no casts in 
those PRIM(xxx):

ENTRY(hv_rtas)
    mr. r4,r3
    lis r3,KVMPPC_H_RTAS@h
    ori r3,r3,KVMPPC_H_RTAS@l
    HVCALL
    blr
hv_rtas_end = .;
.data
   .globl hv_rtas_size
hv_rtas_size:
    .long hv_rtas_end - hv_rtas;

I am missing here something (did not touch assembly for years), what is 
it? Thanks,



> 
>> +MIRP
>> +
>> +PRIM(hv_X2d_rtas_X2d_size)
>> +    PUSH;
>> +    if (check_broken_sc1()) {
>> +        TOS.u = (unsigned long) &hv_rtas_broken_sc1_end -
>> +                (unsigned long) &hv_rtas_broken_sc1;
>> +    } else {
>> +        TOS.u = (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas;
>> +    }
>> +MIRP
> 
> Please indent C code with tabs, not with spaces.
> 
>> diff --git a/lib/libhvcall/hvcall.in b/lib/libhvcall/hvcall.in
>> index 9193162dd8ce..4165c6bcab58 100644
>> --- a/lib/libhvcall/hvcall.in
>> +++ b/lib/libhvcall/hvcall.in
>> @@ -18,6 +18,8 @@ cod(hv-free-crq)
>>   cod(hv-send-crq)
>>   cod(hv-put-tce)
>>   cod(check-and-patch-sc1)
>> +cod(hv-instantiate-rtas)
>> +cod(hv-rtas-size)
>>   
>>   cod(RB@)
>>   cod(RB!)
Thomas Huth July 16, 2019, 10:58 a.m. UTC | #3
On 16/07/2019 12.21, Alexey Kardashevskiy wrote:
> 
> On 16/07/2019 18:40, Thomas Huth wrote:
>>
>> On 16/07/2019 07.11, Alexey Kardashevskiy wrote:
[...]
>>> diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs
>>> index 092f5b1131f0..1b791b538d92 100644
>>> --- a/board-qemu/slof/rtas.fs
>>> +++ b/board-qemu/slof/rtas.fs
>>> @@ -40,38 +40,10 @@ rtas-cb /rtas-control-block erase
>>>   0 VALUE rtas-entry
>>>   0 VALUE rtas-node
>>>   -\ Locate qemu RTAS, remove the linux,... properties we really don't
>>> -\ want them to stick around
>>> -
>>>   372 cp
>>>     : find-qemu-rtas ( -- )
>>>       " /rtas" find-device get-node to rtas-node
>>> -
>>> -    " linux,rtas-base" rtas-node get-package-property IF
>>> -         device-end EXIT THEN
>>> -    drop l@ to rtas-base
>>> -    " linux,rtas-base" delete-property
>>> -
>>> -    " rtas-size" rtas-node get-package-property IF
>>> -         device-end EXIT THEN
>>> -    drop l@ to rtas-size
>>> -
>>> -    " linux,rtas-entry" rtas-node get-package-property IF
>>> -        rtas-base to rtas-entry
>>> -    ELSE
>>> -        drop l@ to rtas-entry
>>> -        " linux,rtas-entry" delete-property
>>> -    THEN
>>> -
>>> -\    ." RTAS found, base=" rtas-base . ."  size=" rtas-size . cr
>>> -
>>> -    \ Patch the RTAS blob with our sc1 patcher if necessary
>>> -    0
>>> -    rtas-base
>>> -    dup rtas-size +
>>> -    check-and-patch-sc1
>>> -
>>>       device-end
>>>   ;
>>>   find-qemu-rtas
>>
>> I think I'd rather simplify that whole function now to:
>>
>> s" /rtas" find-node to rtas-node
>>
>> (no need to put this code into a function, and no need for device-end
>> anymore)
>>
>>> @@ -176,6 +148,14 @@ rtas-node set-node
>>>   : open true ;
>>>   : close ;
>>>   +: store-hv-rtas-size (  )
>>> +    hv-rtas-size
>>> +    dup to rtas-size
>>> +    encode-int s" rtas-size" s" /rtas" find-node set-property
>>> +;
>>> +
>>> +store-hv-rtas-size
>>> +
>>>   : store-rtas-loc ( adr )
>>>       s" /rtas" find-node >r
>>>       encode-int s" slof,rtas-base" r@ set-property
>>> @@ -184,8 +164,12 @@ rtas-node set-node
>>>     : instantiate-rtas ( adr -- entry )
>>>       dup store-rtas-loc
>>> -    dup rtas-base swap rtas-size move
>>> -    rtas-entry rtas-base - +
>>> +    dup to rtas-base
>>> +    dup to rtas-entry
>>> +    s" /rtas" find-node >r
>>> +    dup encode-int s" linux,rtas-base" r@ set-property
>>> +    dup encode-int s" linux,rtas-entry" r> set-property
>>
>> Why do you create the "linux,..." properties here? SLOF should normally
>> not create anything with the "linux," prefix.
> 
> 
> Well, I am removing those from qemu, that's why but I do not need those
> anyway as linux creates them anyway.

The old code in find-qemu-rtas is deleting them explicitely, so yes,
they really should not be available in the device tree that SLOF passes
to the OS.

> 
>>
>>> +    dup hv-instantiate-rtas
>>>   ;
>>>     device-end
>>> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S
>>> index 92cf22e4c22c..19163f619be4 100644
>>> --- a/lib/libhvcall/hvcall.S
>>> +++ b/lib/libhvcall/hvcall.S
>>> @@ -127,6 +127,25 @@ ENTRY(hv_cas)
>>>       HVCALL
>>>       blr
>>>   +/* This is the actual RTAS blob copied to the OS at
>>> instantiate-rtas */
>>> +ENTRY(hv_rtas)
>>> +    mr    r4,r3
>>> +    lis    r3,KVMPPC_H_RTAS@h
>>> +    ori    r3,r3,KVMPPC_H_RTAS@l
>>> +    HVCALL
>>> +    blr
>>> +    .globl hv_rtas_end
>>> +hv_rtas_end = .;
>>
>> you could calculate the size here instead already, a la:
>>
>> hv_rtas_code_size = . - hv_rtas;
>>
>> ...
>>
>>> +ENTRY(hv_rtas_broken_sc1)
>>> +    mr    r4,r3
>>> +    lis    r3,KVMPPC_H_RTAS@h
>>> +    ori    r3,r3,KVMPPC_H_RTAS@l
>>> +    .long    0x7c000268
>>> +    blr
>>> +    .globl hv_rtas_broken_sc1_end
>>> +hv_rtas_broken_sc1_end = .;
>>
>> ... and:
>>
>> hv_rtas_broken_sc1_size = . - hv_rtas_broken_sc1;
>>
>> ...
>>
>>>       .section ".bss"
>>>   inbuf:    .space    16
>>>   inlen:    .space    4
>>> diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
>>> index 5918c901252e..1e47e55a5cc2 100644
>>> --- a/lib/libhvcall/hvcall.code
>>> +++ b/lib/libhvcall/hvcall.code
>>> @@ -128,3 +128,25 @@ PRIM(hv_X2d_update_X2d_dt)
>>>       unsigned long dt = TOS.u;
>>>       TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt);
>>>   MIRP
>>> +
>>> +PRIM(hv_X2d_instantiate_X2d_rtas)
>>> +    unsigned long start = TOS.u; POP;
>>> +    if (check_broken_sc1()) {
>>> +        memcpy((void *) start, &hv_rtas_broken_sc1,
>>> +                (unsigned long) &hv_rtas_broken_sc1_end -
>>> +                (unsigned long) &hv_rtas_broken_sc1);
>>> +    } else {
>>> +        memcpy((void *) start, &hv_rtas,
>>> +                (unsigned long) &hv_rtas_end - (unsigned long)
>>> &hv_rtas);
>>> +    }
>>
>> ... then you can use the size here directly and don't have to deal with
>> the ugly casting to (unsigned long) all over the place.
> 
> 
> If I do as you say, I'll have to take address of hv_rtas_size as
> hv_rtas_size is not a variable but address and for some reason it is
> 0x6faf0014 instead of 0x14. Although it seems to be correct in objdump:

Sounds like it got relocated when the binary is loaded :-/

> This produces nice "extern unsigned int hv_rtas_size" with no casts in
> those PRIM(xxx):
> 
> ENTRY(hv_rtas)
>    mr. r4,r3
>    lis r3,KVMPPC_H_RTAS@h
>    ori r3,r3,KVMPPC_H_RTAS@l
>    HVCALL
>    blr
> hv_rtas_end = .;
> .data
>   .globl hv_rtas_size
> hv_rtas_size:
>    .long hv_rtas_end - hv_rtas;

Does it also work if you omit ".data" and use ".long . - hv_rtas" ?

> I am missing here something (did not touch assembly for years), what is
> it? Thanks,

Yeah, my memories are also a little bit rusty already ... but I think
you're on the right track. Or simply ignore my suggestion and continue
with your original code, I don't mind too much.

 Thomas
Alexey Kardashevskiy July 17, 2019, 4:46 a.m. UTC | #4
On 16/07/2019 20:58, Thomas Huth wrote:
> On 16/07/2019 12.21, Alexey Kardashevskiy wrote:
>>
>> On 16/07/2019 18:40, Thomas Huth wrote:
>>>
>>> On 16/07/2019 07.11, Alexey Kardashevskiy wrote:
> [...]
>>>> diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs
>>>> index 092f5b1131f0..1b791b538d92 100644
>>>> --- a/board-qemu/slof/rtas.fs
>>>> +++ b/board-qemu/slof/rtas.fs
>>>> @@ -40,38 +40,10 @@ rtas-cb /rtas-control-block erase
>>>>    0 VALUE rtas-entry
>>>>    0 VALUE rtas-node
>>>>    -\ Locate qemu RTAS, remove the linux,... properties we really don't
>>>> -\ want them to stick around
>>>> -
>>>>    372 cp
>>>>      : find-qemu-rtas ( -- )
>>>>        " /rtas" find-device get-node to rtas-node
>>>> -
>>>> -    " linux,rtas-base" rtas-node get-package-property IF
>>>> -         device-end EXIT THEN
>>>> -    drop l@ to rtas-base
>>>> -    " linux,rtas-base" delete-property
>>>> -
>>>> -    " rtas-size" rtas-node get-package-property IF
>>>> -         device-end EXIT THEN
>>>> -    drop l@ to rtas-size
>>>> -
>>>> -    " linux,rtas-entry" rtas-node get-package-property IF
>>>> -        rtas-base to rtas-entry
>>>> -    ELSE
>>>> -        drop l@ to rtas-entry
>>>> -        " linux,rtas-entry" delete-property
>>>> -    THEN
>>>> -
>>>> -\    ." RTAS found, base=" rtas-base . ."  size=" rtas-size . cr
>>>> -
>>>> -    \ Patch the RTAS blob with our sc1 patcher if necessary
>>>> -    0
>>>> -    rtas-base
>>>> -    dup rtas-size +
>>>> -    check-and-patch-sc1
>>>> -
>>>>        device-end
>>>>    ;
>>>>    find-qemu-rtas
>>>
>>> I think I'd rather simplify that whole function now to:
>>>
>>> s" /rtas" find-node to rtas-node
>>>
>>> (no need to put this code into a function, and no need for device-end
>>> anymore)
>>>
>>>> @@ -176,6 +148,14 @@ rtas-node set-node
>>>>    : open true ;
>>>>    : close ;
>>>>    +: store-hv-rtas-size (  )
>>>> +    hv-rtas-size
>>>> +    dup to rtas-size
>>>> +    encode-int s" rtas-size" s" /rtas" find-node set-property
>>>> +;
>>>> +
>>>> +store-hv-rtas-size
>>>> +
>>>>    : store-rtas-loc ( adr )
>>>>        s" /rtas" find-node >r
>>>>        encode-int s" slof,rtas-base" r@ set-property
>>>> @@ -184,8 +164,12 @@ rtas-node set-node
>>>>      : instantiate-rtas ( adr -- entry )
>>>>        dup store-rtas-loc
>>>> -    dup rtas-base swap rtas-size move
>>>> -    rtas-entry rtas-base - +
>>>> +    dup to rtas-base
>>>> +    dup to rtas-entry
>>>> +    s" /rtas" find-node >r
>>>> +    dup encode-int s" linux,rtas-base" r@ set-property
>>>> +    dup encode-int s" linux,rtas-entry" r> set-property
>>>
>>> Why do you create the "linux,..." properties here? SLOF should normally
>>> not create anything with the "linux," prefix.
>>
>>
>> Well, I am removing those from qemu, that's why but I do not need those
>> anyway as linux creates them anyway.
> 
> The old code in find-qemu-rtas is deleting them explicitely, so yes,
> they really should not be available in the device tree that SLOF passes
> to the OS.


Sure. Forth is a write only language so when I "read" it first - I 
thought it creates property rather than deletes them :)


>>
>>>
>>>> +    dup hv-instantiate-rtas
>>>>    ;
>>>>      device-end
>>>> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S
>>>> index 92cf22e4c22c..19163f619be4 100644
>>>> --- a/lib/libhvcall/hvcall.S
>>>> +++ b/lib/libhvcall/hvcall.S
>>>> @@ -127,6 +127,25 @@ ENTRY(hv_cas)
>>>>        HVCALL
>>>>        blr
>>>>    +/* This is the actual RTAS blob copied to the OS at
>>>> instantiate-rtas */
>>>> +ENTRY(hv_rtas)
>>>> +    mr    r4,r3
>>>> +    lis    r3,KVMPPC_H_RTAS@h
>>>> +    ori    r3,r3,KVMPPC_H_RTAS@l
>>>> +    HVCALL
>>>> +    blr
>>>> +    .globl hv_rtas_end
>>>> +hv_rtas_end = .;
>>>
>>> you could calculate the size here instead already, a la:
>>>
>>> hv_rtas_code_size = . - hv_rtas;
>>>
>>> ...
>>>
>>>> +ENTRY(hv_rtas_broken_sc1)
>>>> +    mr    r4,r3
>>>> +    lis    r3,KVMPPC_H_RTAS@h
>>>> +    ori    r3,r3,KVMPPC_H_RTAS@l
>>>> +    .long    0x7c000268
>>>> +    blr
>>>> +    .globl hv_rtas_broken_sc1_end
>>>> +hv_rtas_broken_sc1_end = .;
>>>
>>> ... and:
>>>
>>> hv_rtas_broken_sc1_size = . - hv_rtas_broken_sc1;
>>>
>>> ...
>>>
>>>>        .section ".bss"
>>>>    inbuf:    .space    16
>>>>    inlen:    .space    4
>>>> diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
>>>> index 5918c901252e..1e47e55a5cc2 100644
>>>> --- a/lib/libhvcall/hvcall.code
>>>> +++ b/lib/libhvcall/hvcall.code
>>>> @@ -128,3 +128,25 @@ PRIM(hv_X2d_update_X2d_dt)
>>>>        unsigned long dt = TOS.u;
>>>>        TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt);
>>>>    MIRP
>>>> +
>>>> +PRIM(hv_X2d_instantiate_X2d_rtas)
>>>> +    unsigned long start = TOS.u; POP;
>>>> +    if (check_broken_sc1()) {
>>>> +        memcpy((void *) start, &hv_rtas_broken_sc1,
>>>> +                (unsigned long) &hv_rtas_broken_sc1_end -
>>>> +                (unsigned long) &hv_rtas_broken_sc1);
>>>> +    } else {
>>>> +        memcpy((void *) start, &hv_rtas,
>>>> +                (unsigned long) &hv_rtas_end - (unsigned long)
>>>> &hv_rtas);
>>>> +    }
>>>
>>> ... then you can use the size here directly and don't have to deal with
>>> the ugly casting to (unsigned long) all over the place.
>>
>>
>> If I do as you say, I'll have to take address of hv_rtas_size as
>> hv_rtas_size is not a variable but address and for some reason it is
>> 0x6faf0014 instead of 0x14. Although it seems to be correct in objdump:
> 
> Sounds like it got relocated when the binary is loaded :-/

It is relocation - stage1 moves paflof higher, the offset is exactly 
0x6faf0000. hv_rtas_size is in TOC (R_PPC64_TOC16_LO/R_PPC64_TOC16_HA 
but not R_PPC64_TOC16_HI) so this does not get relocated (because SLOF 
ignores these types anyway) but even if it did - it is still 16bit so 
the actual address of hv_rtas_size calculates against the current pc so 
the upper 16bits are not what I expect. Even though hv_rtas_end has 
"ABS" bit set, I still need a way to move that @hv_rtas_size away from TOC.

> 
>> This produces nice "extern unsigned int hv_rtas_size" with no casts in
>> those PRIM(xxx):
>>
>> ENTRY(hv_rtas)
>>     mr. r4,r3
>>     lis r3,KVMPPC_H_RTAS@h
>>     ori r3,r3,KVMPPC_H_RTAS@l
>>     HVCALL
>>     blr
>> hv_rtas_end = .;
>> .data
>>    .globl hv_rtas_size
>> hv_rtas_size:
>>     .long hv_rtas_end - hv_rtas;
> 
> Does it also work if you omit ".data" and use ".long . - hv_rtas" ?


It sure does (posted v2) but this creates a 4 byte variable and I was 
hoping to avoid that and make ld do the calculation. Ah whatever. Thanks,


>> I am missing here something (did not touch assembly for years), what is
>> it? Thanks,
> 
> Yeah, my memories are also a little bit rusty already ... but I think
> you're on the right track. Or simply ignore my suggestion and continue
> with your original code, I don't mind too much.
> 
>   Thomas
>

Patch
diff mbox series

diff --git a/lib/libhvcall/libhvcall.h b/lib/libhvcall/libhvcall.h
index caa4d6d3f17f..bd402be3fa48 100644
--- a/lib/libhvcall/libhvcall.h
+++ b/lib/libhvcall/libhvcall.h
@@ -97,11 +97,15 @@  extern unsigned long hv_logical_ci_store(unsigned long size, unsigned long addr,
 extern unsigned long hv_logical_memop(unsigned long dst, unsigned long src,
 				      unsigned long esize, unsigned long count,
 				      unsigned long op);
+extern int check_broken_sc1(void);
 extern int patch_broken_sc1(void *start, void *end, uint32_t *test_ins);
 
 extern unsigned long hv_cas(unsigned long vec, unsigned long buf,
 			unsigned long size);
 
+extern void *hv_rtas, *hv_rtas_end;
+extern void *hv_rtas_broken_sc1, *hv_rtas_broken_sc1_end;
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LIBHVCALL_H__ */
diff --git a/lib/libhvcall/brokensc1.c b/lib/libhvcall/brokensc1.c
index d97ae77558c7..f01157029b8a 100644
--- a/lib/libhvcall/brokensc1.c
+++ b/lib/libhvcall/brokensc1.c
@@ -36,7 +36,7 @@  static unsigned long hcall(uint32_t inst, unsigned long arg0, unsigned long arg1
 	return r3;
 }
 
-static int check_broken_sc1(void)
+int check_broken_sc1(void)
 {
 	long r;
 
diff --git a/board-qemu/slof/rtas.fs b/board-qemu/slof/rtas.fs
index 092f5b1131f0..1b791b538d92 100644
--- a/board-qemu/slof/rtas.fs
+++ b/board-qemu/slof/rtas.fs
@@ -40,38 +40,10 @@  rtas-cb /rtas-control-block erase
 0 VALUE rtas-entry
 0 VALUE rtas-node
 
-\ Locate qemu RTAS, remove the linux,... properties we really don't
-\ want them to stick around
-
 372 cp
 
 : find-qemu-rtas ( -- )
     " /rtas" find-device get-node to rtas-node
-
-    " linux,rtas-base" rtas-node get-package-property IF
-         device-end EXIT THEN
-    drop l@ to rtas-base
-    " linux,rtas-base" delete-property
-
-    " rtas-size" rtas-node get-package-property IF
-         device-end EXIT THEN
-    drop l@ to rtas-size
-
-    " linux,rtas-entry" rtas-node get-package-property IF
-        rtas-base to rtas-entry
-    ELSE
-        drop l@ to rtas-entry
-        " linux,rtas-entry" delete-property
-    THEN
-
-\    ." RTAS found, base=" rtas-base . ."  size=" rtas-size . cr
-
-    \ Patch the RTAS blob with our sc1 patcher if necessary
-    0
-    rtas-base
-    dup rtas-size +
-    check-and-patch-sc1
-
     device-end
 ;
 find-qemu-rtas
@@ -176,6 +148,14 @@  rtas-node set-node
 : open true ;
 : close ;
 
+: store-hv-rtas-size (  )
+    hv-rtas-size
+    dup to rtas-size
+    encode-int s" rtas-size" s" /rtas" find-node set-property
+;
+
+store-hv-rtas-size
+
 : store-rtas-loc ( adr )
     s" /rtas" find-node >r
     encode-int s" slof,rtas-base" r@ set-property
@@ -184,8 +164,12 @@  rtas-node set-node
 
 : instantiate-rtas ( adr -- entry )
     dup store-rtas-loc
-    dup rtas-base swap rtas-size move
-    rtas-entry rtas-base - +
+    dup to rtas-base
+    dup to rtas-entry
+    s" /rtas" find-node >r
+    dup encode-int s" linux,rtas-base" r@ set-property
+    dup encode-int s" linux,rtas-entry" r> set-property
+    dup hv-instantiate-rtas
 ;
 
 device-end
diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S
index 92cf22e4c22c..19163f619be4 100644
--- a/lib/libhvcall/hvcall.S
+++ b/lib/libhvcall/hvcall.S
@@ -127,6 +127,25 @@  ENTRY(hv_cas)
 	HVCALL
 	blr
 
+/* This is the actual RTAS blob copied to the OS at instantiate-rtas */
+ENTRY(hv_rtas)
+	mr	r4,r3
+	lis	r3,KVMPPC_H_RTAS@h
+	ori	r3,r3,KVMPPC_H_RTAS@l
+	HVCALL
+	blr
+	.globl hv_rtas_end
+hv_rtas_end = .;
+
+ENTRY(hv_rtas_broken_sc1)
+	mr	r4,r3
+	lis	r3,KVMPPC_H_RTAS@h
+	ori	r3,r3,KVMPPC_H_RTAS@l
+	.long	0x7c000268
+	blr
+	.globl hv_rtas_broken_sc1_end
+hv_rtas_broken_sc1_end = .;
+
 	.section ".bss"
 inbuf:	.space	16
 inlen:	.space	4
diff --git a/lib/libhvcall/hvcall.code b/lib/libhvcall/hvcall.code
index 5918c901252e..1e47e55a5cc2 100644
--- a/lib/libhvcall/hvcall.code
+++ b/lib/libhvcall/hvcall.code
@@ -128,3 +128,25 @@  PRIM(hv_X2d_update_X2d_dt)
 	unsigned long dt = TOS.u;
 	TOS.u = hv_generic(KVMPPC_H_UPDATE_DT, dt);
 MIRP
+
+PRIM(hv_X2d_instantiate_X2d_rtas)
+	unsigned long start = TOS.u; POP;
+    if (check_broken_sc1()) {
+        memcpy((void *) start, &hv_rtas_broken_sc1,
+                (unsigned long) &hv_rtas_broken_sc1_end -
+                (unsigned long) &hv_rtas_broken_sc1);
+    } else {
+        memcpy((void *) start, &hv_rtas,
+                (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas);
+    }
+MIRP
+
+PRIM(hv_X2d_rtas_X2d_size)
+    PUSH;
+    if (check_broken_sc1()) {
+        TOS.u = (unsigned long) &hv_rtas_broken_sc1_end -
+                (unsigned long) &hv_rtas_broken_sc1;
+    } else {
+        TOS.u = (unsigned long) &hv_rtas_end - (unsigned long) &hv_rtas;
+    }
+MIRP
diff --git a/lib/libhvcall/hvcall.in b/lib/libhvcall/hvcall.in
index 9193162dd8ce..4165c6bcab58 100644
--- a/lib/libhvcall/hvcall.in
+++ b/lib/libhvcall/hvcall.in
@@ -18,6 +18,8 @@  cod(hv-free-crq)
 cod(hv-send-crq)
 cod(hv-put-tce)
 cod(check-and-patch-sc1)
+cod(hv-instantiate-rtas)
+cod(hv-rtas-size)
 
 cod(RB@)
 cod(RB!)