On Fri, 2023-03-03 at 11:11 +0100, Janosch Frank wrote:
> On 2/28/23 21:44, 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.
> >
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx>
>
> Some small nits below, I can fix that up when picking.
>
> Reviewed-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
Thanks.
>
> > ---
> >
> >
> [...]
> > s390x/Makefile | 1 +
> > s390x/ex.c | 169 ++++++++++++++++++++++++++++++++++++++++++++
> > s390x/unittests.cfg | 3 +
> > .gitlab-ci.yml | 1 +
> > 4 files changed, 174 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..3a22e496
> > --- /dev/null
> > +++ b/s390x/ex.c
> > @@ -0,0 +1,169 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright IBM Corp. 2023
> > + *
> > + * Test EXECUTE (RELATIVE LONG).
> > + * These instruction execute a target instruction. The target instruction is formed
>
> s/instruction/instructions/ ?
Yes.
>
> > + * 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>
> > +
>
> [...]
>
> > +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();
> > +}
> > +
> > +/* LOAD LOGICAL RELATIVE LONG.
> > + * If it is the target of an execute-type instruction, the address is relative
> > + * to the LLGFRL.
> > + */
>
> This is the only instruction where there's no comment about the execute
> behavior but it would only make sense that it follows the same address
> generation rules.
I added CRL first because I overlooked that it *does* specify it's behavior and
decided to keep it in, because why not.
Then checked all the other instructions.
It's a bit wonky, the PoP says:
Operand-Address Generation
...
The signed binary inte-
ger specifies the number of halfwords that is added
to the address of the current instruction (or the
address of the execute-type instruction if the instruc-
tion having the RI2 field is the target of an execute-
type instruction) to form the intermediate value.
So it says *address* not *target* of the execute-type insn.
then:
Branch-Address Generation
...
The branch address is the number of half-
words designated by the RI2 field added to the
address of the relative-branch instruction.
...
In all formats, the sec-
ond addend is the 64-bit address of the branch
instruction. The address of the branch instruction is
the instruction address in the PSW before that
address is updated to address the next sequential
instruction, or it is the address of the target of the
execute-type instruction (EXECUTE or EXECUTE
RELATIVE LONG) if an execute-type instruction is
used.
And everything but LLGFRL explicitly mentions that it is relative to the target.
We could mention in the comment that the PoP doesn't explicitly state that,
but the realty is that that's what the machine does.
>
> > +static void test_llgfrl(void)
> > +{
> > + uint64_t target, value;
> > +
> > + report_prefix_push("LLGFRL");
> > + asm volatile ( ".pushsection .rodata\n"
> > + " .balign 4\n"
> > + "0: llgfrl %[value],0\n"
> > + " .popsection\n"
> > +
> > + " llgfrl %[target],0b\n"
> > + " exrl 0,0b\n"
> > + : [target] "=d" (target),
> > + [value] "=d" (value)
> > + );
> > +
> > + report(target == value, "loaded correct value");
> > + report_prefix_pop();
> > +}
> > +
> > +/*
> > + * COMPARE RELATIVE LONG
> > + * If it is the target of an execute-type instruction, the address is relative
> > + * to the CRL.
> > + */
> > +static void test_crl(void)
> > +{
> > + uint32_t program_mask, cc, crl_word;
> > +
> > + report_prefix_push("CRL");
> > + asm volatile ( ".pushsection .rodata\n"
> > + " .balign 4\n" //operand of crl must be word aligned
>
> Just put it on a new line so we don't mix commenting stiles.
> Inline assembly is already hardly readable anyway.
Could also put it in the asm "#operand of crl must be word aligned" but I haven't
seen that anywhere else, and the benefit is negligible.
>
> > + "0: crl %[crl_word],0\n"
> > + " .popsection\n"
> > +
> > + " lrl %[crl_word],0b\n"
> > + //align (pad with nop), in case the wrong bad operand is used
> > + " .balignw 4,0x0707\n"
> > + " exrl 0,0b\n"
> > + " ipm %[program_mask]\n"
> > + : [program_mask] "=d" (program_mask),
> > + [crl_word] "=d" (crl_word)
> > + :: "cc"
> > + );
> > +
> > + cc = program_mask >> 28;
> > + report(!cc, "operand compared to is relative to CRL");
> > + report_prefix_pop();
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + report_prefix_push("ex");
> > + test_basr();
> > + test_bras();
> > + test_larl();
> > + test_llgfrl();
> > + test_crl();
> > + 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
>
