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

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

On Mon, 13 Mar 2023 23:45:33 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:


[...]

> > > +/*
> > > + * 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.
> 

we discussed this offline :)

> >   
> > > +		"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.
>  */

looks good

> > > +		"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.
> 
> [...]
> 
>