| Message ID | 20191203132813.2734-5-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: Increase architectural compliance | expand |
On Tue, 3 Dec 2019 14:33:25 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 03.12.19 14:28, Janosch Frank wrote: > > We need to set the short psw indication bit in the reset psw, as it is > > a short psw. > > > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > We should also add > commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next) > Author: Janosch Frank <frankja@linux.ibm.com> > AuthorDate: Fri Nov 29 09:20:23 2019 -0500 > Commit: Cornelia Huck <cohuck@redhat.com> > CommitDate: Mon Dec 2 09:58:57 2019 +0100 > > s390x: Properly fetch and test the short psw on diag308 subc 0/1 > > or whatever the final commit id will be. While this patch is not "broken" > it exposes the bug. Probably better to use "Exposed by (...)", then? We'll be fine as long as we have a bios rebuilt with this fix, and anybody using the official QEMU should get that. Pointing to the commit should be enough for any backporters. > > > > > --- > > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-)
On Tue, 3 Dec 2019 08:28:13 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > We need to set the short psw indication bit in the reset psw, as it is > a short psw. > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") s/fixes: 9629823290/Fixes: 962982329029/ > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index 266f1502b9..da13c43cc0 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -12,11 +12,11 @@ > #define KERN_IMAGE_START 0x010000UL > #define PSW_MASK_64 0x0000000100000000ULL > #define PSW_MASK_32 0x0000000080000000ULL > -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) > +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL > +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64) > > typedef struct ResetInfo { > - uint32_t ipl_mask; > - uint32_t ipl_addr; > + uint64_t ipl_psw; > uint32_t ipl_continue; > } ResetInfo; > > @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address) > ResetInfo *current = 0; > > save = *current; > - current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2; > + > + current->ipl_psw = (uint64_t) &jump_to_IPL_2; > + current->ipl_psw |= RESET_PSW_MASK; > current->ipl_continue = address & 0x7fffffff; > > debug_print_int("set IPL addr to", current->ipl_continue); > @@ -82,7 +84,7 @@ void jump_to_low_kernel(void) > } > > /* Trying to get PSW at zero address */ > - if (*((uint64_t *)0) & IPL_PSW_MASK) { > + if (*((uint64_t *)0) & RESET_PSW_MASK) { > jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff); > } > Looks sane to me, but would like an ack from bios maintainers. As this is independent of the other patches (which depend on a headers update), I can pick this and do a rebuild of the bios(es). Unless one of the bios maintainers wants to do that and send me a pull req :), but that seems like overkill.
On 03.12.19 14:28, Janosch Frank wrote: > We need to set the short psw indication bit in the reset psw, as it is > a short psw. > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index 266f1502b9..da13c43cc0 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -12,11 +12,11 @@ > #define KERN_IMAGE_START 0x010000UL > #define PSW_MASK_64 0x0000000100000000ULL > #define PSW_MASK_32 0x0000000080000000ULL > -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) > +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL > +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64) > > typedef struct ResetInfo { > - uint32_t ipl_mask; > - uint32_t ipl_addr; > + uint64_t ipl_psw; > uint32_t ipl_continue; > } ResetInfo; > > @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address) > ResetInfo *current = 0; > > save = *current; > - current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2; > + > + current->ipl_psw = (uint64_t) &jump_to_IPL_2; > + current->ipl_psw |= RESET_PSW_MASK; > current->ipl_continue = address & 0x7fffffff; > > debug_print_int("set IPL addr to", current->ipl_continue); > @@ -82,7 +84,7 @@ void jump_to_low_kernel(void) > } > > /* Trying to get PSW at zero address */ > - if (*((uint64_t *)0) & IPL_PSW_MASK) { > + if (*((uint64_t *)0) & RESET_PSW_MASK) { > jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff); > } > >
On Tue, 3 Dec 2019 08:28:13 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > We need to set the short psw indication bit in the reset psw, as it is > a short psw. > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) Thanks, applied (together with a rebuild of the bios images.)
On Thu, 5 Dec 2019 11:12:39 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 3 Dec 2019 08:28:13 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > > > We need to set the short psw indication bit in the reset psw, as it is > > a short psw. > > > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > > --- > > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > Thanks, applied (together with a rebuild of the bios images.) This unfortunately breaks 'make check-qtest-s390x': TEST check-qtest-s390x: tests/boot-serial-test TEST check-qtest-s390x: tests/pxe-test ERROR - too few tests run (expected 1, got 0) When I revert this, the rebuild, and "s390x: Properly fetch and test the short psw on diag308 subc 0/1" (as it exposes the bug this commit tried to fix), everything passes again. No idea what is wrong, though :( For now, I've dropped the three patches mentioned above from the s390-next branch (I plan to send a pull request later). Let's fix this on top once we figured out whatever went wrong, no need to rush here.
On 12/13/19 1:06 PM, Cornelia Huck wrote: > On Thu, 5 Dec 2019 11:12:39 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Tue, 3 Dec 2019 08:28:13 -0500 >> Janosch Frank <frankja@linux.ibm.com> wrote: >> >>> We need to set the short psw indication bit in the reset psw, as it is >>> a short psw. >>> >>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> Thanks, applied (together with a rebuild of the bios images.) > > This unfortunately breaks 'make check-qtest-s390x': > > TEST check-qtest-s390x: tests/boot-serial-test > TEST check-qtest-s390x: tests/pxe-test > ERROR - too few tests run (expected 1, got 0) > > When I revert this, the rebuild, and "s390x: Properly fetch and test > the short psw on diag308 subc 0/1" (as it exposes the bug this commit > tried to fix), everything passes again. No idea what is wrong, though :( > > For now, I've dropped the three patches mentioned above from the > s390-next branch (I plan to send a pull request later). Let's fix this > on top once we figured out whatever went wrong, no need to rush here. > > Sounds good
On 13/12/2019 13.06, Cornelia Huck wrote: > On Thu, 5 Dec 2019 11:12:39 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Tue, 3 Dec 2019 08:28:13 -0500 >> Janosch Frank <frankja@linux.ibm.com> wrote: >> >>> We need to set the short psw indication bit in the reset psw, as it is >>> a short psw. >>> >>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- >>> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> Thanks, applied (together with a rebuild of the bios images.) > > This unfortunately breaks 'make check-qtest-s390x': > > TEST check-qtest-s390x: tests/boot-serial-test > TEST check-qtest-s390x: tests/pxe-test > ERROR - too few tests run (expected 1, got 0) > > When I revert this, the rebuild, and "s390x: Properly fetch and test > the short psw on diag308 subc 0/1" (as it exposes the bug this commit > tried to fix), everything passes again. No idea what is wrong, though :( > > For now, I've dropped the three patches mentioned above from the > s390-next branch (I plan to send a pull request later). Let's fix this > on top once we figured out whatever went wrong, no need to rush here. I think I might have found the problem. You now need this patch on top to get it working again: diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c --- 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; Without this patch, there is now a 32-bit padding in the struct. Not sure why it then fails (I had a quick look at the disassembly of jump2ipl.o and could not spot anything that was obviously wrong), but if I mark the struct as "packed" to get the old size again, then the pxe-test is working fine for me again. Thomas
On 17/12/2019 13.36, Thomas Huth wrote: > On 13/12/2019 13.06, Cornelia Huck wrote: >> On Thu, 5 Dec 2019 11:12:39 +0100 >> Cornelia Huck <cohuck@redhat.com> wrote: >> >>> On Tue, 3 Dec 2019 08:28:13 -0500 >>> Janosch Frank <frankja@linux.ibm.com> wrote: >>> >>>> We need to set the short psw indication bit in the reset psw, as it is >>>> a short psw. >>>> >>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- >>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>> >>> Thanks, applied (together with a rebuild of the bios images.) >> >> This unfortunately breaks 'make check-qtest-s390x': >> >> TEST check-qtest-s390x: tests/boot-serial-test >> TEST check-qtest-s390x: tests/pxe-test >> ERROR - too few tests run (expected 1, got 0) >> >> When I revert this, the rebuild, and "s390x: Properly fetch and test >> the short psw on diag308 subc 0/1" (as it exposes the bug this commit >> tried to fix), everything passes again. No idea what is wrong, though :( >> >> For now, I've dropped the three patches mentioned above from the >> s390-next branch (I plan to send a pull request later). Let's fix this >> on top once we figured out whatever went wrong, no need to rush here. > > I think I might have found the problem. You now need this patch on top > to get it working again: > > diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > --- 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; > > Without this patch, there is now a 32-bit padding in the struct. Not > sure why it then fails (I had a quick look at the disassembly of > jump2ipl.o and could not spot anything that was obviously wrong), but if > I mark the struct as "packed" to get the old size again, then the > pxe-test is working fine for me again. Never mind, the problem was not in the jump2ipl.c code, but in the pxe-test: The code only worked if the r3 register contained only zeroes in the upper bits. The changes to jump2ipl.c here caused the compiler to put a different value in r3, so the test suddenly crashed. I've now sent a patch to fix the test ("tests/boot-sector: Fix the bad s390x assembler code"). Thomas
On 12/17/19 4:09 PM, Thomas Huth wrote: > On 17/12/2019 13.36, Thomas Huth wrote: >> On 13/12/2019 13.06, Cornelia Huck wrote: >>> On Thu, 5 Dec 2019 11:12:39 +0100 >>> Cornelia Huck <cohuck@redhat.com> wrote: >>> >>>> On Tue, 3 Dec 2019 08:28:13 -0500 >>>> Janosch Frank <frankja@linux.ibm.com> wrote: >>>> >>>>> We need to set the short psw indication bit in the reset psw, as it is >>>>> a short psw. >>>>> >>>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") >>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>> --- >>>>> pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- >>>>> 1 file changed, 7 insertions(+), 5 deletions(-) >>>> >>>> Thanks, applied (together with a rebuild of the bios images.) >>> >>> This unfortunately breaks 'make check-qtest-s390x': >>> >>> TEST check-qtest-s390x: tests/boot-serial-test >>> TEST check-qtest-s390x: tests/pxe-test >>> ERROR - too few tests run (expected 1, got 0) >>> >>> When I revert this, the rebuild, and "s390x: Properly fetch and test >>> the short psw on diag308 subc 0/1" (as it exposes the bug this commit >>> tried to fix), everything passes again. No idea what is wrong, though :( >>> >>> For now, I've dropped the three patches mentioned above from the >>> s390-next branch (I plan to send a pull request later). Let's fix this >>> on top once we figured out whatever went wrong, no need to rush here. >> >> I think I might have found the problem. You now need this patch on top >> to get it working again: >> >> diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >> --- 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; >> >> Without this patch, there is now a 32-bit padding in the struct. Not >> sure why it then fails (I had a quick look at the disassembly of >> jump2ipl.o and could not spot anything that was obviously wrong), but if >> I mark the struct as "packed" to get the old size again, then the >> pxe-test is working fine for me again. > > Never mind, the problem was not in the jump2ipl.c code, but in the > pxe-test: The code only worked if the r3 register contained only zeroes > in the upper bits. The changes to jump2ipl.c here caused the compiler to > put a different value in r3, so the test suddenly crashed. I've now sent > a patch to fix the test ("tests/boot-sector: Fix the bad s390x assembler > code"). > > Thomas Thank you a lot for having a look into that!
On Tue, 3 Dec 2019 08:28:13 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > We need to set the short psw indication bit in the reset psw, as it is > a short psw. > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) FWIW, I'll re-queue this patch (and the bios rebuild) in front of the short psw fix that exposed the problem, in order to preserve bisectability.
On Tue, 3 Dec 2019 08:28:13 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > We need to set the short psw indication bit in the reset psw, as it is > a short psw. > > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) Thanks, requeued this and the bios rebuild.
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index 266f1502b9..da13c43cc0 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -12,11 +12,11 @@ #define KERN_IMAGE_START 0x010000UL #define PSW_MASK_64 0x0000000100000000ULL #define PSW_MASK_32 0x0000000080000000ULL -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64) typedef struct ResetInfo { - uint32_t ipl_mask; - uint32_t ipl_addr; + uint64_t ipl_psw; uint32_t ipl_continue; } ResetInfo; @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address) ResetInfo *current = 0; save = *current; - current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2; + + current->ipl_psw = (uint64_t) &jump_to_IPL_2; + current->ipl_psw |= RESET_PSW_MASK; current->ipl_continue = address & 0x7fffffff; debug_print_int("set IPL addr to", current->ipl_continue); @@ -82,7 +84,7 @@ void jump_to_low_kernel(void) } /* Trying to get PSW at zero address */ - if (*((uint64_t *)0) & IPL_PSW_MASK) { + if (*((uint64_t *)0) & RESET_PSW_MASK) { jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff); }
We need to set the short psw indication bit in the reset psw, as it is a short psw. fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest") Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/jump2ipl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)