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

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

[David Hildenbrand]

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.

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.

 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 */
+	uint8_t		pad_0x0280[0x1200 - 0x0280];	/* 0x0280 */
 	uint64_t	fprs_sa[16];			/* 0x1200 */
 	uint64_t	grs_sa[16];			/* 0x1280 */
 	struct psw	psw_sa;				/* 0x1300 */
@@ -82,4 +84,59 @@ struct lowcore {
 	uint8_t		pgm_int_tdb[0x1900 - 0x1800];	/* 0x1800 */
 } __attribute__ ((__packed__));
 
+#define PGM_CODE_OPERATION		0x01
+#define PGM_CODE_PRIVILEGED_OPERATION	0x02
+#define PGM_CODE_EXECUTE		0x03
+#define PGM_CODE_PROTECTION		0x04
+#define PGM_CODE_ADDRESSING		0x05
+#define PGM_CODE_SPECIFICATION		0x06
+#define PGM_CODE_DATA			0x07
+#define PGM_CODE_FIXED_POINT_OVERFLOW	0x08
+#define PGM_CODE_FIXED_POINT_DIVIDE	0x09
+#define PGM_CODE_DECIMAL_OVERFLOW	0x0a
+#define PGM_CODE_DECIMAL_DIVIDE		0x0b
+#define PGM_CODE_HFP_EXPONENT_OVERFLOW	0x0c
+#define PGM_CODE_HFP_EXPONENT_UNDERFLOW	0x0d
+#define PGM_CODE_HFP_SIGNIFICANCE	0x0e
+#define PGM_CODE_HFP_DIVIDE		0x0f
+#define PGM_CODE_SEGMENT_TRANSLATION	0x10
+#define PGM_CODE_PAGE_TRANSLATION	0x11
+#define PGM_CODE_TRANSLATION_SPEC	0x12
+#define PGM_CODE_SPECIAL_OPERATION	0x13
+#define PGM_CODE_OPERAND		0x15
+#define PGM_CODE_TRACE_TABLE		0x16
+#define PGM_CODE_VECTOR_PROCESSING	0x1b
+#define PGM_CODE_SPACE_SWITCH_EVENT	0x1c
+#define PGM_CODE_HFP_SQUARE_ROOT	0x1d
+#define PGM_CODE_PC_TRANSLATION_SPEC	0x1f
+#define PGM_CODE_AFX_TRANSLATION	0x20
+#define PGM_CODE_ASX_TRANSLATION	0x21
+#define PGM_CODE_LX_TRANSLATION		0x22
+#define PGM_CODE_EX_TRANSLATION		0x23
+#define PGM_CODE_PRIMARY_AUTHORITY	0x24
+#define PGM_CODE_SECONDARY_AUTHORITY	0x25
+#define PGM_CODE_LFX_TRANSLATION	0x26
+#define PGM_CODE_LSX_TRANSLATION	0x27
+#define PGM_CODE_ALET_SPECIFICATION	0x28
+#define PGM_CODE_ALEN_TRANSLATION	0x29
+#define PGM_CODE_ALE_SEQUENCE		0x2a
+#define PGM_CODE_ASTE_VALIDITY		0x2b
+#define PGM_CODE_ASTE_SEQUENCE		0x2c
+#define PGM_CODE_EXTENDED_AUTHORITY	0x2d
+#define PGM_CODE_LSTE_SEQUENCE		0x2e
+#define PGM_CODE_ASTE_INSTANCE		0x2f
+#define PGM_CODE_STACK_FULL		0x30
+#define PGM_CODE_STACK_EMPTY		0x31
+#define PGM_CODE_STACK_SPECIFICATION	0x32
+#define PGM_CODE_STACK_TYPE		0x33
+#define PGM_CODE_STACK_OPERATION	0x34
+#define PGM_CODE_ASCE_TYPE		0x38
+#define PGM_CODE_REGION_FIRST_TRANS	0x39
+#define PGM_CODE_REGION_SECOND_TRANS	0x3a
+#define PGM_CODE_REGION_THIRD_TRANS	0x3b
+#define PGM_CODE_MONITOR_EVENT		0x40
+#define PGM_CODE_PER			0x80
+#define PGM_CODE_CRYPTO_OPERATION	0x119
+#define PGM_CODE_TX_ABORTED_EVENT	0x200
+
 #endif
diff --git a/lib/s390x/asm/irq.h b/lib/s390x/asm/irq.h
new file mode 100644
index 0000000..8f6013b
--- /dev/null
+++ b/lib/s390x/asm/irq.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef _ASMS390X_IRQ_H_
+#define _ASMS390X_IRQ_H_
+#include <asm/arch_def.h>
+
+void handle_pgm_irq(void);
+void expect_pgm_irq(void);
+uint16_t received_pgm_irq(void);
+
+#endif
diff --git a/lib/s390x/irq.c b/lib/s390x/irq.c
new file mode 100644
index 0000000..a54c01a
--- /dev/null
+++ b/lib/s390x/irq.c
@@ -0,0 +1,89 @@
+/*
+ * s390x irq handling
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#include <libcflat.h>
+#include <asm/irq.h>
+#include <asm/barrier.h>
+
+static bool pgm_irq_expected;
+
+void expect_pgm_irq(void)
+{
+	struct lowcore *lc = (void * ) 0;
+
+	pgm_irq_expected = true;
+	lc->pgm_int_code = 0;
+	mb();
+}
+
+uint16_t received_pgm_irq(void)
+{
+	struct lowcore *lc = (void * ) 0;
+
+	mb();
+	return lc->pgm_int_code;
+}
+
+static void fixup_pgm_irq(void)
+{
+	struct lowcore *lc = (void * ) 0;
+
+	switch (lc->pgm_int_code) {
+	case PGM_CODE_SEGMENT_TRANSLATION:
+	case PGM_CODE_PAGE_TRANSLATION:
+	case PGM_CODE_TRACE_TABLE:
+	case PGM_CODE_AFX_TRANSLATION:
+	case PGM_CODE_ASX_TRANSLATION:
+	case PGM_CODE_LX_TRANSLATION:
+	case PGM_CODE_EX_TRANSLATION:
+	case PGM_CODE_PRIMARY_AUTHORITY:
+	case PGM_CODE_SECONDARY_AUTHORITY:
+	case PGM_CODE_LFX_TRANSLATION:
+	case PGM_CODE_LSX_TRANSLATION:
+	case PGM_CODE_ALEN_TRANSLATION:
+	case PGM_CODE_ALE_SEQUENCE:
+	case PGM_CODE_ASTE_VALIDITY:
+	case PGM_CODE_ASTE_SEQUENCE:
+	case PGM_CODE_EXTENDED_AUTHORITY:
+	case PGM_CODE_LSTE_SEQUENCE:
+	case PGM_CODE_ASTE_INSTANCE:
+	case PGM_CODE_STACK_FULL:
+	case PGM_CODE_STACK_EMPTY:
+	case PGM_CODE_STACK_SPECIFICATION:
+	case PGM_CODE_STACK_TYPE:
+	case PGM_CODE_STACK_OPERATION:
+	case PGM_CODE_ASCE_TYPE:
+	case PGM_CODE_REGION_FIRST_TRANS:
+	case PGM_CODE_REGION_SECOND_TRANS:
+	case PGM_CODE_REGION_THIRD_TRANS:
+	case PGM_CODE_PER:
+	case PGM_CODE_CRYPTO_OPERATION:
+		/* The irq was nullified, the old PSW points at the
+		 * responsible instruction. Forward the PSW so we don't loop.
+		 */
+		lc->pgm_old_psw.addr += lc->pgm_int_id;
+	}
+	/* suppressed/terminated/completed point already at the next address */
+}
+
+void handle_pgm_irq(void)
+{
+	struct lowcore *lc = (void *) 0;
+
+	if (!pgm_irq_expected)
+		report_abort("Unexpected pgm irq %d at %#lx, ilen %d\n",
+			     lc->pgm_int_code, lc->pgm_old_psw.addr,
+			     lc->pgm_int_id);
+
+	pgm_irq_expected = false;
+	fixup_pgm_irq();
+}
diff --git a/s390x/Makefile b/s390x/Makefile
index 4e4b94a..712ef49 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -10,6 +10,7 @@ CFLAGS += -Wextra
 CFLAGS += -I $(SRCDIR)/lib
 CFLAGS += -O2
 CFLAGS += -march=z900
+CFLAGS += -fno-delete-null-pointer-checks
 LDFLAGS += -nostdlib
 
 # We want to keep intermediate files
@@ -23,6 +24,7 @@ cflatobjs += lib/alloc.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
 cflatobjs += lib/s390x/sclp-ascii.o
+cflatobjs += lib/s390x/irq.o
 
 OBJDIRS += lib/s390x
 
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 28cd59d..6bed38f 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -10,6 +10,8 @@
  * This code is free software; you can redistribute it and/or modify it
  * under the terms of the GNU Library General Public License version 2.
  */
+#include <asm/asm-offsets.h>
+
 .section .init
 
 /* 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
+	/* setup the initital PSW and enable 64bit addressing */
 	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
+
 	.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");
+	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();
 }
-- 
2.9.3

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

[David Hildenbrand]

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.
> 
> 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.
> 
>  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 */
> +	uint8_t		pad_0x0280[0x1200 - 0x0280];	/* 0x0280 */
>  	uint64_t	fprs_sa[16];			/* 0x1200 */
>  	uint64_t	grs_sa[16];			/* 0x1280 */
>  	struct psw	psw_sa;				/* 0x1300 */
> @@ -82,4 +84,59 @@ struct lowcore {
>  	uint8_t		pgm_int_tdb[0x1900 - 0x1800];	/* 0x1800 */
>  } __attribute__ ((__packed__));
>  
> +#define PGM_CODE_OPERATION		0x01
> +#define PGM_CODE_PRIVILEGED_OPERATION	0x02
> +#define PGM_CODE_EXECUTE		0x03
> +#define PGM_CODE_PROTECTION		0x04
> +#define PGM_CODE_ADDRESSING		0x05
> +#define PGM_CODE_SPECIFICATION		0x06
> +#define PGM_CODE_DATA			0x07
> +#define PGM_CODE_FIXED_POINT_OVERFLOW	0x08
> +#define PGM_CODE_FIXED_POINT_DIVIDE	0x09
> +#define PGM_CODE_DECIMAL_OVERFLOW	0x0a
> +#define PGM_CODE_DECIMAL_DIVIDE		0x0b
> +#define PGM_CODE_HFP_EXPONENT_OVERFLOW	0x0c
> +#define PGM_CODE_HFP_EXPONENT_UNDERFLOW	0x0d
> +#define PGM_CODE_HFP_SIGNIFICANCE	0x0e
> +#define PGM_CODE_HFP_DIVIDE		0x0f
> +#define PGM_CODE_SEGMENT_TRANSLATION	0x10
> +#define PGM_CODE_PAGE_TRANSLATION	0x11
> +#define PGM_CODE_TRANSLATION_SPEC	0x12
> +#define PGM_CODE_SPECIAL_OPERATION	0x13
> +#define PGM_CODE_OPERAND		0x15
> +#define PGM_CODE_TRACE_TABLE		0x16
> +#define PGM_CODE_VECTOR_PROCESSING	0x1b
> +#define PGM_CODE_SPACE_SWITCH_EVENT	0x1c
> +#define PGM_CODE_HFP_SQUARE_ROOT	0x1d
> +#define PGM_CODE_PC_TRANSLATION_SPEC	0x1f
> +#define PGM_CODE_AFX_TRANSLATION	0x20
> +#define PGM_CODE_ASX_TRANSLATION	0x21
> +#define PGM_CODE_LX_TRANSLATION		0x22
> +#define PGM_CODE_EX_TRANSLATION		0x23
> +#define PGM_CODE_PRIMARY_AUTHORITY	0x24
> +#define PGM_CODE_SECONDARY_AUTHORITY	0x25
> +#define PGM_CODE_LFX_TRANSLATION	0x26
> +#define PGM_CODE_LSX_TRANSLATION	0x27
> +#define PGM_CODE_ALET_SPECIFICATION	0x28
> +#define PGM_CODE_ALEN_TRANSLATION	0x29
> +#define PGM_CODE_ALE_SEQUENCE		0x2a
> +#define PGM_CODE_ASTE_VALIDITY		0x2b
> +#define PGM_CODE_ASTE_SEQUENCE		0x2c
> +#define PGM_CODE_EXTENDED_AUTHORITY	0x2d
> +#define PGM_CODE_LSTE_SEQUENCE		0x2e
> +#define PGM_CODE_ASTE_INSTANCE		0x2f
> +#define PGM_CODE_STACK_FULL		0x30
> +#define PGM_CODE_STACK_EMPTY		0x31
> +#define PGM_CODE_STACK_SPECIFICATION	0x32
> +#define PGM_CODE_STACK_TYPE		0x33
> +#define PGM_CODE_STACK_OPERATION	0x34
> +#define PGM_CODE_ASCE_TYPE		0x38
> +#define PGM_CODE_REGION_FIRST_TRANS	0x39
> +#define PGM_CODE_REGION_SECOND_TRANS	0x3a
> +#define PGM_CODE_REGION_THIRD_TRANS	0x3b
> +#define PGM_CODE_MONITOR_EVENT		0x40
> +#define PGM_CODE_PER			0x80
> +#define PGM_CODE_CRYPTO_OPERATION	0x119
> +#define PGM_CODE_TX_ABORTED_EVENT	0x200
> +
>  #endif
> diff --git a/lib/s390x/asm/irq.h b/lib/s390x/asm/irq.h
> new file mode 100644
> index 0000000..8f6013b
> --- /dev/null
> +++ b/lib/s390x/asm/irq.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#ifndef _ASMS390X_IRQ_H_
> +#define _ASMS390X_IRQ_H_
> +#include <asm/arch_def.h>
> +
> +void handle_pgm_irq(void);
> +void expect_pgm_irq(void);
> +uint16_t received_pgm_irq(void);
> +
> +#endif
> diff --git a/lib/s390x/irq.c b/lib/s390x/irq.c
> new file mode 100644
> index 0000000..a54c01a
> --- /dev/null
> +++ b/lib/s390x/irq.c
> @@ -0,0 +1,89 @@
> +/*
> + * s390x irq handling
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/irq.h>
> +#include <asm/barrier.h>
> +
> +static bool pgm_irq_expected;
> +
> +void expect_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	pgm_irq_expected = true;
> +	lc->pgm_int_code = 0;
> +	mb();
> +}
> +
> +uint16_t received_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	mb();
> +	return lc->pgm_int_code;
> +}
> +
> +static void fixup_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	switch (lc->pgm_int_code) {
> +	case PGM_CODE_SEGMENT_TRANSLATION:
> +	case PGM_CODE_PAGE_TRANSLATION:
> +	case PGM_CODE_TRACE_TABLE:
> +	case PGM_CODE_AFX_TRANSLATION:
> +	case PGM_CODE_ASX_TRANSLATION:
> +	case PGM_CODE_LX_TRANSLATION:
> +	case PGM_CODE_EX_TRANSLATION:
> +	case PGM_CODE_PRIMARY_AUTHORITY:
> +	case PGM_CODE_SECONDARY_AUTHORITY:
> +	case PGM_CODE_LFX_TRANSLATION:
> +	case PGM_CODE_LSX_TRANSLATION:
> +	case PGM_CODE_ALEN_TRANSLATION:
> +	case PGM_CODE_ALE_SEQUENCE:
> +	case PGM_CODE_ASTE_VALIDITY:
> +	case PGM_CODE_ASTE_SEQUENCE:
> +	case PGM_CODE_EXTENDED_AUTHORITY:
> +	case PGM_CODE_LSTE_SEQUENCE:
> +	case PGM_CODE_ASTE_INSTANCE:
> +	case PGM_CODE_STACK_FULL:
> +	case PGM_CODE_STACK_EMPTY:
> +	case PGM_CODE_STACK_SPECIFICATION:
> +	case PGM_CODE_STACK_TYPE:
> +	case PGM_CODE_STACK_OPERATION:
> +	case PGM_CODE_ASCE_TYPE:
> +	case PGM_CODE_REGION_FIRST_TRANS:
> +	case PGM_CODE_REGION_SECOND_TRANS:
> +	case PGM_CODE_REGION_THIRD_TRANS:
> +	case PGM_CODE_PER:
> +	case PGM_CODE_CRYPTO_OPERATION:
> +		/* The irq was nullified, the old PSW points at the
> +		 * responsible instruction. Forward the PSW so we don't loop.
> +		 */
> +		lc->pgm_old_psw.addr += lc->pgm_int_id;
> +	}
> +	/* suppressed/terminated/completed point already at the next address */
> +}
> +
> +void handle_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void *) 0;
> +
> +	if (!pgm_irq_expected)
> +		report_abort("Unexpected pgm irq %d at %#lx, ilen %d\n",
> +			     lc->pgm_int_code, lc->pgm_old_psw.addr,
> +			     lc->pgm_int_id);
> +
> +	pgm_irq_expected = false;
> +	fixup_pgm_irq();
> +}
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 4e4b94a..712ef49 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -10,6 +10,7 @@ CFLAGS += -Wextra
>  CFLAGS += -I $(SRCDIR)/lib
>  CFLAGS += -O2
>  CFLAGS += -march=z900
> +CFLAGS += -fno-delete-null-pointer-checks
>  LDFLAGS += -nostdlib
>  
>  # We want to keep intermediate files
> @@ -23,6 +24,7 @@ cflatobjs += lib/alloc.o
>  cflatobjs += lib/s390x/io.o
>  cflatobjs += lib/s390x/stack.o
>  cflatobjs += lib/s390x/sclp-ascii.o
> +cflatobjs += lib/s390x/irq.o
>  
>  OBJDIRS += lib/s390x
>  
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 28cd59d..6bed38f 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -10,6 +10,8 @@
>   * This code is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU Library General Public License version 2.
>   */
> +#include <asm/asm-offsets.h>
> +
>  .section .init
>  
>  /* 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

So, apparently, TCG does not check for specification exceptions.

Here, a
lg      %r1, 0(%r1)

is missing.

> +	stg	%r1, GEN_LC_PGM_NEW_PSW
> +	larl	%r1, pgm_irq
> +	stg	%r1, GEN_LC_PGM_NEW_PSW + 8
> +	/* setup the initital PSW and enable 64bit addressing */
>  	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
> +
>  	.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");
> +	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();
>  }
> 


-- 

Thanks,

David

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

[Thomas Huth]

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?

> 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?

>  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.

> +	uint8_t		pad_0x0280[0x1200 - 0x0280];	/* 0x0280 */
>  	uint64_t	fprs_sa[16];			/* 0x1200 */
>  	uint64_t	grs_sa[16];			/* 0x1280 */
>  	struct psw	psw_sa;				/* 0x1300 */
> @@ -82,4 +84,59 @@ struct lowcore {
>  	uint8_t		pgm_int_tdb[0x1900 - 0x1800];	/* 0x1800 */
>  } __attribute__ ((__packed__));
>  
> +#define PGM_CODE_OPERATION		0x01
> +#define PGM_CODE_PRIVILEGED_OPERATION	0x02
> +#define PGM_CODE_EXECUTE		0x03
> +#define PGM_CODE_PROTECTION		0x04
> +#define PGM_CODE_ADDRESSING		0x05
> +#define PGM_CODE_SPECIFICATION		0x06
> +#define PGM_CODE_DATA			0x07
> +#define PGM_CODE_FIXED_POINT_OVERFLOW	0x08
> +#define PGM_CODE_FIXED_POINT_DIVIDE	0x09
> +#define PGM_CODE_DECIMAL_OVERFLOW	0x0a
> +#define PGM_CODE_DECIMAL_DIVIDE		0x0b
> +#define PGM_CODE_HFP_EXPONENT_OVERFLOW	0x0c
> +#define PGM_CODE_HFP_EXPONENT_UNDERFLOW	0x0d
> +#define PGM_CODE_HFP_SIGNIFICANCE	0x0e
> +#define PGM_CODE_HFP_DIVIDE		0x0f
> +#define PGM_CODE_SEGMENT_TRANSLATION	0x10
> +#define PGM_CODE_PAGE_TRANSLATION	0x11
> +#define PGM_CODE_TRANSLATION_SPEC	0x12
> +#define PGM_CODE_SPECIAL_OPERATION	0x13
> +#define PGM_CODE_OPERAND		0x15
> +#define PGM_CODE_TRACE_TABLE		0x16
> +#define PGM_CODE_VECTOR_PROCESSING	0x1b
> +#define PGM_CODE_SPACE_SWITCH_EVENT	0x1c
> +#define PGM_CODE_HFP_SQUARE_ROOT	0x1d
> +#define PGM_CODE_PC_TRANSLATION_SPEC	0x1f
> +#define PGM_CODE_AFX_TRANSLATION	0x20
> +#define PGM_CODE_ASX_TRANSLATION	0x21
> +#define PGM_CODE_LX_TRANSLATION		0x22
> +#define PGM_CODE_EX_TRANSLATION		0x23
> +#define PGM_CODE_PRIMARY_AUTHORITY	0x24
> +#define PGM_CODE_SECONDARY_AUTHORITY	0x25
> +#define PGM_CODE_LFX_TRANSLATION	0x26
> +#define PGM_CODE_LSX_TRANSLATION	0x27
> +#define PGM_CODE_ALET_SPECIFICATION	0x28
> +#define PGM_CODE_ALEN_TRANSLATION	0x29
> +#define PGM_CODE_ALE_SEQUENCE		0x2a
> +#define PGM_CODE_ASTE_VALIDITY		0x2b
> +#define PGM_CODE_ASTE_SEQUENCE		0x2c
> +#define PGM_CODE_EXTENDED_AUTHORITY	0x2d
> +#define PGM_CODE_LSTE_SEQUENCE		0x2e
> +#define PGM_CODE_ASTE_INSTANCE		0x2f
> +#define PGM_CODE_STACK_FULL		0x30
> +#define PGM_CODE_STACK_EMPTY		0x31
> +#define PGM_CODE_STACK_SPECIFICATION	0x32
> +#define PGM_CODE_STACK_TYPE		0x33
> +#define PGM_CODE_STACK_OPERATION	0x34
> +#define PGM_CODE_ASCE_TYPE		0x38
> +#define PGM_CODE_REGION_FIRST_TRANS	0x39
> +#define PGM_CODE_REGION_SECOND_TRANS	0x3a
> +#define PGM_CODE_REGION_THIRD_TRANS	0x3b
> +#define PGM_CODE_MONITOR_EVENT		0x40
> +#define PGM_CODE_PER			0x80
> +#define PGM_CODE_CRYPTO_OPERATION	0x119
> +#define PGM_CODE_TX_ABORTED_EVENT	0x200
> +
>  #endif
> diff --git a/lib/s390x/asm/irq.h b/lib/s390x/asm/irq.h
> new file mode 100644
> index 0000000..8f6013b
> --- /dev/null
> +++ b/lib/s390x/asm/irq.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#ifndef _ASMS390X_IRQ_H_
> +#define _ASMS390X_IRQ_H_
> +#include <asm/arch_def.h>
> +
> +void handle_pgm_irq(void);
> +void expect_pgm_irq(void);
> +uint16_t received_pgm_irq(void);
> +
> +#endif
> diff --git a/lib/s390x/irq.c b/lib/s390x/irq.c
> new file mode 100644
> index 0000000..a54c01a
> --- /dev/null
> +++ b/lib/s390x/irq.c
> @@ -0,0 +1,89 @@
> +/*
> + * s390x irq handling
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/irq.h>
> +#include <asm/barrier.h>
> +
> +static bool pgm_irq_expected;
> +
> +void expect_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	pgm_irq_expected = true;
> +	lc->pgm_int_code = 0;
> +	mb();
> +}
> +
> +uint16_t received_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	mb();
> +	return lc->pgm_int_code;
> +}

I'd maybe name this "get_pgm_int_code()" instead (but that's just a
matter of taste)

> +static void fixup_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void * ) 0;
> +
> +	switch (lc->pgm_int_code) {
> +	case PGM_CODE_SEGMENT_TRANSLATION:
> +	case PGM_CODE_PAGE_TRANSLATION:
> +	case PGM_CODE_TRACE_TABLE:
> +	case PGM_CODE_AFX_TRANSLATION:
> +	case PGM_CODE_ASX_TRANSLATION:
> +	case PGM_CODE_LX_TRANSLATION:
> +	case PGM_CODE_EX_TRANSLATION:
> +	case PGM_CODE_PRIMARY_AUTHORITY:
> +	case PGM_CODE_SECONDARY_AUTHORITY:
> +	case PGM_CODE_LFX_TRANSLATION:
> +	case PGM_CODE_LSX_TRANSLATION:
> +	case PGM_CODE_ALEN_TRANSLATION:
> +	case PGM_CODE_ALE_SEQUENCE:
> +	case PGM_CODE_ASTE_VALIDITY:
> +	case PGM_CODE_ASTE_SEQUENCE:
> +	case PGM_CODE_EXTENDED_AUTHORITY:
> +	case PGM_CODE_LSTE_SEQUENCE:
> +	case PGM_CODE_ASTE_INSTANCE:
> +	case PGM_CODE_STACK_FULL:
> +	case PGM_CODE_STACK_EMPTY:
> +	case PGM_CODE_STACK_SPECIFICATION:
> +	case PGM_CODE_STACK_TYPE:
> +	case PGM_CODE_STACK_OPERATION:
> +	case PGM_CODE_ASCE_TYPE:
> +	case PGM_CODE_REGION_FIRST_TRANS:
> +	case PGM_CODE_REGION_SECOND_TRANS:
> +	case PGM_CODE_REGION_THIRD_TRANS:
> +	case PGM_CODE_PER:
> +	case PGM_CODE_CRYPTO_OPERATION:
> +		/* The irq was nullified, the old PSW points at the
> +		 * responsible instruction. Forward the PSW so we don't loop.
> +		 */
> +		lc->pgm_old_psw.addr += lc->pgm_int_id;
> +	}
> +	/* suppressed/terminated/completed point already at the next address */
> +}
> +
> +void handle_pgm_irq(void)
> +{
> +	struct lowcore *lc = (void *) 0;
> +
> +	if (!pgm_irq_expected)
> +		report_abort("Unexpected pgm irq %d at %#lx, ilen %d\n",
> +			     lc->pgm_int_code, lc->pgm_old_psw.addr,
> +			     lc->pgm_int_id);
> +
> +	pgm_irq_expected = false;
> +	fixup_pgm_irq();
> +}
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 4e4b94a..712ef49 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -10,6 +10,7 @@ CFLAGS += -Wextra
>  CFLAGS += -I $(SRCDIR)/lib
>  CFLAGS += -O2
>  CFLAGS += -march=z900
> +CFLAGS += -fno-delete-null-pointer-checks
>  LDFLAGS += -nostdlib
>  
>  # We want to keep intermediate files
> @@ -23,6 +24,7 @@ cflatobjs += lib/alloc.o
>  cflatobjs += lib/s390x/io.o
>  cflatobjs += lib/s390x/stack.o
>  cflatobjs += lib/s390x/sclp-ascii.o
> +cflatobjs += lib/s390x/irq.o
>  
>  OBJDIRS += lib/s390x
>  
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 28cd59d..6bed38f 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -10,6 +10,8 @@
>   * This code is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU Library General Public License version 2.
>   */
> +#include <asm/asm-offsets.h>
> +
>  .section .init
>  
>  /* 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:

	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?

>  	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 ?

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)?

>  	.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.

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

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

[David Hildenbrand]

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

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

[David Hildenbrand]

On 30.05.2017 10:03, David Hildenbrand wrote:
> 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.

It works as long as that global variable is not static, because then GCC
must assume that any other code could set it to !0.

-- 

Thanks,

David

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

[Paolo Bonzini]

On 30/05/2017 10:03, David Hildenbrand wrote:
>>> 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.
> 

-fno-delete-null-pointer-checks is the right thing to do, trying to hide
undefined behavior from the compiler leads to misery. :)

Paolo