Re: [kvm-unit-tests PATCH v5] s390x: Add tests for execute-type instructions

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

On Mon, 2023-03-13 at 19:16 +0100, Claudio Imbrenda wrote:
> On Fri, 10 Mar 2023 19:11:31 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> 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.
> > 
> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> > 
> > 
> > v4 -> v5:
> >  * word align the execute-type instruction, preventing a specification
> >    exception if the address calculation is wrong, since LLGFRL requires
> >    word alignment
> >  * change wording of comment
> > 
> > v3 -> v4:
> >  * fix nits (thanks Janosch)
> >  * pickup R-b (thanks Janosch)
> > 
> > v2 -> v3:
> >  * add some comments (thanks Janosch)
> >  * add two new tests (drop Nico's R-b)
> >  * push prefix
> > 
> > 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.
> > 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 has 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.
> > 
> > 
> > Range-diff against v4:
> > 1:  f29ef634 ! 1:  57f8f256 s390x: Add tests for execute-type instructions
> >     @@ s390x/ex.c (new)
> >      +		"	.popsection\n"
> >      +
> >      +		"	llgfrl	%[target],0b\n"
> >     ++		//align (pad with nop), in case the wrong operand is used
> >     ++		"	.balignw 4,0x0707\n"
> >      +		"	exrl	0,0b\n"
> >      +		: [target] "=d" (target),
> >      +		  [value] "=d" (value)
> >     @@ s390x/ex.c (new)
> >      +		"	.popsection\n"
> >      +
> >      +		"	lrl	%[crl_word],0b\n"
> >     -+		//align (pad with nop), in case the wrong bad operand is used
> >     ++		//align (pad with nop), in case the wrong operand is used
> >      +		"	.balignw 4,0x0707\n"
> >      +		"	exrl	0,0b\n"
> >      +		"	ipm	%[program_mask]\n"
> > 
> >  s390x/Makefile      |   1 +
> >  s390x/ex.c          | 172 ++++++++++++++++++++++++++++++++++++++++++++
> >  s390x/unittests.cfg |   3 +
> >  .gitlab-ci.yml      |   1 +
> >  4 files changed, 177 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..f05f8f90
> > --- /dev/null
> > +++ b/s390x/ex.c
> > @@ -0,0 +1,172 @@
> > +// 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>
> > +
> > +/*
> > + * 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 .rodata\n"
> 
> you use .text.ex_bras in the next test, why not something like that here
> (and everywhere else) too?

In the test below we branch to the code in .text.ex_bras.
In all other tests the instruction in .rodata is just an operand of the execute instruction,
and it doesn't get modified.
As for the bras test having a suffix, I guess it's pretty arbitrary, but since it's a handful
of instructions instead of just one, it felt substantial enough to warrant one.

> 
> > +		"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.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"
/*
 * 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"
> 
> please explain why briefly (i.e. if the wrong target is executed and
> the offset mismatches Bad Things™ happen)

Ok, see above.

[...]