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

[Christian Borntraeger]

On 25.02.20 16:00, Christian Borntraeger wrote:
> 
> 
> On 25.02.20 13:58, Jason J. Herne wrote:
>> 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.
> 
> I think we should rewrite jump_to_IPL_2 is assembler as we cannot clear out 
> all registers
> with just inline assembly (we whould kill the stack and others that the 
> compiler might still want).
> 
> Do the register clearing in there and then use something like
> 
> static void jump_to_IPL_2(void)
> {
>     asm volatile(     ....clearing...
>                       "llgf 14,8\n"
>                       "br 14\n");
> }

maybe something like that instead.

    asm volatile(  ...clearing...
                    "llgf 14,%0\n"
                   "br 14\n"::"i" (__builtin_offsetof(ResetInfo, 
ipl_continue)));
 }