Re: [PATCH] pc-bios/s390x: Pack ResetInfo struct

["Jason J. Herne" <jjherne@linux.ibm.com>]

On 2/25/20 6:13 AM, Christian Borntraeger wrote:
> 
> 
> On 25.02.20 11:23, Jason J. Herne wrote:
>> On 2/13/20 1:24 PM, Christian Borntraeger wrote:
>> ...
>>>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>>>> index da13c43cc0..8839226803 100644
>>>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>>>> @@ -18,6 +18,7 @@
>>>>>     typedef struct ResetInfo {
>>>>>         uint64_t ipl_psw;
>>>>>         uint32_t ipl_continue;
>>>>> +    uint32_t pad;
>>>>>     } ResetInfo;
>>>>>       static ResetInfo save;
>>>>>
>>>>>
>>>>> also work? If yes, both variants are valid. Either packed or explicit padding.
>>>>>
>>>>
>>>> I don't believe this will work. I think the problem is that we're overwriting too much memory when we cast address 0 as a ResetInfo and then overwrite it (*current = save). I think we need the struct to be sized at 12-bytes instead of 16.
>>>>
>>>
>>> The idea of the code is that we _save_ the original content from address 0 to save and _restore_ it before jumping into final code. I do not yet understand why this does not work.
>>>
>>
>> I've found the real problem here. Legacy operating systems that expect to start
>> in 32-bit addressing mode can fail if we leave junk in the high halves of our
>> 64-bit registers. This is because some instructions (LA for example) are
>> bi-modal and operate differently depending on the machine's current addressing
>> mode.
>>
>> In the case where we pack the struct, the compiler happens to use the mvc
>> instruction to load/store the current/save memory areas.
>>
>>        *current = save;
>>    1fc:    e3 10 b0 a8 00 04     lg    %r1,168(%r11)
>>    202:    c0 20 00 00 00 00     larl    %r2,202 <jump_to_IPL_2+0x32>
>>              204: R_390_PC32DBL    .bss+0x2
>>    208:    d2 0b 10 00 20 00     mvc    0(12,%r1),0(%r2)
>>
>> Everything works as expected here, our legacy OS boots without issue.
>> However, in the case where we've packed this struct the compiler optimizes the
>> code and uses lmg/stmg instead of mvc to copy the data:
>>
>>        *current = save;
>>    1fc:    e3 10 b0 a8 00 04     lg    %r1,168(%r11)
>>    202:    c0 20 00 00 00 00     larl    %r2,202 <jump_to_IPL_2+0x32>
>>              204: R_390_PC32DBL    .bss+0x2
>>    208:    eb 23 20 00 00 04     lmg    %r2,%r3,0(%r2)
>>    20e:    eb 23 10 00 00 24     stmg    %r2,%r3,0(%r1)
>>
>> Depending on the data being copied, the high halves of the registers may contain
>> non-zero values. Example:
>>
>>      r2             0x108000080000780        74309395999098752
>>      r3             0x601001800004368        432627142283510632
>>
>> So, by sheer luck of the generated assembler, the patch happens to "fix" the
>> problem.  A real fix might be to insert inline assembler that clears the high
>> halves of the registers before we call ipl() in jump_to_IPL_2(). Can we think of
>> a better way to do that than 15 LLGTR instructions? :) Let me know your
>> thoughts
> 
> Does sam31 before the ipl() work?
asm volatile ("sam31\n");

Inserting the above right before ipl(); does not change the outcome, the guest still fails.

This allows the guest to boot.

asm volatile ("llgtr %r2,%r2\n"
               "llgtr %r3,%r3\n");

My guess as to why sam31 does not work: The legacy OS is eventually doing a sam64 and the 
high halves of the registers are not subsequently cleared before use. I could be wrong 
about this though.


-- 
-- Jason J. Herne (jjherne@linux.ibm.com)