[kvm-unit-tests PATCH 0/5] s390x: Add SIE library and simple test

[kvm-unit-tests PATCH 0/5] s390x: Add SIE library and simple test

[Janosch Frank]

This is the absolute minimum needed to run VMs inside the KVM Unit
Tests. It's more of a base for other tests that I can't (yet) publish
than an addition of tests that check KVM functionality. However, I
wanted to decrease the number of WIP patches in my private
branch. Once the library is available maybe others will come and
extend the SIE test itself.

Yes, I have added VM management functionality like VM create/destroy,
etc but as it is not needed right now, I'd like to exclude it from
this patch set for now.

Gitlab:
https://gitlab.com/frankja/kvm-unit-tests/-/tree/sie

CI:
https://gitlab.com/frankja/kvm-unit-tests/-/pipelines/217336822

Janosch Frank (5):
  s390x: Add test_bit to library
  s390x: Consolidate sclp read info
  s390x: SCLP feature checking
  s390x: sie: Add SIE to lib
  s390x: sie: Add first SIE test

 lib/s390x/asm-offsets.c  |  13 +++
 lib/s390x/asm/arch_def.h |   7 ++
 lib/s390x/asm/bitops.h   |  16 ++++
 lib/s390x/asm/facility.h |   3 +-
 lib/s390x/interrupt.c    |   7 ++
 lib/s390x/io.c           |   2 +
 lib/s390x/sclp.c         |  48 ++++++++--
 lib/s390x/sclp.h         |  18 ++++
 lib/s390x/sie.h          | 197 +++++++++++++++++++++++++++++++++++++++
 lib/s390x/smp.c          |  23 ++---
 s390x/Makefile           |   1 +
 s390x/cstart64.S         |  56 +++++++++++
 s390x/sie.c              | 125 +++++++++++++++++++++++++
 13 files changed, 495 insertions(+), 21 deletions(-)
 create mode 100644 lib/s390x/sie.h
 create mode 100644 s390x/sie.c

-- 
2.25.1

[kvm-unit-tests PATCH 1/5] s390x: Add test_bit to library

[Janosch Frank]

Query/feature bits are commonly tested via MSB bit numbers on
s390. Let's add test bit functions, so we don't need to copy code to
test query bits.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/bitops.h   | 16 ++++++++++++++++
 lib/s390x/asm/facility.h |  3 ++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
index e7cdda9..a272dd7 100644
--- a/lib/s390x/asm/bitops.h
+++ b/lib/s390x/asm/bitops.h
@@ -7,4 +7,20 @@
 
 #define BITS_PER_LONG	64
 
+static inline bool test_bit(unsigned long nr,
+			    const volatile unsigned long *ptr)
+{
+	const volatile unsigned char *addr;
+
+	addr = ((const volatile unsigned char *)ptr);
+	addr += (nr ^ (BITS_PER_LONG - 8)) >> 3;
+	return (*addr >> (nr & 7)) & 1;
+}
+
+static inline bool test_bit_inv(unsigned long nr,
+				const volatile unsigned long *ptr)
+{
+	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
+}
+
 #endif
diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
index def2705..5593c2d 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -13,13 +13,14 @@
 #include <libcflat.h>
 #include <asm/facility.h>
 #include <asm/arch_def.h>
+#include <bitops.h>
 
 #define NB_STFL_DOUBLEWORDS 32
 extern uint64_t stfl_doublewords[];
 
 static inline bool test_facility(int nr)
 {
-	return stfl_doublewords[nr / 64] & (0x8000000000000000UL >> (nr % 64));
+	return test_bit_inv(nr, stfl_doublewords);
 }
 
 static inline void stfl(void)
-- 
2.25.1

[kvm-unit-tests PATCH 2/5] s390x: Consolidate sclp read info

[Janosch Frank]

Let's only read the information once and pass a pointer to it instead
of calling sclp multiple times.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/io.c   |  1 +
 lib/s390x/sclp.c | 29 +++++++++++++++++++++++------
 lib/s390x/sclp.h |  3 +++
 lib/s390x/smp.c  | 23 +++++++++--------------
 4 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index c0f0bf7..e19a1f3 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -36,6 +36,7 @@ void setup(void)
 {
 	setup_args_progname(ipl_args);
 	setup_facilities();
+	sclp_read_info();
 	sclp_console_setup();
 	sclp_memory_setup();
 	smp_setup();
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 4054d0e..ea6324e 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -25,6 +25,8 @@ extern unsigned long stacktop;
 static uint64_t storage_increment_size;
 static uint64_t max_ram_size;
 static uint64_t ram_size;
+char _read_info[PAGE_SIZE] __attribute__((__aligned__(4096)));
+static ReadInfo *read_info;
 
 char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
 static volatile bool sclp_busy;
@@ -110,6 +112,22 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
 	report_abort("READ_SCP_INFO failed");
 }
 
+void sclp_read_info(void)
+{
+	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
+	read_info = (ReadInfo *)_read_info;
+}
+
+int sclp_get_cpu_num(void)
+{
+	return read_info->entries_cpu;
+}
+
+CPUEntry *sclp_get_cpu_entries(void)
+{
+	return (void *)read_info + read_info->offset_cpu;
+}
+
 /* Perform service call. Return 0 on success, non-zero otherwise. */
 int sclp_service_call(unsigned int command, void *sccb)
 {
@@ -127,23 +145,22 @@ int sclp_service_call(unsigned int command, void *sccb)
 
 void sclp_memory_setup(void)
 {
-	ReadInfo *ri = (void *)_sccb;
 	uint64_t rnmax, rnsize;
 	int cc;
 
-	sclp_read_scp_info(ri, SCCB_SIZE);
+	assert(read_info);
 
 	/* calculate the storage increment size */
-	rnsize = ri->rnsize;
+	rnsize = read_info->rnsize;
 	if (!rnsize) {
-		rnsize = ri->rnsize2;
+		rnsize = read_info->rnsize2;
 	}
 	storage_increment_size = rnsize << 20;
 
 	/* calculate the maximum memory size */
-	rnmax = ri->rnmax;
+	rnmax = read_info->rnmax;
 	if (!rnmax) {
-		rnmax = ri->rnmax2;
+		rnmax = read_info->rnmax2;
 	}
 	max_ram_size = rnmax * storage_increment_size;
 
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 675f07e..6620531 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -271,6 +271,9 @@ void sclp_wait_busy(void);
 void sclp_mark_busy(void);
 void sclp_console_setup(void);
 void sclp_print(const char *str);
+void sclp_read_info(void);
+int sclp_get_cpu_num(void);
+CPUEntry *sclp_get_cpu_entries(void);
 int sclp_service_call(unsigned int command, void *sccb);
 void sclp_memory_setup(void);
 uint64_t get_ram_size(void);
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 2860e9c..6754061 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -34,8 +34,7 @@ extern void smp_cpu_setup_state(void);
 
 int smp_query_num_cpus(void)
 {
-	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
-	return info->nr_configured;
+	return sclp_get_cpu_num();
 }
 
 struct cpu *smp_cpu_from_addr(uint16_t addr)
@@ -245,22 +244,18 @@ extern uint64_t *stackptr;
 void smp_setup(void)
 {
 	int i = 0;
+	int num = smp_query_num_cpus();
 	unsigned short cpu0_addr = stap();
-	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
+	struct CPUEntry *entry = sclp_get_cpu_entries();
 
-	spin_lock(&lock);
-	sclp_mark_busy();
-	info->h.length = PAGE_SIZE;
-	sclp_service_call(SCLP_READ_CPU_INFO, cpu_info_buffer);
+	if (num > 1)
+		printf("SMP: Initializing, found %d cpus\n", num);
 
-	if (smp_query_num_cpus() > 1)
-		printf("SMP: Initializing, found %d cpus\n", info->nr_configured);
-
-	cpus = calloc(info->nr_configured, sizeof(cpus));
-	for (i = 0; i < info->nr_configured; i++) {
-		cpus[i].addr = info->entries[i].address;
+	cpus = calloc(num, sizeof(cpus));
+	for (i = 0; i < num; i++) {
+		cpus[i].addr = entry[i].address;
 		cpus[i].active = false;
-		if (info->entries[i].address == cpu0_addr) {
+		if (entry[i].address == cpu0_addr) {
 			cpu0 = &cpus[i];
 			cpu0->stack = stackptr;
 			cpu0->lowcore = (void *)0;
-- 
2.25.1

[kvm-unit-tests PATCH 3/5] s390x: SCLP feature checking

[Janosch Frank]

Availability of SIE is announced via a feature bit in a SCLP info CPU
entry. Let's add a framework that allows us to easily check for such
facilities.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/io.c   |  1 +
 lib/s390x/sclp.c | 19 +++++++++++++++++++
 lib/s390x/sclp.h | 15 +++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index e19a1f3..e843601 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -37,6 +37,7 @@ void setup(void)
 	setup_args_progname(ipl_args);
 	setup_facilities();
 	sclp_read_info();
+	sclp_facilities_setup();
 	sclp_console_setup();
 	sclp_memory_setup();
 	smp_setup();
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index ea6324e..599e022 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -11,6 +11,7 @@
  */
 
 #include <libcflat.h>
+#include <bitops.h>
 #include <asm/page.h>
 #include <asm/arch_def.h>
 #include <asm/interrupt.h>
@@ -27,6 +28,7 @@ static uint64_t max_ram_size;
 static uint64_t ram_size;
 char _read_info[PAGE_SIZE] __attribute__((__aligned__(4096)));
 static ReadInfo *read_info;
+struct sclp_facilities sclp_facilities;
 
 char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
 static volatile bool sclp_busy;
@@ -128,6 +130,23 @@ CPUEntry *sclp_get_cpu_entries(void)
 	return (void *)read_info + read_info->offset_cpu;
 }
 
+void sclp_facilities_setup(void)
+{
+	unsigned short cpu0_addr = stap();
+	CPUEntry *cpu;
+	int i;
+
+	assert(read_info);
+
+	cpu = (void *)read_info + read_info->offset_cpu;
+	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
+		if (cpu->address == cpu0_addr) {
+			sclp_facilities.has_sief2 = test_bit_inv(SCLP_CPU_FEATURE_SIEF2_BIT, (void *)&cpu->address);
+			break;
+		}
+	}
+}
+
 /* Perform service call. Return 0 on success, non-zero otherwise. */
 int sclp_service_call(unsigned int command, void *sccb)
 {
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 6620531..bcc9f4b 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -101,6 +101,20 @@ typedef struct CPUEntry {
     uint8_t reserved1;
 } __attribute__((packed)) CPUEntry;
 
+extern struct sclp_facilities sclp_facilities;
+
+struct sclp_facilities {
+	u64 has_sief2 : 1;
+};
+
+/*
+ * test_bit() uses unsigned long ptrs so we give it the ptr to the
+ * address member and offset bits by 16.
+ */
+enum sclp_cpu_feature_bit {
+	SCLP_CPU_FEATURE_SIEF2_BIT = 16 + 4,
+};
+
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
@@ -274,6 +288,7 @@ void sclp_print(const char *str);
 void sclp_read_info(void);
 int sclp_get_cpu_num(void);
 CPUEntry *sclp_get_cpu_entries(void);
+void sclp_facilities_setup(void);
 int sclp_service_call(unsigned int command, void *sccb);
 void sclp_memory_setup(void);
 uint64_t get_ram_size(void);
-- 
2.25.1

[kvm-unit-tests PATCH 4/5] s390x: sie: Add SIE to lib

[Janosch Frank]

This commit adds the definition of the SIE control block struct and
the assembly to execute SIE and save/restore guest registers.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm-offsets.c  |  13 +++
 lib/s390x/asm/arch_def.h |   7 ++
 lib/s390x/interrupt.c    |   7 ++
 lib/s390x/sie.h          | 197 +++++++++++++++++++++++++++++++++++++++
 s390x/cstart64.S         |  56 +++++++++++
 5 files changed, 280 insertions(+)
 create mode 100644 lib/s390x/sie.h

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index 61d2658..1ebfc8f 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -10,6 +10,7 @@
 #include <libcflat.h>
 #include <kbuild.h>
 #include <asm/arch_def.h>
+#include <sie.h>
 
 int main(void)
 {
@@ -71,6 +72,18 @@ int main(void)
 	OFFSET(GEN_LC_ARS_SA, lowcore, ars_sa);
 	OFFSET(GEN_LC_CRS_SA, lowcore, crs_sa);
 	OFFSET(GEN_LC_PGM_INT_TDB, lowcore, pgm_int_tdb);
+	OFFSET(__SF_GPRS, stack_frame, gprs);
+	OFFSET(__SF_SIE_CONTROL, stack_frame, empty1[0]);
+	OFFSET(__SF_SIE_SAVEAREA, stack_frame, empty1[1]);
+	OFFSET(__SF_SIE_REASON, stack_frame, empty1[2]);
+	OFFSET(__SF_SIE_FLAGS, stack_frame, empty1[3]);
+	OFFSET(SIE_SAVEAREA_HOST_GRS, vm_save_area, host.grs[0]);
+	OFFSET(SIE_SAVEAREA_HOST_FPRS, vm_save_area, host.fprs[0]);
+	OFFSET(SIE_SAVEAREA_HOST_FPC, vm_save_area, host.fpc);
+	OFFSET(SIE_SAVEAREA_GUEST_GRS, vm_save_area, guest.grs[0]);
+	OFFSET(SIE_SAVEAREA_GUEST_FPRS, vm_save_area, guest.fprs[0]);
+	OFFSET(SIE_SAVEAREA_GUEST_FPC, vm_save_area, guest.fpc);
+
 
 	return 0;
 }
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index edc06ef..2cd3fc7 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -10,6 +10,13 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
+struct stack_frame {
+	unsigned long back_chain;
+	unsigned long empty1[5];
+	unsigned long gprs[10];
+	unsigned int  empty2[8];
+};
+
 struct psw {
 	uint64_t	mask;
 	uint64_t	addr;
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index a074505..4b9a640 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -13,6 +13,7 @@
 #include <asm/barrier.h>
 #include <sclp.h>
 #include <interrupt.h>
+#include <sie.h>
 
 static bool pgm_int_expected;
 static bool ext_int_expected;
@@ -59,6 +60,12 @@ void register_pgm_cleanup_func(void (*f)(void))
 
 static void fixup_pgm_int(void)
 {
+	/* If we have an error on SIE we directly move to sie_exit */
+	if (lc->pgm_old_psw.addr >= (uint64_t)&sie_entry &&
+	    lc->pgm_old_psw.addr <= (uint64_t)&sie_entry + 10) {
+		lc->pgm_old_psw.addr = (uint64_t)&sie_exit;
+	}
+
 	switch (lc->pgm_int_code) {
 	case PGM_INT_CODE_PRIVILEGED_OPERATION:
 		/* Normal operation is in supervisor state, so this exception
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
new file mode 100644
index 0000000..b00bdf4
--- /dev/null
+++ b/lib/s390x/sie.h
@@ -0,0 +1,197 @@
+#ifndef SIE_H
+#define SIE_H
+
+#define CPUSTAT_STOPPED    0x80000000
+#define CPUSTAT_WAIT       0x10000000
+#define CPUSTAT_ECALL_PEND 0x08000000
+#define CPUSTAT_STOP_INT   0x04000000
+#define CPUSTAT_IO_INT     0x02000000
+#define CPUSTAT_EXT_INT    0x01000000
+#define CPUSTAT_RUNNING    0x00800000
+#define CPUSTAT_RETAINED   0x00400000
+#define CPUSTAT_TIMING_SUB 0x00020000
+#define CPUSTAT_SIE_SUB    0x00010000
+#define CPUSTAT_RRF        0x00008000
+#define CPUSTAT_SLSV       0x00004000
+#define CPUSTAT_SLSR       0x00002000
+#define CPUSTAT_ZARCH      0x00000800
+#define CPUSTAT_MCDS       0x00000100
+#define CPUSTAT_KSS        0x00000200
+#define CPUSTAT_SM         0x00000080
+#define CPUSTAT_IBS        0x00000040
+#define CPUSTAT_GED2       0x00000010
+#define CPUSTAT_G          0x00000008
+#define CPUSTAT_GED        0x00000004
+#define CPUSTAT_J          0x00000002
+#define CPUSTAT_P          0x00000001
+
+struct kvm_s390_sie_block {
+	uint32_t 	cpuflags;		/* 0x0000 */
+	uint32_t : 1;			/* 0x0004 */
+	uint32_t 	prefix : 18;
+	uint32_t : 1;
+	uint32_t 	ibc : 12;
+	uint8_t		reserved08[4];		/* 0x0008 */
+#define PROG_IN_SIE (1<<0)
+	uint32_t	prog0c;			/* 0x000c */
+	uint8_t		reserved10[16];		/* 0x0010 */
+#define PROG_BLOCK_SIE	(1<<0)
+#define PROG_REQUEST	(1<<1)
+	uint32_t 	prog20;		/* 0x0020 */
+	uint8_t		reserved24[4];		/* 0x0024 */
+	uint64_t	cputm;			/* 0x0028 */
+	uint64_t	ckc;			/* 0x0030 */
+	uint64_t	epoch;			/* 0x0038 */
+	uint32_t	svcc;			/* 0x0040 */
+#define LCTL_CR0	0x8000
+#define LCTL_CR6	0x0200
+#define LCTL_CR9	0x0040
+#define LCTL_CR10	0x0020
+#define LCTL_CR11	0x0010
+#define LCTL_CR14	0x0002
+	uint16_t   	lctl;			/* 0x0044 */
+	int16_t		icpua;			/* 0x0046 */
+#define ICTL_OPEREXC	0x80000000
+#define ICTL_PINT	0x20000000
+#define ICTL_LPSW	0x00400000
+#define ICTL_STCTL	0x00040000
+#define ICTL_ISKE	0x00004000
+#define ICTL_SSKE	0x00002000
+#define ICTL_RRBE	0x00001000
+#define ICTL_TPROT	0x00000200
+	uint32_t	ictl;			/* 0x0048 */
+#define ECA_CEI		0x80000000
+#define ECA_IB		0x40000000
+#define ECA_SIGPI	0x10000000
+#define ECA_MVPGI	0x01000000
+#define ECA_AIV		0x00200000
+#define ECA_VX		0x00020000
+#define ECA_PROTEXCI	0x00002000
+#define ECA_APIE	0x00000008
+#define ECA_SII		0x00000001
+	uint32_t	eca;			/* 0x004c */
+#define ICPT_INST	0x04
+#define ICPT_PROGI	0x08
+#define ICPT_INSTPROGI	0x0C
+#define ICPT_EXTREQ	0x10
+#define ICPT_EXTINT	0x14
+#define ICPT_IOREQ	0x18
+#define ICPT_WAIT	0x1c
+#define ICPT_VALIDITY	0x20
+#define ICPT_STOP	0x28
+#define ICPT_OPEREXC	0x2C
+#define ICPT_PARTEXEC	0x38
+#define ICPT_IOINST	0x40
+#define ICPT_KSS	0x5c
+	uint8_t		icptcode;		/* 0x0050 */
+	uint8_t		icptstatus;		/* 0x0051 */
+	uint16_t	ihcpu;			/* 0x0052 */
+	uint8_t		reserved54[2];		/* 0x0054 */
+	uint16_t	ipa;			/* 0x0056 */
+	uint32_t	ipb;			/* 0x0058 */
+	uint32_t	scaoh;			/* 0x005c */
+#define FPF_BPBC 	0x20
+	uint8_t		fpf;			/* 0x0060 */
+#define ECB_GS		0x40
+#define ECB_TE		0x10
+#define ECB_SRSI	0x04
+#define ECB_HOSTPROTINT	0x02
+	uint8_t		ecb;			/* 0x0061 */
+#define ECB2_CMMA	0x80
+#define ECB2_IEP	0x20
+#define ECB2_PFMFI	0x08
+#define ECB2_ESCA	0x04
+	uint8_t    	ecb2;                   /* 0x0062 */
+#define ECB3_DEA 0x08
+#define ECB3_AES 0x04
+#define ECB3_RI  0x01
+	uint8_t    	ecb3;			/* 0x0063 */
+	uint32_t	scaol;			/* 0x0064 */
+	uint8_t		reserved68;		/* 0x0068 */
+	uint8_t    	epdx;			/* 0x0069 */
+	uint8_t    	reserved6a[2];		/* 0x006a */
+	uint32_t	todpr;			/* 0x006c */
+#define GISA_FORMAT1 0x00000001
+	uint32_t	gd;			/* 0x0070 */
+	uint8_t		reserved74[12];		/* 0x0074 */
+	uint64_t	mso;			/* 0x0080 */
+	uint64_t	msl;			/* 0x0088 */
+	struct psw	gpsw;			/* 0x0090 */
+	uint64_t	gg14;			/* 0x00a0 */
+	uint64_t	gg15;			/* 0x00a8 */
+	uint8_t		reservedb0[8];		/* 0x00b0 */
+#define HPID_KVM	0x4
+#define HPID_VSIE	0x5
+	uint8_t		hpid;			/* 0x00b8 */
+	uint8_t		reservedb9[11];		/* 0x00b9 */
+	uint16_t	extcpuaddr;		/* 0x00c4 */
+	uint16_t	eic;			/* 0x00c6 */
+	uint32_t	reservedc8;		/* 0x00c8 */
+	uint16_t	pgmilc;			/* 0x00cc */
+	uint16_t	iprcc;			/* 0x00ce */
+	uint32_t	dxc;			/* 0x00d0 */
+	uint16_t	mcn;			/* 0x00d4 */
+	uint8_t		perc;			/* 0x00d6 */
+	uint8_t		peratmid;		/* 0x00d7 */
+	uint64_t	peraddr;		/* 0x00d8 */
+	uint8_t		eai;			/* 0x00e0 */
+	uint8_t		peraid;			/* 0x00e1 */
+	uint8_t		oai;			/* 0x00e2 */
+	uint8_t		armid;			/* 0x00e3 */
+	uint8_t		reservede4[4];		/* 0x00e4 */
+	uint64_t	tecmc;			/* 0x00e8 */
+	uint8_t		reservedf0[12];		/* 0x00f0 */
+#define CRYCB_FORMAT_MASK 0x00000003
+#define CRYCB_FORMAT0 0x00000000
+#define CRYCB_FORMAT1 0x00000001
+#define CRYCB_FORMAT2 0x00000003
+	uint32_t	crycbd;			/* 0x00fc */
+	uint64_t	gcr[16];		/* 0x0100 */
+	uint64_t	gbea;			/* 0x0180 */
+	uint8_t		reserved188[8];		/* 0x0188 */
+	uint64_t   	sdnxo;			/* 0x0190 */
+	uint8_t    	reserved198[8];		/* 0x0198 */
+	uint32_t	fac;			/* 0x01a0 */
+	uint8_t		reserved1a4[20];	/* 0x01a4 */
+	uint64_t	cbrlo;			/* 0x01b8 */
+	uint8_t		reserved1c0[8];		/* 0x01c0 */
+#define ECD_HOSTREGMGMT	0x20000000
+#define ECD_MEF		0x08000000
+#define ECD_ETOKENF	0x02000000
+#define ECD_ECC		0x00200000
+	uint32_t	ecd;			/* 0x01c8 */
+	uint8_t		reserved1cc[18];	/* 0x01cc */
+	uint64_t	pp;			/* 0x01de */
+	uint8_t		reserved1e6[2];		/* 0x01e6 */
+	uint64_t	itdba;			/* 0x01e8 */
+	uint64_t   	riccbd;			/* 0x01f0 */
+	uint64_t	gvrd;			/* 0x01f8 */
+} __attribute__((packed));
+
+struct vm_save_regs {
+	u64 grs[16];
+	u64 fprs[16];
+	u32 fpc;
+};
+
+/* We might be able to nestle all of this into the stack frame. But
+ * having a dedicated save area that saves more than the s390 ELF ABI
+ * defines leaves us more freedom in the implementation.
+*/
+struct vm_save_area {
+	struct vm_save_regs guest;
+	struct vm_save_regs host;
+};
+
+struct vm {
+	struct kvm_s390_sie_block *sblk;
+	struct vm_save_area save_area;
+	/* Ptr to first guest page */
+	u8 *guest_mem;
+};
+
+extern u64 sie_entry;
+extern u64 sie_exit;
+extern void sie64a(struct kvm_s390_sie_block *sblk, struct vm_save_area *save_area);
+
+#endif /* SIE_H */
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 4e51150..2711018 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -203,6 +203,62 @@ smp_cpu_setup_state:
 	/* If the function returns, just loop here */
 0:	j	0
 
+/*
+ * sie64a calling convention:
+ * %r2 pointer to sie control block
+ * %r3 guest register save area
+ */
+.globl sie64a
+sie64a:
+	# Save host grs, fprs, fpc
+	stmg	%r0,%r14,SIE_SAVEAREA_HOST_GRS(%r3)	# save kernel registers
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8  + SIE_SAVEAREA_HOST_FPRS(%r3)
+	.endr
+	stfpc	SIE_SAVEAREA_HOST_FPC(%r3)
+
+	# Store scb and save_area pointer into stack frame
+	stg	%r2,__SF_SIE_CONTROL(%r15)	# save control block pointer
+	stg	%r3,__SF_SIE_SAVEAREA(%r15)	# save guest register save area
+
+	# Load guest's gprs, fprs and fpc
+	lmg	%r0,%r13,SIE_SAVEAREA_GUEST_GRS(%r3)
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8 + SIE_SAVEAREA_GUEST_FPRS(%r3)
+	.endr
+	lfpc	SIE_SAVEAREA_GUEST_FPC(%r3)
+
+	# Move scb ptr into r14 for the sie instruction
+	lg	%r14,__SF_SIE_CONTROL(%r15)
+
+.globl sie_entry
+sie_entry:
+	sie	0(%r14)
+	nopr	7
+	nopr	7
+	nopr	7
+
+.globl sie_exit
+sie_exit:
+	# Load guest register save area
+	lg	%r14,__SF_SIE_SAVEAREA(%r15)
+
+	# Store guest's gprs, fprs and fpc
+	stmg	%r0,%r13,SIE_SAVEAREA_GUEST_GRS(%r14)	# save guest gprs 0-13
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8  + SIE_SAVEAREA_GUEST_FPRS(%r14)
+	.endr
+	stfpc	SIE_SAVEAREA_GUEST_FPC(%r14)
+
+	# Restore host's gprs, fprs and fpc
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8 + SIE_SAVEAREA_HOST_FPRS(%r14)
+	.endr
+	lfpc	SIE_SAVEAREA_HOST_FPC(%r14)
+	lmg	%r0,%r14,SIE_SAVEAREA_HOST_GRS(%r14)	# restore kernel registers
+
+	br	%r14
+
 pgm_int:
 	SAVE_REGS
 	brasl	%r14, handle_pgm_int
-- 
2.25.1

[kvm-unit-tests PATCH 5/5] s390x: sie: Add first SIE test

[Janosch Frank]

Let's check if we get the correct interception data on a few
diags. This commit is more of an addition of boilerplate code than a
real test.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile |   1 +
 s390x/sie.c    | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)
 create mode 100644 s390x/sie.c

diff --git a/s390x/Makefile b/s390x/Makefile
index b079a26..7a95092 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -19,6 +19,7 @@ tests += $(TEST_DIR)/smp.elf
 tests += $(TEST_DIR)/sclp.elf
 tests += $(TEST_DIR)/css.elf
 tests += $(TEST_DIR)/uv-guest.elf
+tests += $(TEST_DIR)/sie.elf
 
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 ifneq ($(HOST_KEY_DOCUMENT),)
diff --git a/s390x/sie.c b/s390x/sie.c
new file mode 100644
index 0000000..41b429a
--- /dev/null
+++ b/s390x/sie.c
@@ -0,0 +1,125 @@
+#include <libcflat.h>
+#include <asm/asm-offsets.h>
+#include <asm/arch_def.h>
+#include <asm/interrupt.h>
+#include <asm/page.h>
+#include <alloc_page.h>
+#include <vmalloc.h>
+#include <asm/facility.h>
+#include <mmu.h>
+#include <sclp.h>
+#include <sie.h>
+
+static u8 *guest;
+static u8 *guest_instr;
+static struct vm vm;
+
+static void handle_validity(struct vm *vm)
+{
+	report(0, "VALIDITY: %x", vm->sblk->ipb >> 16);
+}
+
+static void sie(struct vm *vm)
+{
+	while (vm->sblk->icptcode == 0) {
+		sie64a(vm->sblk, &vm->save_area);
+		if (vm->sblk->icptcode == 32)
+		    handle_validity(vm);
+	}
+	vm->save_area.guest.grs[14] = vm->sblk->gg14;
+	vm->save_area.guest.grs[15] = vm->sblk->gg15;
+}
+
+static void sblk_cleanup(struct vm *vm)
+{
+	vm->sblk->icptcode = 0;
+}
+
+static void intercept_diag_10(void)
+{
+	u32 instr = 0x83020010;
+
+	vm.sblk->gpsw.addr = PAGE_SIZE * 2;
+	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
+
+	memset(guest_instr, 0, PAGE_SIZE);
+	memcpy(guest_instr, &instr, 4);
+	sie(&vm);
+	report(vm.sblk->icptcode == 4 && vm.sblk->ipa == 0x8302 && vm.sblk->ipb == 0x100000,
+	       "Diag 10 intercept");
+	sblk_cleanup(&vm);
+}
+
+static void intercept_diag_44(void)
+{
+	u32 instr = 0x83020044;
+
+	vm.sblk->gpsw.addr = PAGE_SIZE * 2;
+	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
+
+	memset(guest_instr, 0, PAGE_SIZE);
+	memcpy(guest_instr, &instr, 4);
+	sie(&vm);
+	report(vm.sblk->icptcode == 4 && vm.sblk->ipa == 0x8302 && vm.sblk->ipb == 0x440000,
+	       "Diag 44 intercept");
+	sblk_cleanup(&vm);
+}
+
+static void intercept_diag_9c(void)
+{
+	u32 instr = 0x8302009c;
+
+	vm.sblk->gpsw.addr = PAGE_SIZE * 2;
+	vm.sblk->gpsw.mask = 0x0000000180000000ULL;
+
+	memset(guest_instr, 0, PAGE_SIZE);
+	memcpy(guest_instr, &instr, 4);
+	sie(&vm);
+	report(vm.sblk->icptcode == 4 && vm.sblk->ipa == 0x8302 && vm.sblk->ipb == 0x9c0000,
+	       "Diag 9c intercept");
+	sblk_cleanup(&vm);
+}
+
+static void setup_guest(void)
+{
+	setup_vm();
+
+	/* Allocate 1MB as guest memory */
+	guest = alloc_pages(8);
+	/* The first two pages are the lowcore */
+	guest_instr = guest + PAGE_SIZE * 2;
+
+	vm.sblk = alloc_page();
+
+	vm.sblk->cpuflags = CPUSTAT_ZARCH | CPUSTAT_RUNNING;
+	vm.sblk->prefix = 0;
+	/*
+	 * Pageable guest with the same ASCE as the test programm, but
+	 * the guest memory 0x0 is offset to start at the allocated
+	 * guest pages and end after 1MB.
+	 *
+	 * It's not pretty but faster and easier than managing guest ASCEs.
+	 */
+	vm.sblk->mso = (u64)guest;
+	vm.sblk->msl = (u64)guest;
+	vm.sblk->ihcpu = 0xffff;
+
+	vm.sblk->crycbd = (uint64_t)alloc_page();
+}
+
+int main(void)
+{
+	report_prefix_push("sie");
+	if (!sclp_facilities.has_sief2) {
+		report_skip("SIEF2 facility unavailable");
+		goto done;
+	}
+
+	setup_guest();
+	intercept_diag_10();
+	intercept_diag_44();
+	intercept_diag_9c();
+done:
+	report_prefix_pop();
+	return report_summary();
+}
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/5] s390x: Add test_bit to library

[Cornelia Huck]

On Tue, 17 Nov 2020 10:42:11 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Query/feature bits are commonly tested via MSB bit numbers on
> s390. Let's add test bit functions, so we don't need to copy code to
> test query bits.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/bitops.h   | 16 ++++++++++++++++
>  lib/s390x/asm/facility.h |  3 ++-
>  2 files changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 2/5] s390x: Consolidate sclp read info

[Cornelia Huck]

On Tue, 17 Nov 2020 10:42:12 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's only read the information once and pass a pointer to it instead
> of calling sclp multiple times.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 29 +++++++++++++++++++++++------
>  lib/s390x/sclp.h |  3 +++
>  lib/s390x/smp.c  | 23 +++++++++--------------
>  4 files changed, 36 insertions(+), 20 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [kvm-unit-tests PATCH 3/5] s390x: SCLP feature checking

[Cornelia Huck]

On Tue, 17 Nov 2020 10:42:13 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Availability of SIE is announced via a feature bit in a SCLP info CPU
> entry. Let's add a framework that allows us to easily check for such
> facilities.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>  lib/s390x/sclp.h | 15 +++++++++++++++
>  3 files changed, 35 insertions(+)

(...)

> +void sclp_facilities_setup(void)
> +{
> +	unsigned short cpu0_addr = stap();
> +	CPUEntry *cpu;
> +	int i;
> +
> +	assert(read_info);
> +
> +	cpu = (void *)read_info + read_info->offset_cpu;
> +	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
> +		if (cpu->address == cpu0_addr) {
> +			sclp_facilities.has_sief2 = test_bit_inv(SCLP_CPU_FEATURE_SIEF2_BIT, (void *)&cpu->address);

Can you wrap this? This line is really overlong.

(Also, just to understand: Is sief2 only indicated for cpu0, and not
for the other cpus?)

> +			break;
> +		}
> +	}
> +}
> +
>  /* Perform service call. Return 0 on success, non-zero otherwise. */
>  int sclp_service_call(unsigned int command, void *sccb)
>  {

Re: [kvm-unit-tests PATCH 3/5] s390x: SCLP feature checking

[Janosch Frank]

[-- Attachment #1.1.1: Type: text/plain, Size: 1695 bytes --]

On 11/18/20 6:50 PM, Cornelia Huck wrote:
> On Tue, 17 Nov 2020 10:42:13 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Availability of SIE is announced via a feature bit in a SCLP info CPU
>> entry. Let's add a framework that allows us to easily check for such
>> facilities.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/io.c   |  1 +
>>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>>  lib/s390x/sclp.h | 15 +++++++++++++++
>>  3 files changed, 35 insertions(+)
> 
> (...)

Btw. I also wanted to add this to the struct sclp_facilities:
u64 reserved : 63:

> 
>> +void sclp_facilities_setup(void)
>> +{
>> +	unsigned short cpu0_addr = stap();
>> +	CPUEntry *cpu;
>> +	int i;
>> +
>> +	assert(read_info);
>> +
>> +	cpu = (void *)read_info + read_info->offset_cpu;
>> +	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>> +		if (cpu->address == cpu0_addr) {
>> +			sclp_facilities.has_sief2 = test_bit_inv(SCLP_CPU_FEATURE_SIEF2_BIT, (void *)&cpu->address);
> 
> Can you wrap this? This line is really overlong.

Sure

> 
> (Also, just to understand: Is sief2 only indicated for cpu0, and not
> for the other cpus?)

That's an excellent question I don't know the answer to.
I had a look at the kernel's implementation of this and that's what
they're doing so I figured they had a reason for it.

The documentation doesn't seem to say anything about that, but there's a
lot of it I haven't yet read.

> 
>> +			break;
>> +		}
>> +	}
>> +}
>> +
>>  /* Perform service call. Return 0 on success, non-zero otherwise. */
>>  int sclp_service_call(unsigned int command, void *sccb)
>>  {
> 


[-- Attachment #1.1.2: OpenPGP_0xE354E6B8E238B9F8.asc --]
[-- Type: application/pgp-keys, Size: 7995 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [kvm-unit-tests PATCH 1/5] s390x: Add test_bit to library

[Thomas Huth]

On 17/11/2020 16.42, Janosch Frank wrote:
> Query/feature bits are commonly tested via MSB bit numbers on
> s390. Let's add test bit functions, so we don't need to copy code to
> test query bits.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/bitops.h   | 16 ++++++++++++++++
>  lib/s390x/asm/facility.h |  3 ++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
> index e7cdda9..a272dd7 100644
> --- a/lib/s390x/asm/bitops.h
> +++ b/lib/s390x/asm/bitops.h
> @@ -7,4 +7,20 @@
>  
>  #define BITS_PER_LONG	64
>  
> +static inline bool test_bit(unsigned long nr,
> +			    const volatile unsigned long *ptr)
> +{
> +	const volatile unsigned char *addr;
> +
> +	addr = ((const volatile unsigned char *)ptr);
> +	addr += (nr ^ (BITS_PER_LONG - 8)) >> 3;
> +	return (*addr >> (nr & 7)) & 1;
> +}
> +
> +static inline bool test_bit_inv(unsigned long nr,
> +				const volatile unsigned long *ptr)
> +{
> +	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
> +}

I think you should mention in the patch description that these functions
match the implementations in the kernel (and thus are good for kernel
developers who are used to these).

Thus I think you should also now add a license statement to this file
("SPDX-License-Identifier: GPL-2.0" or so).

With these modifications:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH 1/5] s390x: Add test_bit to library

[Janosch Frank]

On 11/19/20 9:26 AM, Thomas Huth wrote:
> On 17/11/2020 16.42, Janosch Frank wrote:
>> Query/feature bits are commonly tested via MSB bit numbers on
>> s390. Let's add test bit functions, so we don't need to copy code to
>> test query bits.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/bitops.h   | 16 ++++++++++++++++
>>  lib/s390x/asm/facility.h |  3 ++-
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
>> index e7cdda9..a272dd7 100644
>> --- a/lib/s390x/asm/bitops.h
>> +++ b/lib/s390x/asm/bitops.h
>> @@ -7,4 +7,20 @@
>>  
>>  #define BITS_PER_LONG	64
>>  
>> +static inline bool test_bit(unsigned long nr,
>> +			    const volatile unsigned long *ptr)
>> +{
>> +	const volatile unsigned char *addr;
>> +
>> +	addr = ((const volatile unsigned char *)ptr);
>> +	addr += (nr ^ (BITS_PER_LONG - 8)) >> 3;
>> +	return (*addr >> (nr & 7)) & 1;
>> +}
>> +
>> +static inline bool test_bit_inv(unsigned long nr,
>> +				const volatile unsigned long *ptr)
>> +{
>> +	return test_bit(nr ^ (BITS_PER_LONG - 1), ptr);
>> +}
> 
> I think you should mention in the patch description that these functions
> match the implementations in the kernel (and thus are good for kernel
> developers who are used to these).

There are only so many ways one can write 4 lines of code :-)

> 
> Thus I think you should also now add a license statement to this file
> ("SPDX-License-Identifier: GPL-2.0" or so).

Definitely, thanks for reminding me

> 
> With these modifications:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks

Re: [kvm-unit-tests PATCH 2/5] s390x: Consolidate sclp read info

[Thomas Huth]

On 17/11/2020 16.42, Janosch Frank wrote:
> Let's only read the information once and pass a pointer to it instead
> of calling sclp multiple times.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 29 +++++++++++++++++++++++------
>  lib/s390x/sclp.h |  3 +++
>  lib/s390x/smp.c  | 23 +++++++++--------------
>  4 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> index c0f0bf7..e19a1f3 100644
> --- a/lib/s390x/io.c
> +++ b/lib/s390x/io.c
> @@ -36,6 +36,7 @@ void setup(void)
>  {
>  	setup_args_progname(ipl_args);
>  	setup_facilities();
> +	sclp_read_info();
>  	sclp_console_setup();
>  	sclp_memory_setup();
>  	smp_setup();
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 4054d0e..ea6324e 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -25,6 +25,8 @@ extern unsigned long stacktop;
>  static uint64_t storage_increment_size;
>  static uint64_t max_ram_size;
>  static uint64_t ram_size;
> +char _read_info[PAGE_SIZE] __attribute__((__aligned__(4096)));
> +static ReadInfo *read_info;
>  
>  char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>  static volatile bool sclp_busy;
> @@ -110,6 +112,22 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
>  	report_abort("READ_SCP_INFO failed");
>  }
>  
> +void sclp_read_info(void)
> +{
> +	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
> +	read_info = (ReadInfo *)_read_info;
> +}
> +
> +int sclp_get_cpu_num(void)
> +{
> +	return read_info->entries_cpu;
> +}
[...]
>  int smp_query_num_cpus(void)
>  {
> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
> -	return info->nr_configured;
> +	return sclp_get_cpu_num();
>  }

You've changed from ->nr_configured to ->entries_cpu ... I assume that's ok?
Worth to mention the change and rationale in the patch description?

>  struct cpu *smp_cpu_from_addr(uint16_t addr)
> @@ -245,22 +244,18 @@ extern uint64_t *stackptr;
>  void smp_setup(void)
>  {
>  	int i = 0;
> +	int num = smp_query_num_cpus();
>  	unsigned short cpu0_addr = stap();
> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
> +	struct CPUEntry *entry = sclp_get_cpu_entries();
>  
> -	spin_lock(&lock);
> -	sclp_mark_busy();
> -	info->h.length = PAGE_SIZE;
> -	sclp_service_call(SCLP_READ_CPU_INFO, cpu_info_buffer);
> +	if (num > 1)
> +		printf("SMP: Initializing, found %d cpus\n", num);
>  
> -	if (smp_query_num_cpus() > 1)
> -		printf("SMP: Initializing, found %d cpus\n", info->nr_configured);
> -
> -	cpus = calloc(info->nr_configured, sizeof(cpus));
> -	for (i = 0; i < info->nr_configured; i++) {
> -		cpus[i].addr = info->entries[i].address;
> +	cpus = calloc(num, sizeof(cpus));
> +	for (i = 0; i < num; i++) {
> +		cpus[i].addr = entry[i].address;
>  		cpus[i].active = false;
> -		if (info->entries[i].address == cpu0_addr) {
> +		if (entry[i].address == cpu0_addr) {
>  			cpu0 = &cpus[i];
>  			cpu0->stack = stackptr;
>  			cpu0->lowcore = (void *)0;
> 

What about smp_teardown()? It seems to use cpu_info_buffer->nr_configured,
too, which is now likely not valid anymore?

 Thomas

Re: [kvm-unit-tests PATCH 2/5] s390x: Consolidate sclp read info

[Janosch Frank]

On 11/19/20 9:41 AM, Thomas Huth wrote:
> On 17/11/2020 16.42, Janosch Frank wrote:
>> Let's only read the information once and pass a pointer to it instead
>> of calling sclp multiple times.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/io.c   |  1 +
>>  lib/s390x/sclp.c | 29 +++++++++++++++++++++++------
>>  lib/s390x/sclp.h |  3 +++
>>  lib/s390x/smp.c  | 23 +++++++++--------------
>>  4 files changed, 36 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
>> index c0f0bf7..e19a1f3 100644
>> --- a/lib/s390x/io.c
>> +++ b/lib/s390x/io.c
>> @@ -36,6 +36,7 @@ void setup(void)
>>  {
>>  	setup_args_progname(ipl_args);
>>  	setup_facilities();
>> +	sclp_read_info();
>>  	sclp_console_setup();
>>  	sclp_memory_setup();
>>  	smp_setup();
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index 4054d0e..ea6324e 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -25,6 +25,8 @@ extern unsigned long stacktop;
>>  static uint64_t storage_increment_size;
>>  static uint64_t max_ram_size;
>>  static uint64_t ram_size;
>> +char _read_info[PAGE_SIZE] __attribute__((__aligned__(4096)));
>> +static ReadInfo *read_info;
>>  
>>  char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>>  static volatile bool sclp_busy;
>> @@ -110,6 +112,22 @@ static void sclp_read_scp_info(ReadInfo *ri, int length)
>>  	report_abort("READ_SCP_INFO failed");
>>  }
>>  
>> +void sclp_read_info(void)
>> +{
>> +	sclp_read_scp_info((void *)_read_info, SCCB_SIZE);
>> +	read_info = (ReadInfo *)_read_info;
>> +}
>> +
>> +int sclp_get_cpu_num(void)
>> +{
>> +	return read_info->entries_cpu;
>> +}
> [...]
>>  int smp_query_num_cpus(void)
>>  {
>> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
>> -	return info->nr_configured;
>> +	return sclp_get_cpu_num();
>>  }
> 
> You've changed from ->nr_configured to ->entries_cpu ... I assume that's ok?
> Worth to mention the change and rationale in the patch description?

Well, it's not so much a change of struct members than a change of
structs themselves. Looking at our QEMU implementation, both struct
members transport the same data so I don't see a problem there.

> 
>>  struct cpu *smp_cpu_from_addr(uint16_t addr)
>> @@ -245,22 +244,18 @@ extern uint64_t *stackptr;
>>  void smp_setup(void)
>>  {
>>  	int i = 0;
>> +	int num = smp_query_num_cpus();
>>  	unsigned short cpu0_addr = stap();
>> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
>> +	struct CPUEntry *entry = sclp_get_cpu_entries();
>>  
>> -	spin_lock(&lock);
>> -	sclp_mark_busy();
>> -	info->h.length = PAGE_SIZE;
>> -	sclp_service_call(SCLP_READ_CPU_INFO, cpu_info_buffer);
>> +	if (num > 1)
>> +		printf("SMP: Initializing, found %d cpus\n", num);
>>  
>> -	if (smp_query_num_cpus() > 1)
>> -		printf("SMP: Initializing, found %d cpus\n", info->nr_configured);
>> -
>> -	cpus = calloc(info->nr_configured, sizeof(cpus));
>> -	for (i = 0; i < info->nr_configured; i++) {
>> -		cpus[i].addr = info->entries[i].address;
>> +	cpus = calloc(num, sizeof(cpus));
>> +	for (i = 0; i < num; i++) {
>> +		cpus[i].addr = entry[i].address;
>>  		cpus[i].active = false;
>> -		if (info->entries[i].address == cpu0_addr) {
>> +		if (entry[i].address == cpu0_addr) {
>>  			cpu0 = &cpus[i];
>>  			cpu0->stack = stackptr;
>>  			cpu0->lowcore = (void *)0;
>>
> 
> What about smp_teardown()? It seems to use cpu_info_buffer->nr_configured,
> too, which is now likely not valid anymore?

Good catch, I forgot to remove the whole cpu info buffer.

> 
>  Thomas
> 

Re: [kvm-unit-tests PATCH 3/5] s390x: SCLP feature checking

[Thomas Huth]

On 17/11/2020 16.42, Janosch Frank wrote:
> Availability of SIE is announced via a feature bit in a SCLP info CPU
> entry. Let's add a framework that allows us to easily check for such
> facilities.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>  lib/s390x/sclp.h | 15 +++++++++++++++
>  3 files changed, 35 insertions(+)
[...]
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 6620531..bcc9f4b 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -101,6 +101,20 @@ typedef struct CPUEntry {
>      uint8_t reserved1;
>  } __attribute__((packed)) CPUEntry;
>  
> +extern struct sclp_facilities sclp_facilities;
> +
> +struct sclp_facilities {
> +	u64 has_sief2 : 1;
> +};
> +
> +/*
> + * test_bit() uses unsigned long ptrs so we give it the ptr to the
> + * address member and offset bits by 1> + */
> +enum sclp_cpu_feature_bit {
> +	SCLP_CPU_FEATURE_SIEF2_BIT = 16 + 4,
> +};

That's kind of ugly ... why don't you simply replace the CPUEntry.features[]
array with a bitfield, similar to what the kernel does with "struct
sclp_core_entry" ?

 Thomas

Re: [kvm-unit-tests PATCH 3/5] s390x: SCLP feature checking

[Janosch Frank]

On 11/19/20 10:15 AM, Thomas Huth wrote:
> On 17/11/2020 16.42, Janosch Frank wrote:
>> Availability of SIE is announced via a feature bit in a SCLP info CPU
>> entry. Let's add a framework that allows us to easily check for such
>> facilities.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/io.c   |  1 +
>>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>>  lib/s390x/sclp.h | 15 +++++++++++++++
>>  3 files changed, 35 insertions(+)
> [...]
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index 6620531..bcc9f4b 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -101,6 +101,20 @@ typedef struct CPUEntry {
>>      uint8_t reserved1;
>>  } __attribute__((packed)) CPUEntry;
>>  
>> +extern struct sclp_facilities sclp_facilities;
>> +
>> +struct sclp_facilities {
>> +	u64 has_sief2 : 1;
>> +};
>> +
>> +/*
>> + * test_bit() uses unsigned long ptrs so we give it the ptr to the
>> + * address member and offset bits by 1> + */
>> +enum sclp_cpu_feature_bit {
>> +	SCLP_CPU_FEATURE_SIEF2_BIT = 16 + 4,
>> +};
> 
> That's kind of ugly ... why don't you simply replace the CPUEntry.features[]
> array with a bitfield, similar to what the kernel does with "struct
> sclp_core_entry" ?

That's an excellent idea, will do!

> 
>  Thomas
> 
>