On Tue, 2023-04-04 at 17:05 +0200, Thomas Huth wrote: > On 04/04/2023 16.54, Nina Schoetterl-Glausch wrote: > > On Tue, 2023-04-04 at 16:15 +0200, Thomas Huth wrote: > > > On 04/04/2023 13.36, Nico Boehr wrote: > > > > From: Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> > > > > > > > > Test the instruction address used by targets of an execute instruction. > > > > When the target instruction calculates a relative address, the result is > > > > relative to the target instruction, not the execute instruction. > > > > > > > > Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx> > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> > > > > Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx> > > > > Link: https://lore.kernel.org/r/20230317112339.774659-1-nsg@xxxxxxxxxxxxx > > > > Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx> > > > > --- > > > > s390x/Makefile | 1 + > > > > s390x/ex.c | 188 ++++++++++++++++++++++++++++++++++++++++++++ > > > > s390x/unittests.cfg | 3 + > > > > .gitlab-ci.yml | 1 + > > > > 4 files changed, 193 insertions(+) > > > > create mode 100644 s390x/ex.c > > > > > > > > diff --git a/s390x/Makefile b/s390x/Makefile > > > > index ab146eb..a80db53 100644 > > > > --- a/s390x/Makefile > > > > +++ b/s390x/Makefile > > > > @@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf > > > > tests += $(TEST_DIR)/panic-loop-pgm.elf > > > > tests += $(TEST_DIR)/migration-sck.elf > > > > tests += $(TEST_DIR)/exittime.elf > > > > +tests += $(TEST_DIR)/ex.elf > > > > > > > > pv-tests += $(TEST_DIR)/pv-diags.elf > > > > > > > > diff --git a/s390x/ex.c b/s390x/ex.c > > > > new file mode 100644 > > > > index 0000000..dbd8030 > > > > --- /dev/null > > > > +++ b/s390x/ex.c > > > > @@ -0,0 +1,188 @@ > > > > +// SPDX-License-Identifier: GPL-2.0-only > > > > +/* > > > > + * Copyright IBM Corp. 2023 > > > > + * > > > > + * Test EXECUTE (RELATIVE LONG). > > > > + * These instructions execute a target instruction. The target instruction is formed > > > > + * by reading an instruction from memory and optionally modifying some of its bits. > > > > + * The execution of the target instruction is the same as if it was executed > > > > + * normally as part of the instruction sequence, except for the instruction > > > > + * address and the instruction-length code. > > > > + */ > > > > + > > > > +#include <libcflat.h> > > > > + > > > > +/* > > > > + * Accesses to the operand of execute-type instructions are instruction fetches. > > > > + * Minimum alignment is two, since the relative offset is specified by number of halfwords. > > > > + */ > > > > +asm ( ".pushsection .text.exrl_targets,\"x\"\n" > > > > +" .balign 2\n" > > > > +" .popsection\n" > > > > +); > > > > + > > > > +/* > > > > + * BRANCH AND SAVE, register register variant. > > > > + * Saves the next instruction address (address from PSW + length of instruction) > > > > + * to the first register. No branch is taken in this test, because 0 is > > > > + * specified as target. > > > > + * BASR does *not* perform a relative address calculation with an intermediate. > > > > + */ > > > > +static void test_basr(void) > > > > +{ > > > > + uint64_t ret_addr, after_ex; > > > > + > > > > + report_prefix_push("BASR"); > > > > + asm volatile ( ".pushsection .text.exrl_targets\n" > > > > + "0: basr %[ret_addr],0\n" > > > > + " .popsection\n" > > > > + > > > > + " larl %[after_ex],1f\n" > > > > + " exrl 0,0b\n" > > > > + "1:\n" > > > > + : [ret_addr] "=d" (ret_addr), > > > > + [after_ex] "=d" (after_ex) > > > > + ); > > > > + > > > > + report(ret_addr == after_ex, "return address after EX"); > > > > + report_prefix_pop(); > > > > +} > > > > + > > > > +/* > > > > + * BRANCH RELATIVE AND SAVE. > > > > + * According to PoP (Branch-Address Generation), the address calculated relative > > > > + * to the instruction address is relative to BRAS when it is the target of an > > > > + * execute-type instruction, not relative to the execute-type instruction. > > > > + */ > > > > +static void test_bras(void) > > > > +{ > > > > + uint64_t after_target, ret_addr, after_ex, branch_addr; > > > > + > > > > + report_prefix_push("BRAS"); > > > > + asm volatile ( ".pushsection .text.exrl_targets\n" > > > > + "0: bras %[ret_addr],1f\n" > > > > + " nopr %%r7\n" > > > > + "1: larl %[branch_addr],0\n" > > > > + " j 4f\n" > > > > + " .popsection\n" > > > > + > > > > + " larl %[after_target],1b\n" > > > > + " larl %[after_ex],3f\n" > > > > + "2: exrl 0,0b\n" > > > > +/* > > > > + * In case the address calculation is correct, we jump by the relative offset 1b-0b from 0b to 1b. > > > > + * In case the address calculation is relative to the exrl (i.e. a test failure), > > > > + * put a valid instruction at the same relative offset from the exrl, so the test continues in a > > > > + * controlled manner. > > > > + */ > > > > + "3: larl %[branch_addr],0\n" > > > > + "4:\n" > > > > + > > > > + " .if (1b - 0b) != (3b - 2b)\n" > > > > + " .error \"right and wrong target must have same offset\"\n" > > > > + " .endif\n" > > > > > > FWIW, this is failing with Clang 15 for me: > > > > > > s390x/ex.c:81:4: error: expected absolute expression > > > " .if (1b - 0b) != (3b - 2b)\n" > > > ^ > > > <inline asm>:12:6: note: instantiated into assembly here > > > .if (1b - 0b) != (3b - 2b) > > > > Seems gcc is smarter here than clang. > > Yeah, the assembler from clang is quite a bit behind on s390x ... in the > past I was only able to compile the k-u-t with Clang when using the > "-no-integrated-as" option ... but at least in the most recent version it > seems to have caught up now enough to be very close to compile it with the > built-in assembler, so it would be great to get this problem here fixed > somehow, too... > > > Just deleting that .if block would work, it's basically only a static assert. > > What do you think? > > Other than that I can't think of anything. > > Yes, either delete it ... or maybe you could return the two values (1b - 0b) > and (3b - 2b) as output from the asm statement and do an assert() in C instead? No, that's too late, it'd crash before if the invariant doesn't hold. Could do a runtime check in asm but I don't think it's worth it. So lets go for deletion. Do you wan't to fix it up when pulling or do you want a new version and pull request? > > Thomas >
