Re: [kvm-unit-tests PATCH v1] s390x: add a pgm irq handler and a way to expect them

[David Hildenbrand <david@redhat.com>]

On 29.05.2017 18:35, Thomas Huth wrote:
> On 29.05.2017 14:17, David Hildenbrand wrote:
>> The pgm irq handler will detect unexpected pgm irqs and allows to
>> expect pgm irqs + verify that the pgm irq was triggered.
>>
>> We need "-fno-delete-null-pointer-checks", otherwise trying to access the
>> lowcore at address 0 makes GCC generate very weird code.
> 
> I wonder whether you could get rid of that by using a global variable
> for lc instead?

As far as I remember, I tried that and it didn't change a thing. Will do
a quick test.

> 
>> Add two tests to test for simple operation and addressing exception.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  Note: This will require TCG patch "target/s390x: addressing exceptions are
>>  suppressing" in order to pass.
>>
>>  Patch "[kvm-unit-tests PATCH v1] s390x: generate asm offsets for the lowcore"
>>  is requires as a prerequisite.
> 
> Maybe send them together as a series next time?

Yes, will send them out in one series for the next round.

> 
>>  lib/s390x/asm-offsets.c  |  1 +
>>  lib/s390x/asm/arch_def.h | 59 +++++++++++++++++++++++++++++++-
>>  lib/s390x/asm/irq.h      | 18 ++++++++++
>>  lib/s390x/irq.c          | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/Makefile           |  2 ++
>>  s390x/cstart64.S         | 16 +++++++++
>>  s390x/selftest.c         | 14 ++++++++
>>  7 files changed, 198 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/s390x/asm/irq.h
>>  create mode 100644 lib/s390x/irq.c
>>
>> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
>> index 28915e5..cc357e9 100644
>> --- a/lib/s390x/asm-offsets.c
>> +++ b/lib/s390x/asm-offsets.c
>> @@ -54,6 +54,7 @@ int main(void)
>>  	OFFSET(GEN_LC_PGM_NEW_PSW, lowcore, pgm_new_psw);
>>  	OFFSET(GEN_LC_MCCK_NEW_PSW, lowcore, mcck_new_psw);
>>  	OFFSET(GEN_LC_IO_NEW_PSW, lowcore, io_new_psw);
>> +	OFFSET(GEN_LC_SW_IRQ_AREA, lowcore, sw_irq_area);
>>  	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
>>  	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);
>>  	OFFSET(GEN_LC_PSW_SA, lowcore, psw_sa);
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 25c9516..c49261a 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -64,7 +64,9 @@ struct lowcore {
>>  	struct psw	pgm_new_psw;			/* 0x01d0 */
>>  	struct psw	mcck_new_psw;			/* 0x01e0 */
>>  	struct psw	io_new_psw;			/* 0x01f0 */
>> -	uint8_t		pad_0x0200[0x1200 - 0x0200];	/* 0x0200 */
>> +	/* sw definition: large enough to save all grs in irq handlers */
>> +	uint8_t		sw_irq_area[0x280 - 0x0200];	/* 0x0200 */
> 
> Since you're saving 64-bit registers into this array, I'd suggest to
> make it an array of uint64_t values instead of uint8_t.

Ack. Also added save areas for fpc + fprs.

[...]>>  /* entry point - for KVM + TCG we directly start in 64 bit mode */
>> @@ -21,6 +23,12 @@ start:
>>  	larl	%r1, initital_psw
>>  	lpswe	0(%r1)
>>  init_psw_cont:
>> +	/* setup pgm irq handler */
>> +	larl	%r1, pgm_irq_mask
>> +	stg	%r1, GEN_LC_PGM_NEW_PSW
>> +	larl	%r1, pgm_irq
>> +	stg	%r1, GEN_LC_PGM_NEW_PSW + 8
> 
> Not sure whether it's better/nicer, but I think you could also do:

I now do

larl %r1, pgm_irq_psw
mvc GEN_LC_PGM_NEW_PSW(16), 0(%r1)

> 
> 	lmg	%r0, %r1, pgm_irq_psw
> 	stmg	%r0, %r1, GEN_LC_PGM_NEW_PSW
> 	...
> pgm_irq_psw:
> 	.quad	0x0000000180000000, pgm_irq
> 
>> +	/* setup the initital PSW and enable 64bit addressing */
> 
> That comment does not really match the code below?

Yes, wonder how that slipped in.

/* setup cr0, enabling e.g. AFP-register control */

> 
>>  	larl	%r1, initital_cr0
>>  	lctlg	%c0, %c0, 0(%r1)
>>  	/* call setup() */
>> @@ -36,9 +44,17 @@ init_psw_cont:
>>  	/* call exit() */
>>  	j exit
>>  
>> +pgm_irq:
>> +	stmg	%r0, %r15, GEN_LC_SW_IRQ_AREA
>> +	brasl	%r14, handle_pgm_irq
>> +	lmg	%r0, %r15, GEN_LC_SW_IRQ_AREA
>> +	lpswe GEN_LC_PGM_OLD_PSW
> 
> Use a TAB instead of space after lpswe ?

Ack.

> 
> I also wonder whether you've got to save the floating point registers
> here, too, in case the compiler emits some floating point code in the
> interrupt handlers (since we're not using -msoftfloat anymore)?

Yes you're right. Saving/restoring fprs + fpc now. Unfortunately these
can't be saved/loaded in one shot like grs. For vector registers, there
is again an instruction to save/restore them in one shot.

> 
>>  	.align	8
>>  initital_psw:
>>  	.quad	0x0000000180000000, init_psw_cont
>> +pgm_irq_mask:
>> +	.quad	0x0000000180000000
>>  initital_cr0:
>>  	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
>>  	.quad	0x0000000000040000
>> diff --git a/s390x/selftest.c b/s390x/selftest.c
>> index 4558e47..a67b87a 100644
>> --- a/s390x/selftest.c
>> +++ b/s390x/selftest.c
>> @@ -10,6 +10,7 @@
>>   */
>>  #include <libcflat.h>
>>  #include <util.h>
>> +#include <asm/irq.h>
>>  
>>  static void test_fp(void)
>>  {
>> @@ -25,6 +26,18 @@ static void test_fp(void)
>>  	report("3.0/2.0 == 1.5", c == 1.5);
>>  }
>>  
>> +static void test_pgm_irq(void)
>> +{
>> +	expect_pgm_irq();
>> +	asm volatile(
>> +		"	.insn e,0x0001");
> 
> I'd put that into one line. And please add a comment - not everybody
> knows what opcode 0x0001 is good for.

Makes sense!

> 
>> +	report("OPERATION pgm irq", received_pgm_irq() == PGM_CODE_OPERATION);
>> +
>> +	expect_pgm_irq();
>> +	*((unsigned int*)-1) = 1;
>> +	report("ADDRESSING pgm irq", received_pgm_irq() == PGM_CODE_ADDRESSING);
>> +}
>> +
>>  int main(int argc, char**argv)
>>  {
>>  	report_prefix_push("selftest");
>> @@ -36,6 +49,7 @@ int main(int argc, char**argv)
>>  	report("argv[2] == 123", !strcmp(argv[2], "123"));
>>  
>>  	test_fp();
>> +	test_pgm_irq();
>>  
>>  	return report_summary();
>>  }
>>
> 
>  Thomas
> 

Thanks a lot Thomas!

-- 

Thanks,

David