[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jason J. Herne
Subject: Re: [PATCH] pc-bios/s390x: Pack ResetInfo struct
Date: Tue, 25 Feb 2020 05:23:56 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0

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

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

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)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]