Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them

[Thomas Huth]

On 02/09/2020 12.30, Janosch Frank wrote:
> On 9/1/20 7:22 PM, Thomas Huth wrote:
>> On 31/08/2020 17.09, 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>
>>> ---
>>>  pc-bios/s390-ccw/jump2ipl.c |  4 +++
>>>  pc-bios/s390-ccw/netmain.c  |  3 ++
>>>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>>>  3 files changed, 52 insertions(+), 17 deletions(-)
>>
>> Patch looks basically fine to me, I just got some questions for my
>> understanding below...
>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index 5b8352d257..bb94ba7550 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -14,6 +14,7 @@
>>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>>  #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>>>  
>>> +extern uint64_t psw_save_io[], psw_save_ext[];
>>>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>>  
>>>  void write_reset_psw(uint64_t psw)
>>> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address)
>>>      /* Ensure the guest output starts fresh */
>>>      sclp_print("\n");
>>>  
>>> +    memcpy(&lowcore->io_new_psw, psw_save_io, 16);
>>> +    memcpy(&lowcore->external_new_psw, psw_save_ext, 16);
>>> +
>>>      /*
>>>       * HACK ALERT.
>>>       * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
>>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>>> index 056e93a818..74ef28fbc6 100644
>>> --- a/pc-bios/s390-ccw/netmain.c
>>> +++ b/pc-bios/s390-ccw/netmain.c
>>> @@ -32,6 +32,7 @@
>>>  #include <time.h>
>>>  #include <pxelinux.h>
>>>  
>>> +#include "s390-arch.h"
>>>  #include "s390-ccw.h"
>>>  #include "cio.h"
>>>  #include "virtio.h"
>>> @@ -43,6 +44,8 @@
>>>  extern char _start[];
>>>  void write_iplb_location(void) {}
>>>  
>>> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>>> +
>>>  #define KERNEL_ADDR             ((void *)0L)
>>>  #define KERNEL_MAX_SIZE         ((long)_start)
>>>  #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux 
>>> kernel */
>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>>> index ce519300a1..939aac3a7c 100644
>>> --- a/pc-bios/s390-ccw/start.S
>>> +++ b/pc-bios/s390-ccw/start.S
>>> @@ -34,7 +34,17 @@ remainder:
>>>     larl    %r2,memsetxc
>>>     ex      %r3,0(%r2)
>>>  done:
>>> -   j      main             /* And call C */
>>> +        /* prepare i/o call handler */
>>> +        larl  %r1, io_new_code
>>> +        larl  %r2, io_new_psw
>>> +        stg   %r1, 8(%r2)
>>> +        mvc   0x1f0(16),0(%r2)
>>> +        /* prepare external call handler */
>>> +        larl  %r1, external_new_code
>>> +        larl  %r2, external_new_psw
>>> +        stg   %r1, 8(%r2)
>>
>> Can't you specify the external_new_code and io_new_code in the
>> external_new_psw / io_new_psw directly? Or is our relocation code not
>> good enough for this?
> 
> Initially I had some problems with this. I just had another try and it
> seems to work well, but as the testing infrastructure doesn't really
> work, I can't vouch for that.

You could maybe dump the memory in both cases to see whether
external_new_psw and io_new_psw contain the same values before and after
the change?

>> In case you respin, could you maybe add some local #defines for 0x1f0
>> and 0x1b0 ?
> 
> At the top of this file?

Yes, please.

 Thomas