Re: [PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Thomas Huth <thuth@redhat.com>]

On 19/11/2020 22.11, Eric Farman wrote:
> 
> 
> On 11/19/20 3:20 PM, Thomas Huth wrote:
>> On 19/11/2020 17.57, Eric Farman wrote:
>>> Let's look at the Reset PSW first instead of the contents of memory.
>>> It might be leftover from an earlier system boot when processing
>>> a chreipl.
>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index fbae45b03c..67b4afb6a0 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
>>>     void jump_to_low_kernel(void)
>>>   {
>>> -    /*
>>> -     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>> from
>>> -     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>> Linux
>>> -     * kernel start address (when jumping to the PSW-at-zero address
>>> instead,
>>> -     * the kernel startup code fails when we booted from a network device).
>>> -     */
>>> -    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>> -        jump_to_IPL_code(KERN_IMAGE_START);
>>> -    }
>>> -
>>>       /* Trying to get PSW at zero address */
>>>       if (*((uint64_t *)0) & RESET_PSW_MASK) {
>>>           /*
>>> @@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
>>>           jump_to_IPL_code(0);
>>>       }
>>>   +    /*
>>> +     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>> from
>>> +     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>> Linux
>>> +     * kernel start address (when jumping to the PSW-at-zero address
>>> instead,
>>> +     * the kernel startup code fails when we booted from a network device).
>>> +     */
>>> +    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>> +        jump_to_IPL_code(KERN_IMAGE_START);
>>> +    }
>>
>> That feels a little bit dangerous ... I assume the order has been that way
>> for a reason, e.g. I think we had to jump to KERN_IMAGE_START for some older
>> versions of the Linux kernel since the startup code that was referenced by
>> the PSW at address zero was not working in KVM...
> 
> Makes sense.  It does seem like a precarious piece of code.
> 
>>
>> What do you think about this alternate idea instead: Clear the memory at
>> location 0x10008 at the very beginning of the main() function (or maybe in
>> boot_setup())? 
> 
> This seems to work too (I put it in boot_setup(), prior to call to
> store_iplb()).

Great, can you send the patch before your holidays next week? (If you don't
have enough time, that's ok, too, it's trivial enough, so I think I could
write such a patch, too)

 Thomas