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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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");
}