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

Wed Jul 17 14:46:32 AEST 2019

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

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

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
> 

-- 
Alexey