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@xxxxxxxxxxxxx>
> > Reviewed-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
> > ---
> >
> >
> > 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
>
