| Message ID | 20190716051131.65756-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtas: Integrate RTAS blob | expand |
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
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!)
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
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 >
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!)
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(-)