On 13.02.20 19:02, Jason J. Herne wrote: > On 2/6/20 5:09 AM, Christian Borntraeger wrote: >> >> >> On 05.02.20 19:21, Jason J. Herne wrote: >>> This fixes vfio-ccw when booting non-Linux operating systems. Without this >>> struct being packed, a few extra bytes of low core memory get overwritten >>> when >>> we assign a value to memory address 0 in jump_to_IPL_2. This is enough to >>> cause some non-Linux OSes of fail when booting. >>> >>> The problem was introduced by: >>> 5c6f0d5f46a77d77 "pc-bios/s390x: Fix reset psw mask". >>> >>> The fix is to pack the struct thereby removing the 4 bytes of padding that >>> get >>> added at the end, likely to allow an array of these structs to naturally >>> align >>> on an 8-byte boundary. >>> >>> Fixes: 5c6f0d5f46a7 ("pc-bios/s390x: Fix reset psw mask") >>> CC: Janosch Frank <fran...@linux.ibm.com> >>> Signed-off-by: Jason J. Herne <jjhe...@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/jump2ipl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >>> index da13c43cc0..1e9eaa037f 100644 >>> --- a/pc-bios/s390-ccw/jump2ipl.c >>> +++ b/pc-bios/s390-ccw/jump2ipl.c >>> @@ -18,7 +18,7 @@ >>> typedef struct ResetInfo { >>> uint64_t ipl_psw; >>> uint32_t ipl_continue; >>> -} ResetInfo; >>> +} __attribute__((packed)) ResetInfo; >>> static ResetInfo save; >> >> Just looked into that. >> >> We do save the old content in "save" and restore the old memory content. >> >> 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 */ >> } >> >> void jump_to_IPL_code(uint64_t address) >> { >> /* store the subsystem information _after_ the bootmap was loaded */ >> write_subsystem_identification(); >> >> /* prevent unknown IPL types in the guest */ >> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { >> iplb.pbt = S390_IPL_TYPE_CCW; >> set_iplb(&iplb); >> } >> >> /* >> * The IPL PSW is at address 0. We also must not overwrite the >> * content of non-BIOS memory after we loaded the guest, so we >> * save the original content and restore it in jump_to_IPL_2. >> */ >> ResetInfo *current = 0; >> >> --->save = *current; >> >> >> >> does something like >> >> >> 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.