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

[Christian Borntraeger]

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?

> 
> jump_to_IPL_2 for easy reference:
>     static void jump_to_IPL_2(void)
>     {
>         ResetInfo *current = 0;
> 
>         void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
>         *current = save;
>         ipl(); /* should not return */
>     }
> 
>