qemu-devel
[Top][All Lists]
Advanced

[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 07:58:07 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0

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.


--
-- Jason J. Herne (address@hidden)



reply via email to

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