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

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

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.

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 */
     }


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