[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RFC v2 3/4] pc-bios: s390x: Save io and external new PSWs bef
From: |
Thomas Huth |
Subject: |
Re: [PATCH RFC v2 3/4] pc-bios: s390x: Save io and external new PSWs before overwriting them |
Date: |
Thu, 27 Aug 2020 18:16:55 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
On 27/08/2020 16.30, Janosch Frank wrote:
> On 8/27/20 2:52 PM, Thomas Huth wrote:
>> On 27/08/2020 11.31, Janosch Frank wrote:
>>> Currently we always overwrite the mentioned exception new PSWs before
>>> loading the enabled wait PSW. Let's save the PSW before overwriting
>>> and restore it right before starting the loaded kernel.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>
>>> Maybe we should rather statically allocate a lowcore so we don't dirty
>>> 0x0 at all.
>>>
>>> ---
>>> pc-bios/s390-ccw/jump2ipl.c | 3 ++
>>> pc-bios/s390-ccw/start.S | 62 +++++++++++++++++++++++++++----------
>>> 2 files changed, 48 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index 143d027bf7..a44f3ab5b3 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -13,12 +13,15 @@
>>> #define KERN_IMAGE_START 0x010000UL
>>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>>
>>> +extern uint64_t *psw_save_io, *psw_save_ext;
>>
>> I think that should be
>>
>> extern uint64_t psw_save_io[], psw_save_ext[];
>>
>> instead ... otherwise you'll end up with some funny bugs here, won't you?
>
> What kind of bugs are you expecting?
Well, "extern uint64_t var[];" and "extern uint64_t *var;" are two
different kind of things. One is an array, one is a pointer variable.
Looking at your assembler code, you obviously tried to declare an array
there, not a pointer variable.
Have a try with this test program:
#include <string.h>
extern unsigned long *var;
void main(void)
{
asm volatile (" nop ; nop ; nop "); /* marker */
memcpy((void *)0x1f0, var, 16);
asm volatile (" nop ; nop ; nop "); /* marker */
}
After compiling that with -O2, and disassembling the corresponding .o
file, I get this code between the nops:
c: c4 18 00 00 00 00 lgrl %r1,c <main+0xc>
e: R_390_PC32DBL var+0x2
12: e7 00 10 00 00 06 vl %v0,0(%r1)
18: e7 00 01 f0 00 0e vst %v0,496
The "lgrl %r1,var" is likely not what you wanted here.
If you now replace the "*var" with "var[]", you get this instead:
c: c0 10 00 00 00 00 larl %r1,c <main+0xc>
e: R_390_PC32DBL var+0x2
12: e7 00 10 00 00 06 vl %v0,0(%r1)
18: e7 00 01 f0 00 0e vst %v0,496
"larl" looks better now, doesn't it?
>>
>>> uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>>
>>> static void jump_to_IPL_2(void)
>>> {
>>> /* Restore reset PSW and io and external new PSWs */
>>
>> Ok, now the comment makes sense :-)
>>> *reset_psw = save_psw;
>>> + memcpy((void *)0x1f0, psw_save_io, 16);
>>> + memcpy((void *)0x1b0, psw_save_ext, 16);
>>
>> Could you use &lowcore->external_new_psw and &lowcore->io_new_psw
>> instead of the magic numbers?
>
> I can, but that means that I need to declare lowcore in netmain.c as
> well as including s390-arch.h
If that does not cause any other big hurdles, I think I'd prefer that
instead of using magic numbers.
Thanks,
Thomas