| Message ID | 20211217103137.1293092-3-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: diag288: Improve readability | expand |
On Fri, Dec 17, 2021 at 11:31:37AM +0100, Nico Boehr wrote: > Use a more descriptive name instead of the magic number 424 (address of > restart new PSW in the lowcore). > > In addition, add a comment to make it more obvious what the ASM snippet > does. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > s390x/diag288.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/s390x/diag288.c b/s390x/diag288.c > index da7b06c365bf..a2c263e38338 100644 > --- a/s390x/diag288.c > +++ b/s390x/diag288.c > @@ -94,12 +94,15 @@ static void test_bite(void) > /* Arm watchdog */ > lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; > diag288(CODE_INIT, 15, ACTION_RESTART); > + /* Wait for restart interruption */ > asm volatile(" larl 0, 1f\n" > - " stg 0, 424\n" > + " stg 0, %[restart_new_psw]\n" > "0: nop\n" > " j 0b\n" > "1:" > - : : : "0"); > + : > + : [restart_new_psw] "T" (lc->restart_new_psw.addr) Even though it was wrong and missing before: this is an output not an input parameter. Also, older compilers might fail if only the "T" constraint is given (see gcc commit 3e4be43f69da ("S/390: Memory constraint cleanup")). Which means: "=RT" would be correct. To be on the safe side, and to avoid that gcc optimizes any potential prior C code away, I'd recommend to use "+RT" in this case. Also there is an ordering problem here: starting the time bomb before the restart psw has been setup is racy. It is unlikely that this fails, but still... Correct would be to setup the restart psw, and then start the time bomb. This would also allow to shorten the runtime of this test case to 1 second, instead of the 15 seconds it is running now. It was all like that before, I know. Just some comments ;)
On 12/17/21 12:08, Heiko Carstens wrote: > On Fri, Dec 17, 2021 at 11:31:37AM +0100, Nico Boehr wrote: >> Use a more descriptive name instead of the magic number 424 (address of >> restart new PSW in the lowcore). >> >> In addition, add a comment to make it more obvious what the ASM snippet >> does. >> >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> --- >> s390x/diag288.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/s390x/diag288.c b/s390x/diag288.c >> index da7b06c365bf..a2c263e38338 100644 >> --- a/s390x/diag288.c >> +++ b/s390x/diag288.c >> @@ -94,12 +94,15 @@ static void test_bite(void) >> /* Arm watchdog */ >> lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; >> diag288(CODE_INIT, 15, ACTION_RESTART); >> + /* Wait for restart interruption */ >> asm volatile(" larl 0, 1f\n" >> - " stg 0, 424\n" >> + " stg 0, %[restart_new_psw]\n" >> "0: nop\n" >> " j 0b\n" >> "1:" >> - : : : "0"); >> + : >> + : [restart_new_psw] "T" (lc->restart_new_psw.addr) > > Even though it was wrong and missing before: this is an output not an input > parameter. Also, older compilers might fail if only the "T" constraint is > given (see gcc commit 3e4be43f69da ("S/390: Memory constraint cleanup")). > Which means: "=RT" would be correct. To be on the safe side, and to avoid > that gcc optimizes any potential prior C code away, I'd recommend to use > "+RT" in this case. Thanks for clearing that up, those intricate details are quite hard to find/remember if you only write inline assembly every few months. > > Also there is an ordering problem here: starting the time bomb before the > restart psw has been setup is racy. It is unlikely that this fails, but > still... > > Correct would be to setup the restart psw, and then start the time > bomb. This would also allow to shorten the runtime of this test case to > 1 second, instead of the 15 seconds it is running now. While you are correct, the minimum value of the timer is 15s. Racing that will be quite hard. @Nico but yes, while you're at it you could switch that around so I don't have to explain that a second time. > > It was all like that before, I know. Just some comments ;) >
diff --git a/s390x/diag288.c b/s390x/diag288.c index da7b06c365bf..a2c263e38338 100644 --- a/s390x/diag288.c +++ b/s390x/diag288.c @@ -94,12 +94,15 @@ static void test_bite(void) /* Arm watchdog */ lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; diag288(CODE_INIT, 15, ACTION_RESTART); + /* Wait for restart interruption */ asm volatile(" larl 0, 1f\n" - " stg 0, 424\n" + " stg 0, %[restart_new_psw]\n" "0: nop\n" " j 0b\n" "1:" - : : : "0"); + : + : [restart_new_psw] "T" (lc->restart_new_psw.addr) + : "0"); report_pass("restart"); }
Use a more descriptive name instead of the magic number 424 (address of restart new PSW in the lowcore). In addition, add a comment to make it more obvious what the ASM snippet does. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- s390x/diag288.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)