Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Tue, 3 Dec 2019 08:28:13 -0500 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 > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) Thanks, requeued this and the bios rebuild.
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Tue, 3 Dec 2019 08:28:13 -0500 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 > --- > 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.
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
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 wrote: >>> On Tue, 3 Dec 2019 08:28:13 -0500 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 > --- > 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': >>> >>>TESTcheck-qtest-s390x: tests/boot-serial-test >>>TESTcheck-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! signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
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 wrote: >> >>> On Tue, 3 Dec 2019 08:28:13 -0500 >>> 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 --- 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': >> >>TESTcheck-qtest-s390x: tests/boot-serial-test >>TESTcheck-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
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On 13/12/2019 13.06, Cornelia Huck wrote: > On Thu, 5 Dec 2019 11:12:39 +0100 > Cornelia Huck wrote: > >> On Tue, 3 Dec 2019 08:28:13 -0500 >> 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 >>> --- >>> 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': > >TESTcheck-qtest-s390x: tests/boot-serial-test >TESTcheck-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
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On 12/13/19 1:06 PM, Cornelia Huck wrote: > On Thu, 5 Dec 2019 11:12:39 +0100 > Cornelia Huck wrote: > >> On Tue, 3 Dec 2019 08:28:13 -0500 >> 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 >>> --- >>> 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': > >TESTcheck-qtest-s390x: tests/boot-serial-test >TESTcheck-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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Thu, 5 Dec 2019 11:12:39 +0100 Cornelia Huck wrote: > On Tue, 3 Dec 2019 08:28:13 -0500 > 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 > > --- > > 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': TESTcheck-qtest-s390x: tests/boot-serial-test TESTcheck-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.
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Tue, 3 Dec 2019 08:28:13 -0500 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 > --- > pc-bios/s390-ccw/jump2ipl.c | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) Thanks, applied (together with a rebuild of the bios images.)
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
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 Acked-by: Christian Borntraeger > --- > 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 0x01UL > #define PSW_MASK_64 0x0001ULL > #define PSW_MASK_32 0x8000ULL > -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) > +#define PSW_MASK_SHORTPSW 0x0008ULL > +#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) _to_IPL_2; > + > +current->ipl_psw = (uint64_t) _to_IPL_2; > +current->ipl_psw |= RESET_PSW_MASK; > current->ipl_continue = address & 0x7fff; > > 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)) & 0x7fff); > } > >
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Tue, 3 Dec 2019 08:28:13 -0500 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") s/fixes: 9629823290/Fixes: 962982329029/ > Signed-off-by: Janosch Frank > --- > 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 0x01UL > #define PSW_MASK_64 0x0001ULL > #define PSW_MASK_32 0x8000ULL > -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) > +#define PSW_MASK_SHORTPSW 0x0008ULL > +#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) _to_IPL_2; > + > +current->ipl_psw = (uint64_t) _to_IPL_2; > +current->ipl_psw |= RESET_PSW_MASK; > current->ipl_continue = address & 0x7fff; > > 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)) & 0x7fff); > } > 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.
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
On Tue, 3 Dec 2019 14:33:25 +0100 Christian Borntraeger 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 > > We should also add > commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, > github-cohuck/s390-next) > Author: Janosch Frank > AuthorDate: Fri Nov 29 09:20:23 2019 -0500 > Commit: Cornelia Huck > 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(-)
[PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
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 --- 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 0x01UL #define PSW_MASK_64 0x0001ULL #define PSW_MASK_32 0x8000ULL -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) +#define PSW_MASK_SHORTPSW 0x0008ULL +#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) _to_IPL_2; + +current->ipl_psw = (uint64_t) _to_IPL_2; +current->ipl_psw |= RESET_PSW_MASK; current->ipl_continue = address & 0x7fff; 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)) & 0x7fff); } -- 2.20.1
Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask
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 We should also add commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next) Author: Janosch Frank AuthorDate: Fri Nov 29 09:20:23 2019 -0500 Commit: Cornelia Huck 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. > --- > 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 0x01UL > #define PSW_MASK_64 0x0001ULL > #define PSW_MASK_32 0x8000ULL > -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64) > +#define PSW_MASK_SHORTPSW 0x0008ULL > +#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) _to_IPL_2; > + > +current->ipl_psw = (uint64_t) _to_IPL_2; > +current->ipl_psw |= RESET_PSW_MASK; > current->ipl_continue = address & 0x7fff; > > 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)) & 0x7fff); > } > >