Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Thomas Huth]

On 23/11/2020 08.39, Christian Borntraeger wrote:
> On 20.11.20 17:01, Eric Farman wrote:
>> A Linux binary will have the string "S390EP" at address 0x10008,
>> which is important in getting the guest up off the ground. In the
>> case of a reboot (specifically chreipl going to a new device),
>> we should defer to the PSW at address zero for the new config,
>> which will re-write "S390EP" from the new image.
>>
>> Let's clear it out at this point so that a reipl to, say, a DASD
>> passthrough device drives the IPL path from scratch without disrupting
>> disrupting the order of operations for other boots.
>>
>> Rather than hardcoding the address of this magic (again), let's
>> define it somewhere so that the two users are visibly related.
> 
> 
> Hmmm, this might have side effects, e.g. if you do something like a kdump
> or kexec to a non-Linux binary that happens to have code at 0x10008, no?

Do these scenarios really go through the s390-ccw bios again, or do they
rather bypass the bios and jump directly into the new kernel?

> As far as I can tell, the problem should only happen for a ccw type IPL
> so why not

Not sure whether it really can only happen in these cases... for example,
would it also be possible to reboot from a Linux kernel into a
kvm-unit-test? ... these also do not have the S390EP magic, IIRC.

> [...]
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -178,6 +178,12 @@ static void boot_setup(void)
>>      memcpy(lpmsg + 10, loadparm_str, 8);
>>      sclp_print(lpmsg);
>>  
>> +    /*
>> +     * Clear out any potential S390EP magic (see jump_to_low_kernel()),
>> +     * so we don't taint our decision-making process during a reboot.
>> +     */
>> +    memset((char *)S390EP, 0, 6);
> 
> 
> move this into find_subch
> in here:
> ------------- snip ---------------
>             case CU_TYPE_DASD_3990:
>             case CU_TYPE_DASD_2107:
>                 return true;
> ------------- snip ---------------
> 

That would be is_dev_possibly_bootable() now? ... not sure whether this is
the best location... maybe put it better at the beginning of dasd_ipl() instead?

 Thomas