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

[Jason J. Herne]

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 (address@hidden)