Re: [PATCH] pc-bios: s390x: Only set lowcore iplb address on list-directed IPL

[Thomas Huth <thuth@redhat.com>]

On 19/08/2020 12.46, Janosch Frank wrote:
> On 8/19/20 11:45 AM, Cornelia Huck wrote:
>> On Wed, 19 Aug 2020 11:32:34 +0200
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> On 8/17/20 7:51 PM, Jason J. Herne wrote:
>>>> On 8/17/20 12:30 PM, Cornelia Huck wrote:  
>>>>> On Mon, 17 Aug 2020 10:17:34 -0400
>>>>> "Jason J. Herne" <jjherne@linux.ibm.com> wrote:
>>>>>  
>>>>>> The POP states that the IPLB location is only written to 0x14 for
>>>>>> list-directed IPL. Some operating systems expect 0x14 to not change on
>>>>>> boot and will fail IPL if it does change.
>>>>>>
>>>>>> Fixes: 9bfc04f9ef6802fff0  
>>>>>
>>>>> Should be
>>>>>
>>>>> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
>>>>>  
>>>>>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>>>>>> Reviewed-by: Janosch Frank <frankja@de.ibm.com>
>>>>>> ---
>>>>>>   pc-bios/s390-ccw/jump2ipl.c | 5 ++++-
>>>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>>>>> index 767012bf0c..5e3e13f4b0 100644
>>>>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>>>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>>>>> @@ -33,7 +33,10 @@ void jump_to_IPL_code(uint64_t address)
>>>>>>   {
>>>>>>       /* store the subsystem information _after_ the bootmap was loaded */
>>>>>>       write_subsystem_identification();
>>>>>> -    write_iplb_location();
>>>>>> +
>>>>>> +    if (iplb.pbt != S390_IPL_TYPE_CCW) {
>>>>>> +            write_iplb_location();
>>>>>> +    }  
>>>>>
>>>>> What happens for ipl types other than CCW and FCP? IOW, should that
>>>>> rather be a positive check for S390_IPL_TYPE_FCP?
>>>>>  
>>>>>>   
>>>>>>       /* prevent unknown IPL types in the guest */
>>>>>>       if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {  
>>>>>  
>>>>
>>>> Based on my (admittedly limited) understanding of the architecture and
>>>> code, I believe write_iplb_location() should be called at least for
>>>> S390_IPL_TYPE_FCP but I'm not 100% sure on S390_IPL_TYPE_QEMU_SCSI.
>>>> Perhaps Janosch has an idea?
>>>>
>>>> It was originally unconditional, and my new conditional excludes vfio
>>>> CCW which is definitely a step in the right direction, in any case :).  
>>>
>>> If I remember correctly the problem was that ZIPL used the IPLB lowcore
>>> ptr without checking how it was booted (CCW or FCP). That was fixed in
>>> mid of July by testing if diag308 gives back a config or not.
>>
>> So we have the problem that old zipl relies on the presence of a value
>> that must not be there if you follow the architecture? Nasty.
>>
>> (Is it really "must not change" vs "don't expect anything here"? Not
>> sure if I'm looking at the right part of the documentation.)
> 
> Well if the loaded program overwrites absolute 0x0, we shouldn't modify
> it if we are not explicitly allowed to, no?
> 
> We already talked about saving the exception new addresses and restoring
> them before jumping to the new kernel. I think we might need to go a
> step further and use a non zero prefix for the bios to avoid any changes
> to absolute 0x0.
> 
> However that wouldn't fix this dilemma.

Sorry, I'm just back from summer vacation ... so what's the conclusion
for Jason's patch here? Should it be included as-is now or do we rather
neeed another rework here instead?

 Thomas