| Message ID | 20230224152015.2943564-1-nsg@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] s390x: Add tests for execute-type instructions | expand |
On 2/24/23 16:20, Nina Schoetterl-Glausch wrote: > 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. For instructions like execute where the details matter it's a great idea to have a lot of comments maybe even loose references to the PoP so people can read up on the issue more easily. > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Reviewed-by: Nico Boehr <nrb@linux.ibm.com> > --- > > > v1 -> v2: > * add test to unittests.cfg and .gitlab-ci.yml > * pick up R-b (thanks Nico) > > > TCG does the address calculation relative to the execute instruction. Always? I.e. what are you telling me here? > > > s390x/Makefile | 1 + > s390x/ex.c | 92 +++++++++++++++++++++++++++++++++++++++++++++ > s390x/unittests.cfg | 3 ++ > .gitlab-ci.yml | 1 + > 4 files changed, 97 insertions(+) > create mode 100644 s390x/ex.c > > diff --git a/s390x/Makefile b/s390x/Makefile > index 97a61611..6cf8018b 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 00000000..1bf4d8cd > --- /dev/null > +++ b/s390x/ex.c > @@ -0,0 +1,92 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright IBM Corp. 2023 > + * > + * Test EXECUTE (RELATIVE LONG). > + */ > + > +#include <libcflat.h> > + Take my words with some salt, I never had a close look at the branch instructions other than brc. This is "branch and save" and the "r" in "basr" says that it's the RR variant. It's not relative the way that "bras" is, right? Hence ret_addr and after_ex both point to 1f. I'd like to have a comment here that states that this is not a relative branch at all. The r specifies the instruction format. > +static void test_basr(void) > +{ > + uint64_t ret_addr, after_ex; > + > + report_prefix_push("BASR"); > + asm volatile ( ".pushsection .rodata\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(); > +} > + > +/* > + * According to PoP (Branch-Address Generation), the address is relative to > + * BRAS when it is the target of an execute-type instruction. > + */ Is there any merit in testing the other br* instructions as well or are they running through the same TCG function? > +static void test_bras(void) > +{ > + uint64_t after_target, ret_addr, after_ex, branch_addr; > + > + report_prefix_push("BRAS"); > + asm volatile ( ".pushsection .text.ex_bras, \"x\"\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" > + "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" > + : [after_target] "=d" (after_target), > + [ret_addr] "=d" (ret_addr), > + [after_ex] "=d" (after_ex), > + [branch_addr] "=d" (branch_addr) > + ); > + > + report(after_target == branch_addr, "address calculated relative to BRAS"); > + report(ret_addr == after_ex, "return address after EX"); > + report_prefix_pop(); > +} > + Add: /* larl follows the address generation of relative branch instructions */ > +static void test_larl(void) > +{ > + uint64_t target, addr; > + > + report_prefix_push("LARL"); > + asm volatile ( ".pushsection .rodata\n" > + "0: larl %[addr],0\n" > + " .popsection\n" > + > + " larl %[target],0b\n" > + " exrl 0,0b\n" > + : [target] "=d" (target), > + [addr] "=d" (addr) > + ); > + > + report(target == addr, "address calculated relative to LARL"); > + report_prefix_pop(); > +} > + > +int main(int argc, char **argv) > +{ We're missing push and pop around the test function block so that we know which file generated the output. report_prefix_push("execute"); > + test_basr(); > + test_bras(); > + test_larl(); report_prefix_pop(); > + return report_summary(); > +} > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > index d97eb5e9..b61faf07 100644 > --- a/s390x/unittests.cfg > +++ b/s390x/unittests.cfg > @@ -215,3 +215,6 @@ file = migration-skey.elf > smp = 2 > groups = migration > extra_params = -append '--parallel' > + > +[execute] > +file = ex.elf > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index ad7949c9..a999f64a 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -275,6 +275,7 @@ s390x-kvm: > - ACCEL=kvm ./run_tests.sh > selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf > cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie > + execute > | tee results.txt > - grep -q PASS results.txt && ! grep -q FAIL results.txt > only: > > base-commit: e3c5c3ef2524c58023073c0fadde2e8ae3c04ec6
On Mon, 2023-02-27 at 16:44 +0100, Janosch Frank wrote: > On 2/24/23 16:20, Nina Schoetterl-Glausch wrote: > > 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. > > For instructions like execute where the details matter it's a great idea > to have a lot of comments maybe even loose references to the PoP so > people can read up on the issue more easily. > > > > > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Reviewed-by: Nico Boehr <nrb@linux.ibm.com> > > --- > > > > > > v1 -> v2: > > * add test to unittests.cfg and .gitlab-ci.yml > > * pick up R-b (thanks Nico) > > > > > > TCG does the address calculation relative to the execute instruction. > > Always? AFAIK yes. Everything that has an operand that is relative to the instruction given by the immediate in the instruction and goes through in2_ri2 in TCG will have this problem, because in2_ri2 does the calculation relative to pc_next which is the address of the EX(RL). That should make fixing it easier tho. > I.e. what are you telling me here? > > > > > > > s390x/Makefile | 1 + > > s390x/ex.c | 92 +++++++++++++++++++++++++++++++++++++++++++++ > > s390x/unittests.cfg | 3 ++ > > .gitlab-ci.yml | 1 + > > 4 files changed, 97 insertions(+) > > create mode 100644 s390x/ex.c > > > > diff --git a/s390x/Makefile b/s390x/Makefile > > index 97a61611..6cf8018b 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 00000000..1bf4d8cd > > --- /dev/null > > +++ b/s390x/ex.c > > @@ -0,0 +1,92 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright IBM Corp. 2023 > > + * > > + * Test EXECUTE (RELATIVE LONG). > > + */ > > + > > +#include <libcflat.h> > > + > > Take my words with some salt, I never had a close look at the branch > instructions other than brc. > > This is "branch and save" and the "r" in "basr" says that it's the RR > variant. It's not relative the way that "bras" is, right? Yes, it isn't. It's so there are tests for different address generations that are a function of the current instruction address. > > Hence ret_addr and after_ex both point to 1f. > > I'd like to have a comment here that states that this is not a relative > branch at all. The r specifies the instruction format. Yeah, will do. > > > +static void test_basr(void) > > +{ > > + uint64_t ret_addr, after_ex; > > + > > + report_prefix_push("BASR"); > > + asm volatile ( ".pushsection .rodata\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(); > > +} > > + > > +/* > > + * According to PoP (Branch-Address Generation), the address is relative to > > + * BRAS when it is the target of an execute-type instruction. > > + */ > > Is there any merit in testing the other br* instructions as well or are > they running through the same TCG function? Well, there is in the sense that it's better not to make assumptions about the implementation, but given the above it shouldn't make a difference in practice, unless my understanding is wrong or I'm missing some special case. > > > +static void test_bras(void) > > +{ > > + uint64_t after_target, ret_addr, after_ex, branch_addr; > > + > > + report_prefix_push("BRAS"); > > + asm volatile ( ".pushsection .text.ex_bras, \"x\"\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" > > + "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" > > + : [after_target] "=d" (after_target), > > + [ret_addr] "=d" (ret_addr), > > + [after_ex] "=d" (after_ex), > > + [branch_addr] "=d" (branch_addr) > > + ); > > + > > + report(after_target == branch_addr, "address calculated relative to BRAS"); > > + report(ret_addr == after_ex, "return address after EX"); > > + report_prefix_pop(); > > +} > > + > > Add: > /* larl follows the address generation of relative branch instructions */ Yes, will also add another test for a relative immediate instruction that doesn't explicitly state the same in the description. > > +static void test_larl(void) > > +{ > > + uint64_t target, addr; > > + > > + report_prefix_push("LARL"); > > + asm volatile ( ".pushsection .rodata\n" > > + "0: larl %[addr],0\n" > > + " .popsection\n" > > + > > + " larl %[target],0b\n" > > + " exrl 0,0b\n" > > + : [target] "=d" (target), > > + [addr] "=d" (addr) > > + ); > > + > > + report(target == addr, "address calculated relative to LARL"); > > + report_prefix_pop(); > > +} > > + > > +int main(int argc, char **argv) > > +{ > > We're missing push and pop around the test function block so that we > know which file generated the output. > > report_prefix_push("execute"); > > > + test_basr(); > > + test_bras(); > > + test_larl(); > > report_prefix_pop(); > > > + return report_summary(); > > +} > > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg > > index d97eb5e9..b61faf07 100644 > > --- a/s390x/unittests.cfg > > +++ b/s390x/unittests.cfg > > @@ -215,3 +215,6 @@ file = migration-skey.elf > > smp = 2 > > groups = migration > > extra_params = -append '--parallel' > > + > > +[execute] > > +file = ex.elf > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index ad7949c9..a999f64a 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -275,6 +275,7 @@ s390x-kvm: > > - ACCEL=kvm ./run_tests.sh > > selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf > > cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie > > + execute > > | tee results.txt > > - grep -q PASS results.txt && ! grep -q FAIL results.txt > > only: > > > > base-commit: e3c5c3ef2524c58023073c0fadde2e8ae3c04ec6 >
diff --git a/s390x/Makefile b/s390x/Makefile index 97a61611..6cf8018b 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 00000000..1bf4d8cd --- /dev/null +++ b/s390x/ex.c @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright IBM Corp. 2023 + * + * Test EXECUTE (RELATIVE LONG). + */ + +#include <libcflat.h> + +static void test_basr(void) +{ + uint64_t ret_addr, after_ex; + + report_prefix_push("BASR"); + asm volatile ( ".pushsection .rodata\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(); +} + +/* + * According to PoP (Branch-Address Generation), the address is relative to + * BRAS when it is the target of an 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.ex_bras, \"x\"\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" + "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" + : [after_target] "=d" (after_target), + [ret_addr] "=d" (ret_addr), + [after_ex] "=d" (after_ex), + [branch_addr] "=d" (branch_addr) + ); + + report(after_target == branch_addr, "address calculated relative to BRAS"); + report(ret_addr == after_ex, "return address after EX"); + report_prefix_pop(); +} + +static void test_larl(void) +{ + uint64_t target, addr; + + report_prefix_push("LARL"); + asm volatile ( ".pushsection .rodata\n" + "0: larl %[addr],0\n" + " .popsection\n" + + " larl %[target],0b\n" + " exrl 0,0b\n" + : [target] "=d" (target), + [addr] "=d" (addr) + ); + + report(target == addr, "address calculated relative to LARL"); + report_prefix_pop(); +} + +int main(int argc, char **argv) +{ + test_basr(); + test_bras(); + test_larl(); + + return report_summary(); +} diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg index d97eb5e9..b61faf07 100644 --- a/s390x/unittests.cfg +++ b/s390x/unittests.cfg @@ -215,3 +215,6 @@ file = migration-skey.elf smp = 2 groups = migration extra_params = -append '--parallel' + +[execute] +file = ex.elf diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ad7949c9..a999f64a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -275,6 +275,7 @@ s390x-kvm: - ACCEL=kvm ./run_tests.sh selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie + execute | tee results.txt - grep -q PASS results.txt && ! grep -q FAIL results.txt only: