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