Re: [kvm-unit-tests GIT PULL v2 11/14] s390x: Add tests for execute-type instructions

Thomas Huth
2023-04-04T15:05:58Z

On 04/04/2023 16.54, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-04-04 at 16:15 +0200, Thomas Huth wrote:
>> On 04/04/2023 13.36, Nico Boehr wrote:
>>> From: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>
>>> 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>
>>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> Link: https://lore.kernel.org/r/20230317112339.774659-1-nsg@linux.ibm.com
>>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>>> ---
>>> s390x/Makefile | 1 +
>>> s390x/ex.c | 188 ++++++++++++++++++++++++++++++++++++++++++++
>>> s390x/unittests.cfg | 3 +
>>> .gitlab-ci.yml | 1 +
>>> 4 files changed, 193 insertions(+)
>>> create mode 100644 s390x/ex.c
>>>
>>> diff --git a/s390x/Makefile b/s390x/Makefile
>>> index ab146eb..a80db53 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 0000000..dbd8030
>>> --- /dev/null
>>> +++ b/s390x/ex.c
>>> @@ -0,0 +1,188 @@
>>> +// 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>
>>> +
>>> +/*
>>> + * Accesses to the operand of execute-type instructions are instruction fetches.
>>> + * Minimum alignment is two, since the relative offset is specified by number of halfwords.
>>> + */
>>> +asm ( ".pushsection .text.exrl_targets,\"x\"\n"
>>> +" .balign 2\n"
>>> +" .popsection\n"
>>> +);
>>> +
>>> +/*
>>> + * 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 .text.exrl_targets\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();
>>> +}
>>> +
>>> +/*
>>> + * 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.exrl_targets\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"
>>> + " .endif\n"
>>
>> FWIW, this is failing with Clang 15 for me:
>>
>> s390x/ex.c:81:4: error: expected absolute expression
>> " .if (1b - 0b) != (3b - 2b)\n"
>> ^
>> <inline asm>:12:6: note: instantiated into assembly here
>> .if (1b - 0b) != (3b - 2b)
>
> Seems gcc is smarter here than clang.

Yeah, the assembler from clang is quite a bit behind on s390x ... in the
past I was only able to compile the k-u-t with Clang when using the
"-no-integrated-as" option ... but at least in the most recent version it
seems to have caught up now enough to be very close to compile it with the
built-in assembler, so it would be great to get this problem here fixed
somehow, too...

> Just deleting that .if block would work, it's basically only a static assert.
> What do you think?
> Other than that I can't think of anything.

Yes, either delete it ... or maybe you could return the two values (1b - 0b)
and (3b - 2b) as output from the asm statement and do an assert() in C instead?

Thomas