| Message ID | 20211217103137.1293092-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: diag288: Improve readability | expand |
On 17/12/2021 11.31, Nico Boehr wrote: > We clobber r0 and thus should let the compiler know we're doing so. > > Because we change from basic to extended ASM, we need to change the > register names, as %r0 will be interpreted as a token in the assembler > template. > > For consistency, we align with the common style in kvm-unit-tests which > is just 0. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > s390x/diag288.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/s390x/diag288.c b/s390x/diag288.c > index 072c04a5cbd6..da7b06c365bf 100644 > --- a/s390x/diag288.c > +++ b/s390x/diag288.c > @@ -94,11 +94,12 @@ static void test_bite(void) > /* Arm watchdog */ > lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; > diag288(CODE_INIT, 15, ACTION_RESTART); > - asm volatile(" larl %r0, 1f\n" > - " stg %r0, 424\n" > + asm volatile(" larl 0, 1f\n" > + " stg 0, 424\n" Would it work to use %%r0 instead? > "0: nop\n" > " j 0b\n" > - "1:"); > + "1:" > + : : : "0"); > report_pass("restart"); > } Anyway: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 12/17/21 14:47, Thomas Huth wrote: > On 17/12/2021 11.31, Nico Boehr wrote: >> We clobber r0 and thus should let the compiler know we're doing so. >> >> Because we change from basic to extended ASM, we need to change the >> register names, as %r0 will be interpreted as a token in the assembler >> template. >> >> For consistency, we align with the common style in kvm-unit-tests which >> is just 0. >> >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> --- >> s390x/diag288.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/s390x/diag288.c b/s390x/diag288.c >> index 072c04a5cbd6..da7b06c365bf 100644 >> --- a/s390x/diag288.c >> +++ b/s390x/diag288.c >> @@ -94,11 +94,12 @@ static void test_bite(void) >> /* Arm watchdog */ >> lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; >> diag288(CODE_INIT, 15, ACTION_RESTART); >> - asm volatile(" larl %r0, 1f\n" >> - " stg %r0, 424\n" >> + asm volatile(" larl 0, 1f\n" >> + " stg 0, 424\n" > > Would it work to use %%r0 instead? Yes, but I told him that looks weird, so that one is on me. @claudio @thomas What's your preferred way of dealing with this? > >> "0: nop\n" >> " j 0b\n" >> - "1:"); >> + "1:" >> + : : : "0"); >> report_pass("restart"); >> } > > Anyway: > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On Fri, 17 Dec 2021 15:16:34 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 12/17/21 14:47, Thomas Huth wrote: > > On 17/12/2021 11.31, Nico Boehr wrote: > >> We clobber r0 and thus should let the compiler know we're doing so. > >> > >> Because we change from basic to extended ASM, we need to change the > >> register names, as %r0 will be interpreted as a token in the assembler > >> template. > >> > >> For consistency, we align with the common style in kvm-unit-tests which > >> is just 0. > >> > >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > >> --- > >> s390x/diag288.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/s390x/diag288.c b/s390x/diag288.c > >> index 072c04a5cbd6..da7b06c365bf 100644 > >> --- a/s390x/diag288.c > >> +++ b/s390x/diag288.c > >> @@ -94,11 +94,12 @@ static void test_bite(void) > >> /* Arm watchdog */ > >> lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; > >> diag288(CODE_INIT, 15, ACTION_RESTART); > >> - asm volatile(" larl %r0, 1f\n" > >> - " stg %r0, 424\n" > >> + asm volatile(" larl 0, 1f\n" > >> + " stg 0, 424\n" > > > > Would it work to use %%r0 instead? > > Yes, but I told him that looks weird, so that one is on me. > @claudio @thomas What's your preferred way of dealing with this? I would prefer just 0 since that's what we use everywhere else too, but I won't oppose %%r0 if there are strong arguments for it (but then we need to decide a policy and stick to it) > > > > >> "0: nop\n" > >> " j 0b\n" > >> - "1:"); > >> + "1:" > >> + : : : "0"); > >> report_pass("restart"); > >> } > > > > Anyway: > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > >
On 17/12/2021 15.16, Janosch Frank wrote: > On 12/17/21 14:47, Thomas Huth wrote: >> On 17/12/2021 11.31, Nico Boehr wrote: >>> We clobber r0 and thus should let the compiler know we're doing so. >>> >>> Because we change from basic to extended ASM, we need to change the >>> register names, as %r0 will be interpreted as a token in the assembler >>> template. >>> >>> For consistency, we align with the common style in kvm-unit-tests which >>> is just 0. >>> >>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >>> --- >>> s390x/diag288.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/s390x/diag288.c b/s390x/diag288.c >>> index 072c04a5cbd6..da7b06c365bf 100644 >>> --- a/s390x/diag288.c >>> +++ b/s390x/diag288.c >>> @@ -94,11 +94,12 @@ static void test_bite(void) >>> /* Arm watchdog */ >>> lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; >>> diag288(CODE_INIT, 15, ACTION_RESTART); >>> - asm volatile(" larl %r0, 1f\n" >>> - " stg %r0, 424\n" >>> + asm volatile(" larl 0, 1f\n" >>> + " stg 0, 424\n" >> >> Would it work to use %%r0 instead? > > Yes, but I told him that looks weird, so that one is on me. > @claudio @thomas What's your preferred way of dealing with this? I think it's mostly a matter of taste. I slightly prefer %%r0 to just 0 so that it is clear from the first sight that it is a register and not an immediate constant. Additionally, there used to be a problem with older versions of Clang that required the %%rX syntax, see: https://git.qemu.org/?p=qemu.git;a=commitdiff;h=052b66e7211af6 But we're not supporting those Clang versions for the kvm-unit-tests anyway, so that doesn't really count. Thus, I don't mind too much, if everybody else prefers the bare numbers, then let's go with this. Thomas
diff --git a/s390x/diag288.c b/s390x/diag288.c index 072c04a5cbd6..da7b06c365bf 100644 --- a/s390x/diag288.c +++ b/s390x/diag288.c @@ -94,11 +94,12 @@ static void test_bite(void) /* Arm watchdog */ lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT; diag288(CODE_INIT, 15, ACTION_RESTART); - asm volatile(" larl %r0, 1f\n" - " stg %r0, 424\n" + asm volatile(" larl 0, 1f\n" + " stg 0, 424\n" "0: nop\n" " j 0b\n" - "1:"); + "1:" + : : : "0"); report_pass("restart"); }
We clobber r0 and thus should let the compiler know we're doing so. Because we change from basic to extended ASM, we need to change the register names, as %r0 will be interpreted as a token in the assembler template. For consistency, we align with the common style in kvm-unit-tests which is just 0. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- s390x/diag288.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)