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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

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?

> +		"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"
> +		"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)

> +		"	.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();
> +}
> +
> +/*
> + * LOAD ADDRESS RELATIVE LONG.
> + * If it is the target of an execute-type instruction, the address is relative
> + * to the LARL.
> + */
> +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.
> + */
> +static void test_llgfrl(void)
> +{
> +	uint64_t target, value;
> +
> +	report_prefix_push("LLGFRL");
> +	asm volatile ( ".pushsection .rodata\n"
> +		"	.balign	4\n"

explain the alignment (like you did in the test below)

> +		"0:	llgfrl	%[value],0\n"
> +		"	.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)
> +	);
> +
> +	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"
> +		 //operand of crl must be word aligned
> +		 "	.balign	4\n"
> +		"0:	crl	%[crl_word],0\n"
> +		"	.popsection\n"
> +
> +		"	lrl	%[crl_word],0b\n"
> +		//align (pad with nop), in case the wrong 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