[SLOF] [PATCH slof] rtas: Integrate RTAS blob

[Thomas Huth]

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 at h
>>> +    ori    r3,r3,KVMPPC_H_RTAS at 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 at h
>>> +    ori    r3,r3,KVMPPC_H_RTAS at 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 at h
>    ori r3,r3,KVMPPC_H_RTAS at 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