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

[kvm-unit-tests PATCH v3 0/8] 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.

v3:
	* Rebased on re-license patches
	* Split assembly
	* Now using ICPT_* constants
	* Added read_info asserts
	* Fixed missing spin_lock() in smp.c lib
	* Replaced duplicated code in sie test with generic intercept test
	* Replaced uv-guest.x bit testing with test_bit_inv()
	* Some other minor cleanups

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

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

Janosch Frank (8):
  s390x: Add test_bit to library
  s390x: Consolidate sclp read info
  s390x: SCLP feature checking
  s390x: Split assembly and move to s390x/asm/
  s390x: sie: Add SIE to lib
  s390x: sie: Add first SIE test
  s390x: Add diag318 intercept test
  s390x: Fix sclp.h style issues

 lib/s390x/asm-offsets.c    |  13 +++
 lib/s390x/asm/arch_def.h   |   7 ++
 lib/s390x/asm/bitops.h     |  26 +++++
 lib/s390x/asm/facility.h   |   3 +-
 lib/s390x/interrupt.c      |   7 ++
 lib/s390x/io.c             |   2 +
 lib/s390x/sclp.c           |  52 ++++++++--
 lib/s390x/sclp.h           | 178 ++++++++++++++++++---------------
 lib/s390x/sie.h            | 197 +++++++++++++++++++++++++++++++++++++
 lib/s390x/smp.c            |  27 +++--
 s390x/Makefile             |   9 +-
 s390x/{ => asm}/cstart64.S | 119 +---------------------
 s390x/asm/lib.S            | 121 +++++++++++++++++++++++
 s390x/asm/macros.S         |  77 +++++++++++++++
 s390x/intercept.c          |  19 ++++
 s390x/sie.c                | 113 +++++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 s390x/uv-guest.c           |   6 +-
 18 files changed, 754 insertions(+), 225 deletions(-)
 create mode 100644 lib/s390x/sie.h
 rename s390x/{ => asm}/cstart64.S (50%)
 create mode 100644 s390x/asm/lib.S
 create mode 100644 s390x/asm/macros.S
 create mode 100644 s390x/sie.c

-- 
2.25.1

[kvm-unit-tests PATCH v3 1/8] 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.

The test_bit code has been taken from the kernel since most s390x KVM unit
test developers are used to them.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/asm/bitops.h   | 26 ++++++++++++++++++++++++++
 lib/s390x/asm/facility.h |  3 ++-
 s390x/uv-guest.c         |  6 +++---
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/lib/s390x/asm/bitops.h b/lib/s390x/asm/bitops.h
index e7cdda9..792881e 100644
--- a/lib/s390x/asm/bitops.h
+++ b/lib/s390x/asm/bitops.h
@@ -1,3 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *    Bitops taken from the kernel as most developers are already used
+ *    to them.
+ *
+ *    Copyright IBM Corp. 1999,2013
+ *
+ *    Author(s): Martin Schwidefsky <schwidefsky@de.ibm.com>,
+ *
+ */
 #ifndef _ASMS390X_BITOPS_H_
 #define _ASMS390X_BITOPS_H_
 
@@ -7,4 +17,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 7828cf8..95d4a15 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -11,13 +11,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)
diff --git a/s390x/uv-guest.c b/s390x/uv-guest.c
index bc947ab..e51b85e 100644
--- a/s390x/uv-guest.c
+++ b/s390x/uv-guest.c
@@ -75,11 +75,11 @@ static void test_query(void)
 	 * Ultravisor version and are expected to always be available
 	 * because they are basic building blocks.
 	 */
-	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_QUI)),
+	report(test_bit_inv(BIT_UVC_CMD_QUI, &uvcb.inst_calls_list[0]),
 	       "query indicated");
-	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_SET_SHARED_ACCESS)),
+	report(test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
 	       "share indicated");
-	report(uvcb.inst_calls_list[0] & (1UL << (63 - BIT_UVC_CMD_REMOVE_SHARED_ACCESS)),
+	report(test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, &uvcb.inst_calls_list[0]),
 	       "unshare indicated");
 	report_prefix_pop();
 }
-- 
2.25.1

[kvm-unit-tests PATCH v3 2/8] 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>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 lib/s390x/io.c   |  1 +
 lib/s390x/sclp.c | 31 +++++++++++++++++++++++++------
 lib/s390x/sclp.h |  3 +++
 lib/s390x/smp.c  | 27 +++++++++++----------------
 4 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index 1ff0589..6a1da63 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -34,6 +34,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 08a4813..bf1d9c0 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -23,6 +23,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;
@@ -108,6 +110,24 @@ 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)
+{
+	assert(read_info);
+	return read_info->entries_cpu;
+}
+
+CPUEntry *sclp_get_cpu_entries(void)
+{
+	assert(read_info);
+	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)
 {
@@ -125,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 9a6aad0..acd86d5 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -268,6 +268,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 c4f02dc..dfcfd28 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -23,7 +23,6 @@
 #include "smp.h"
 #include "sclp.h"
 
-static char cpu_info_buffer[PAGE_SIZE] __attribute__((__aligned__(4096)));
 static struct cpu *cpus;
 static struct cpu *cpu0;
 static struct spinlock lock;
@@ -32,8 +31,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)
@@ -226,10 +224,10 @@ void smp_teardown(void)
 {
 	int i = 0;
 	uint16_t this_cpu = stap();
-	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
+	int num = smp_query_num_cpus();
 
 	spin_lock(&lock);
-	for (; i < info->nr_configured; i++) {
+	for (; i < num; i++) {
 		if (cpus[i].active &&
 		    cpus[i].addr != this_cpu) {
 			sigp_retry(cpus[i].addr, SIGP_STOP, 0, NULL);
@@ -243,22 +241,19 @@ 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

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

[Thomas Huth]

On 11/12/2020 11.00, 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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 31 +++++++++++++++++++++++++------
>  lib/s390x/sclp.h |  3 +++
>  lib/s390x/smp.c  | 27 +++++++++++----------------
>  4 files changed, 40 insertions(+), 22 deletions(-)

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:33 -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>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 31 +++++++++++++++++++++++++------
>  lib/s390x/sclp.h |  3 +++
>  lib/s390x/smp.c  | 27 +++++++++++----------------
>  4 files changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> index 1ff0589..6a1da63 100644
> --- a/lib/s390x/io.c
> +++ b/lib/s390x/io.c
> @@ -34,6 +34,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 08a4813..bf1d9c0 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -23,6 +23,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)));

why not __aligned__((PAGE_SIZE)) ?

> +static ReadInfo *read_info;

I wonder if a union would be cleaner? although later on you check if
the pointer is NULL to see if the information is there, so I guess it
can stay

>  
>  char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>  static volatile bool sclp_busy;
> @@ -108,6 +110,24 @@ 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)
> +{
> +	assert(read_info);
> +	return read_info->entries_cpu;
> +}
> +
> +CPUEntry *sclp_get_cpu_entries(void)
> +{
> +	assert(read_info);
> +	return (void *)read_info + read_info->offset_cpu;

are you doing arithmetic on a void pointer? please don't, it's ugly and
against the specs. moreover you do have a char pointer...

why not:
return (CPUEntry *)(_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)
>  {
> @@ -125,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 9a6aad0..acd86d5 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -268,6 +268,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 c4f02dc..dfcfd28 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -23,7 +23,6 @@
>  #include "smp.h"
>  #include "sclp.h"
>  
> -static char cpu_info_buffer[PAGE_SIZE]
> __attribute__((__aligned__(4096))); static struct cpu *cpus;
>  static struct cpu *cpu0;
>  static struct spinlock lock;
> @@ -32,8 +31,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)
> @@ -226,10 +224,10 @@ void smp_teardown(void)
>  {
>  	int i = 0;
>  	uint16_t this_cpu = stap();
> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
> +	int num = smp_query_num_cpus();
>  
>  	spin_lock(&lock);
> -	for (; i < info->nr_configured; i++) {
> +	for (; i < num; i++) {
>  		if (cpus[i].active &&
>  		    cpus[i].addr != this_cpu) {
>  			sigp_retry(cpus[i].addr, SIGP_STOP, 0, NULL);
> @@ -243,22 +241,19 @@ 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;

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

[Janosch Frank]

On 12/17/20 12:47 PM, Claudio Imbrenda wrote:
> On Fri, 11 Dec 2020 05:00:33 -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>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>  lib/s390x/io.c   |  1 +
>>  lib/s390x/sclp.c | 31 +++++++++++++++++++++++++------
>>  lib/s390x/sclp.h |  3 +++
>>  lib/s390x/smp.c  | 27 +++++++++++----------------
>>  4 files changed, 40 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
>> index 1ff0589..6a1da63 100644
>> --- a/lib/s390x/io.c
>> +++ b/lib/s390x/io.c
>> @@ -34,6 +34,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 08a4813..bf1d9c0 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -23,6 +23,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)));
> 
> why not __aligned__((PAGE_SIZE)) ?

Because aligned is not defined as a compiler attribute in the lib AFAIK.
I can of course use PAGE_SIZE though.

> 
>> +static ReadInfo *read_info;
> 
> I wonder if a union would be cleaner? although later on you check if
> the pointer is NULL to see if the information is there, so I guess it
> can stay

I'm rather wondering if we want to replace that with an allocation,
these PAGE_SIZE arrays are just looking strange.

Let me put that on my TODO list for next year...

> 
>>  
>>  char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>>  static volatile bool sclp_busy;
>> @@ -108,6 +110,24 @@ 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)
>> +{
>> +	assert(read_info);
>> +	return read_info->entries_cpu;
>> +}
>> +
>> +CPUEntry *sclp_get_cpu_entries(void)
>> +{
>> +	assert(read_info);
>> +	return (void *)read_info + read_info->offset_cpu;
> 
> are you doing arithmetic on a void pointer? please don't, it's ugly and
> against the specs. moreover you do have a char pointer...
> 
> why not:
> return (CPUEntry *)(_read_info + read_info->offset_cpu);

I seem to be one of those crazy persons who actually like void pointers.
Your suggestion looks good too, I'll replace my code with it.

> 
>> +}
>> +
>>  /* Perform service call. Return 0 on success, non-zero otherwise. */
>>  int sclp_service_call(unsigned int command, void *sccb)
>>  {
>> @@ -125,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 9a6aad0..acd86d5 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -268,6 +268,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 c4f02dc..dfcfd28 100644
>> --- a/lib/s390x/smp.c
>> +++ b/lib/s390x/smp.c
>> @@ -23,7 +23,6 @@
>>  #include "smp.h"
>>  #include "sclp.h"
>>  
>> -static char cpu_info_buffer[PAGE_SIZE]
>> __attribute__((__aligned__(4096))); static struct cpu *cpus;
>>  static struct cpu *cpu0;
>>  static struct spinlock lock;
>> @@ -32,8 +31,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)
>> @@ -226,10 +224,10 @@ void smp_teardown(void)
>>  {
>>  	int i = 0;
>>  	uint16_t this_cpu = stap();
>> -	struct ReadCpuInfo *info = (void *)cpu_info_buffer;
>> +	int num = smp_query_num_cpus();
>>  
>>  	spin_lock(&lock);
>> -	for (; i < info->nr_configured; i++) {
>> +	for (; i < num; i++) {
>>  		if (cpus[i].active &&
>>  		    cpus[i].addr != this_cpu) {
>>  			sigp_retry(cpus[i].addr, SIGP_STOP, 0, NULL);
>> @@ -243,22 +241,19 @@ 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;
> 

[kvm-unit-tests PATCH v3 3/8] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/io.c   |  1 +
 lib/s390x/sclp.c | 19 +++++++++++++++++++
 lib/s390x/sclp.h | 13 ++++++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/io.c b/lib/s390x/io.c
index 6a1da63..ef9f59e 100644
--- a/lib/s390x/io.c
+++ b/lib/s390x/io.c
@@ -35,6 +35,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 bf1d9c0..cf6ea7c 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -9,6 +9,7 @@
  */
 
 #include <libcflat.h>
+#include <bitops.h>
 #include <asm/page.h>
 #include <asm/arch_def.h>
 #include <asm/interrupt.h>
@@ -25,6 +26,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 = cpu->feat_sief2;
+			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 acd86d5..6c86037 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -92,12 +92,22 @@ typedef struct SCCBHeader {
 typedef struct CPUEntry {
     uint8_t address;
     uint8_t reserved0;
-    uint8_t features[SCCB_CPU_FEATURE_LEN];
+    uint8_t : 4;
+    uint8_t feat_sief2 : 1;
+    uint8_t : 3;
+    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
     uint8_t reserved2[6];
     uint8_t type;
     uint8_t reserved1;
 } __attribute__((packed)) CPUEntry;
 
+extern struct sclp_facilities sclp_facilities;
+
+struct sclp_facilities {
+	uint64_t has_sief2 : 1;
+	uint64_t : 63;
+};
+
 typedef struct ReadInfo {
     SCCBHeader h;
     uint16_t rnmax;
@@ -271,6 +281,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

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

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:34 -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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>  lib/s390x/sclp.h | 13 ++++++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> index 6a1da63..ef9f59e 100644
> --- a/lib/s390x/io.c
> +++ b/lib/s390x/io.c
> @@ -35,6 +35,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 bf1d9c0..cf6ea7c 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <libcflat.h>
> +#include <bitops.h>

you add this include, but it seems you are not actually using it?

>  #include <asm/page.h>
>  #include <asm/arch_def.h>
>  #include <asm/interrupt.h>
> @@ -25,6 +26,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;

another void* arithmetic. consider using well-defined constructs, like

cpu = (CPUEntry *)(_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 = cpu->feat_sief2;
> +			break;

this only checks CPU 0. I wonder if you shouldn't check all CPUs? Or if
we assume that all CPUs have the same facilities, isn't it enough to
check the first CPU in the list? (i.e. avoid the loop)

> +		}
> +	}
> +}
> +
>  /* 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 acd86d5..6c86037 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -92,12 +92,22 @@ typedef struct SCCBHeader {
>  typedef struct CPUEntry {
>      uint8_t address;
>      uint8_t reserved0;
> -    uint8_t features[SCCB_CPU_FEATURE_LEN];
> +    uint8_t : 4;
> +    uint8_t feat_sief2 : 1;
> +    uint8_t : 3;
> +    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
>      uint8_t reserved2[6];
>      uint8_t type;
>      uint8_t reserved1;
>  } __attribute__((packed)) CPUEntry;
>  
> +extern struct sclp_facilities sclp_facilities;
> +
> +struct sclp_facilities {
> +	uint64_t has_sief2 : 1;
> +	uint64_t : 63;
> +};
> +
>  typedef struct ReadInfo {
>      SCCBHeader h;
>      uint16_t rnmax;
> @@ -271,6 +281,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);

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

[Janosch Frank]

On 12/17/20 1:18 PM, Claudio Imbrenda wrote:
> On Fri, 11 Dec 2020 05:00:34 -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>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/s390x/io.c   |  1 +
>>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>>  lib/s390x/sclp.h | 13 ++++++++++++-
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
>> index 6a1da63..ef9f59e 100644
>> --- a/lib/s390x/io.c
>> +++ b/lib/s390x/io.c
>> @@ -35,6 +35,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 bf1d9c0..cf6ea7c 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include <libcflat.h>
>> +#include <bitops.h>
> 
> you add this include, but it seems you are not actually using it?

Leftover from last version

> 
>>  #include <asm/page.h>
>>  #include <asm/arch_def.h>
>>  #include <asm/interrupt.h>
>> @@ -25,6 +26,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;
> 
> another void* arithmetic. consider using well-defined constructs, like
> 
> cpu = (CPUEntry *)(_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 = cpu->feat_sief2;
>> +			break;
> 
> this only checks CPU 0. I wonder if you shouldn't check all CPUs? Or if
> we assume that all CPUs have the same facilities, isn't it enough to
> check the first CPU in the list? (i.e. avoid the loop)

This is the way.

Thomas already asked me that. I had a look what the kernel does and
that's what they are doing. QEMU writes the same feature bits to all
cpus and I haven't found an explanation for that code yet but I figured
there might (have) be(en) one.

> 
>> +		}
>> +	}
>> +}
>> +
>>  /* 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 acd86d5..6c86037 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -92,12 +92,22 @@ typedef struct SCCBHeader {
>>  typedef struct CPUEntry {
>>      uint8_t address;
>>      uint8_t reserved0;
>> -    uint8_t features[SCCB_CPU_FEATURE_LEN];
>> +    uint8_t : 4;
>> +    uint8_t feat_sief2 : 1;
>> +    uint8_t : 3;
>> +    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
>>      uint8_t reserved2[6];
>>      uint8_t type;
>>      uint8_t reserved1;
>>  } __attribute__((packed)) CPUEntry;
>>  
>> +extern struct sclp_facilities sclp_facilities;
>> +
>> +struct sclp_facilities {
>> +	uint64_t has_sief2 : 1;
>> +	uint64_t : 63;
>> +};
>> +
>>  typedef struct ReadInfo {
>>      SCCBHeader h;
>>      uint16_t rnmax;
>> @@ -271,6 +281,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);
> 

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

[Thomas Huth]

On 17/12/2020 16.21, Janosch Frank wrote:
> On 12/17/20 1:18 PM, Claudio Imbrenda wrote:
>> On Fri, 11 Dec 2020 05:00:34 -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>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  lib/s390x/io.c   |  1 +
>>>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>>>  lib/s390x/sclp.h | 13 ++++++++++++-
>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
>>> index 6a1da63..ef9f59e 100644
>>> --- a/lib/s390x/io.c
>>> +++ b/lib/s390x/io.c
>>> @@ -35,6 +35,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 bf1d9c0..cf6ea7c 100644
>>> --- a/lib/s390x/sclp.c
>>> +++ b/lib/s390x/sclp.c
>>> @@ -9,6 +9,7 @@
>>>   */
>>>  
>>>  #include <libcflat.h>
>>> +#include <bitops.h>
>>
>> you add this include, but it seems you are not actually using it?
> 
> Leftover from last version
> 
>>
>>>  #include <asm/page.h>
>>>  #include <asm/arch_def.h>
>>>  #include <asm/interrupt.h>
>>> @@ -25,6 +26,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;
>>
>> another void* arithmetic. consider using well-defined constructs, like
>>
>> cpu = (CPUEntry *)(_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 = cpu->feat_sief2;
>>> +			break;
>>
>> this only checks CPU 0. I wonder if you shouldn't check all CPUs? Or if
>> we assume that all CPUs have the same facilities, isn't it enough to
>> check the first CPU in the list? (i.e. avoid the loop)
> 
> This is the way.
> 
> Thomas already asked me that. I had a look what the kernel does and
> that's what they are doing. QEMU writes the same feature bits to all
> cpus and I haven't found an explanation for that code yet but I figured
> there might (have) be(en) one.

Well, if two people are asking, that's maybe a good indication that a
comment in the code would be a good idea? (even if it just references to the
kernel way of doing it?)

 Thomas

[kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Janosch Frank]

I've added too much to cstart64.S which is not start related
already. Now that I want to add even more code it's time to split
cstart64.S. lib.S has functions that are used in tests. macros.S
contains macros which are used in cstart64.S and lib.S

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/Makefile             |   8 +--
 s390x/{ => asm}/cstart64.S | 119 ++-----------------------------------
 s390x/asm/lib.S            |  65 ++++++++++++++++++++
 s390x/asm/macros.S         |  77 ++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 119 deletions(-)
 rename s390x/{ => asm}/cstart64.S (50%)
 create mode 100644 s390x/asm/lib.S
 create mode 100644 s390x/asm/macros.S

diff --git a/s390x/Makefile b/s390x/Makefile
index b079a26..fb62e87 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -66,10 +66,10 @@ cflatobjs += lib/s390x/css_lib.o
 
 OBJDIRS += lib/s390x
 
-cstart.o = $(TEST_DIR)/cstart64.o
+asmlib = $(TEST_DIR)/asm/cstart64.o $(TEST_DIR)/asm/lib.o
 
 FLATLIBS = $(libcflat)
-%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(cstart.o)
+%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
 	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) \
 		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
 	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
@@ -87,7 +87,7 @@ FLATLIBS = $(libcflat)
 	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
 
 arch_clean: asm_offsets_clean
-	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
+	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d $(TEST_DIR)/asm/*.{o,elf,bin} $(TEST_DIR)/asm/.*.d
 
 generated-files = $(asm-offsets)
-$(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files)
+$(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)
diff --git a/s390x/cstart64.S b/s390x/asm/cstart64.S
similarity index 50%
rename from s390x/cstart64.S
rename to s390x/asm/cstart64.S
index cc86fc7..ace0c0d 100644
--- a/s390x/cstart64.S
+++ b/s390x/asm/cstart64.S
@@ -3,14 +3,17 @@
  * s390x startup code
  *
  * Copyright (c) 2017 Red Hat Inc
+ * Copyright (c) 2019 IBM Corp.
  *
  * Authors:
  *  Thomas Huth <thuth@redhat.com>
  *  David Hildenbrand <david@redhat.com>
+ *  Janosch Frank <frankja@linux.ibm.com>
  */
 #include <asm/asm-offsets.h>
 #include <asm/sigp.h>
 
+#include "macros.S"
 .section .init
 
 /*
@@ -87,120 +90,7 @@ clear_bss_remainder:
 memsetxc:
 	xc 0(1,%r1),0(%r1)
 
-	.macro SAVE_REGS
-	/* save grs 0-15 */
-	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
-	/* save crs 0-15 */
-	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
-	/* load a cr0 that has the AFP control bit which enables all FPRs */
-	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	/* save fprs 0-15 + fpc */
-	la	%r1, GEN_LC_SW_INT_FPRS
-	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	std	\i, \i * 8(%r1)
-	.endr
-	stfpc	GEN_LC_SW_INT_FPC
-	.endm
-
-	.macro RESTORE_REGS
-	/* restore fprs 0-15 + fpc */
-	la	%r1, GEN_LC_SW_INT_FPRS
-	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	ld	\i, \i * 8(%r1)
-	.endr
-	lfpc	GEN_LC_SW_INT_FPC
-	/* restore crs 0-15 */
-	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
-	/* restore grs 0-15 */
-	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
-	.endm
-
-/* Save registers on the stack (r15), so we can have stacked interrupts. */
-	.macro SAVE_REGS_STACK
-	/* Allocate a stack frame for 15 general registers */
-	slgfi   %r15, 15 * 8
-	/* Store registers r0 to r14 on the stack */
-	stmg    %r0, %r14, 0(%r15)
-	/* Allocate a stack frame for 16 floating point registers */
-	/* The size of a FP register is the size of an double word */
-	slgfi   %r15, 16 * 8
-	/* Save fp register on stack: offset to SP is multiple of reg number */
-	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	std	\i, \i * 8(%r15)
-	.endr
-	/* Save fpc, but keep stack aligned on 64bits */
-	slgfi   %r15, 8
-	efpc	%r0
-	stg	%r0, 0(%r15)
-	.endm
-
-/* Restore the register in reverse order */
-	.macro RESTORE_REGS_STACK
-	/* Restore fpc */
-	lfpc	0(%r15)
-	algfi	%r15, 8
-	/* Restore fp register from stack: SP still where it was left */
-	/* and offset to SP is a multiple of reg number */
-	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	ld	\i, \i * 8(%r15)
-	.endr
-	/* Now that we're done, rewind the stack pointer by 16 double word */
-	algfi   %r15, 16 * 8
-	/* Load the registers from stack */
-	lmg     %r0, %r14, 0(%r15)
-	/* Rewind the stack by 15 double word */
-	algfi   %r15, 15 * 8
-	.endm
-
 .section .text
-/*
- * load_reset calling convention:
- * %r2 subcode (0 or 1)
- */
-.globl diag308_load_reset
-diag308_load_reset:
-	SAVE_REGS
-	/* Backup current PSW mask, as we have to restore it on success */
-	epsw	%r0, %r1
-	st	%r0, GEN_LC_SW_INT_PSW
-	st	%r1, GEN_LC_SW_INT_PSW + 4
-	/* Load reset psw mask (short psw, 64 bit) */
-	lg	%r0, reset_psw
-	/* Load the success label address */
-	larl    %r1, 0f
-	/* Or it to the mask */
-	ogr	%r0, %r1
-	/* Store it at the reset PSW location (real 0x0) */
-	stg	%r0, 0
-	/* Do the reset */
-	diag    %r0,%r2,0x308
-	/* Failure path */
-	xgr	%r2, %r2
-	br	%r14
-	/* Success path */
-	/* load a cr0 that has the AFP control bit which enables all FPRs */
-0:	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
-	lhi	%r2, 1
-	larl	%r0, 1f
-	stg	%r0, GEN_LC_SW_INT_PSW + 8
-	lpswe	GEN_LC_SW_INT_PSW
-1:	br	%r14
-
-.globl smp_cpu_setup_state
-smp_cpu_setup_state:
-	xgr	%r1, %r1
-	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
-	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
-	/* We should only go once through cpu setup and not for every restart */
-	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
-	larl	%r14, 0f
-	lpswe	GEN_LC_SW_INT_PSW
-	/* If the function returns, just loop here */
-0:	j	0
-
 pgm_int:
 	SAVE_REGS
 	brasl	%r14, handle_pgm_int
@@ -232,8 +122,6 @@ svc_int:
 	lpswe	GEN_LC_SVC_OLD_PSW
 
 	.align	8
-reset_psw:
-	.quad	0x0008000180000000
 initial_psw:
 	.quad	0x0000000180000000, clear_bss_start
 pgm_int_psw:
@@ -246,6 +134,7 @@ io_int_psw:
 	.quad	0x0000000180000000, io_int
 svc_int_psw:
 	.quad	0x0000000180000000, svc_int
+.globl initial_cr0
 initial_cr0:
 	/* enable AFP-register control, so FP regs (+BFP instr) can be used */
 	.quad	0x0000000000040000
diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
new file mode 100644
index 0000000..4d78ec6
--- /dev/null
+++ b/s390x/asm/lib.S
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * s390x assembly library
+ *
+ * Copyright (c) 2019 IBM Corp.
+ *
+ * Authors:
+ *    Janosch Frank <frankja@linux.ibm.com>
+ */
+#include <asm/asm-offsets.h>
+#include <asm/sigp.h>
+
+#include "macros.S"
+
+/*
+ * load_reset calling convention:
+ * %r2 subcode (0 or 1)
+ */
+.globl diag308_load_reset
+diag308_load_reset:
+	SAVE_REGS
+	/* Backup current PSW mask, as we have to restore it on success */
+	epsw	%r0, %r1
+	st	%r0, GEN_LC_SW_INT_PSW
+	st	%r1, GEN_LC_SW_INT_PSW + 4
+	/* Load reset psw mask (short psw, 64 bit) */
+	lg	%r0, reset_psw
+	/* Load the success label address */
+	larl    %r1, 0f
+	/* Or it to the mask */
+	ogr	%r0, %r1
+	/* Store it at the reset PSW location (real 0x0) */
+	stg	%r0, 0
+	/* Do the reset */
+	diag    %r0,%r2,0x308
+	/* Failure path */
+	xgr	%r2, %r2
+	br	%r14
+	/* Success path */
+	/* load a cr0 that has the AFP control bit which enables all FPRs */
+0:	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)
+	RESTORE_REGS
+	lhi	%r2, 1
+	larl	%r0, 1f
+	stg	%r0, GEN_LC_SW_INT_PSW + 8
+	lpswe	GEN_LC_SW_INT_PSW
+1:	br	%r14
+
+/* Sets up general registers and cr0 when a new cpu is brought online. */
+.globl smp_cpu_setup_state
+smp_cpu_setup_state:
+	xgr	%r1, %r1
+	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
+	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
+	/* We should only go once through cpu setup and not for every restart */
+	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
+	larl	%r14, 0f
+	lpswe	GEN_LC_SW_INT_PSW
+	/* If the function returns, just loop here */
+0:	j	0
+
+	.align	8
+reset_psw:
+	.quad	0x0008000180000000
diff --git a/s390x/asm/macros.S b/s390x/asm/macros.S
new file mode 100644
index 0000000..37a6a63
--- /dev/null
+++ b/s390x/asm/macros.S
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * s390x assembly macros
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ * Copyright (c) 2020 IBM Corp.
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *  David Hildenbrand <david@redhat.com>
+ */
+#include <asm/asm-offsets.h>
+	.macro SAVE_REGS
+	/* save grs 0-15 */
+	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
+	/* save crs 0-15 */
+	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
+	/* load a cr0 that has the AFP control bit which enables all FPRs */
+	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)
+	/* save fprs 0-15 + fpc */
+	la	%r1, GEN_LC_SW_INT_FPRS
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r1)
+	.endr
+	stfpc	GEN_LC_SW_INT_FPC
+	.endm
+
+	.macro RESTORE_REGS
+	/* restore fprs 0-15 + fpc */
+	la	%r1, GEN_LC_SW_INT_FPRS
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r1)
+	.endr
+	lfpc	GEN_LC_SW_INT_FPC
+	/* restore crs 0-15 */
+	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
+	/* restore grs 0-15 */
+	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
+	.endm
+
+/* Save registers on the stack (r15), so we can have stacked interrupts. */
+	.macro SAVE_REGS_STACK
+	/* Allocate a stack frame for 15 general registers */
+	slgfi   %r15, 15 * 8
+	/* Store registers r0 to r14 on the stack */
+	stmg    %r0, %r14, 0(%r15)
+	/* Allocate a stack frame for 16 floating point registers */
+	/* The size of a FP register is the size of an double word */
+	slgfi   %r15, 16 * 8
+	/* Save fp register on stack: offset to SP is multiple of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	std	\i, \i * 8(%r15)
+	.endr
+	/* Save fpc, but keep stack aligned on 64bits */
+	slgfi   %r15, 8
+	efpc	%r0
+	stg	%r0, 0(%r15)
+	.endm
+
+/* Restore the register in reverse order */
+	.macro RESTORE_REGS_STACK
+	/* Restore fpc */
+	lfpc	0(%r15)
+	algfi	%r15, 8
+	/* Restore fp register from stack: SP still where it was left */
+	/* and offset to SP is a multiple of reg number */
+	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
+	ld	\i, \i * 8(%r15)
+	.endr
+	/* Now that we're done, rewind the stack pointer by 16 double word */
+	algfi   %r15, 16 * 8
+	/* Load the registers from stack */
+	lmg     %r0, %r14, 0(%r15)
+	/* Rewind the stack by 15 double word */
+	algfi   %r15, 15 * 8
+	.endm
-- 
2.25.1

Re: [kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Thomas Huth]

On 11/12/2020 11.00, Janosch Frank wrote:
> I've added too much to cstart64.S which is not start related
> already. Now that I want to add even more code it's time to split
> cstart64.S. lib.S has functions that are used in tests. macros.S
> contains macros which are used in cstart64.S and lib.S
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile             |   8 +--
>  s390x/{ => asm}/cstart64.S | 119 ++-----------------------------------
>  s390x/asm/lib.S            |  65 ++++++++++++++++++++
>  s390x/asm/macros.S         |  77 ++++++++++++++++++++++++
>  4 files changed, 150 insertions(+), 119 deletions(-)
>  rename s390x/{ => asm}/cstart64.S (50%)
>  create mode 100644 s390x/asm/lib.S
>  create mode 100644 s390x/asm/macros.S
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..fb62e87 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -66,10 +66,10 @@ cflatobjs += lib/s390x/css_lib.o
>  
>  OBJDIRS += lib/s390x
>  
> -cstart.o = $(TEST_DIR)/cstart64.o
> +asmlib = $(TEST_DIR)/asm/cstart64.o $(TEST_DIR)/asm/lib.o
>  
>  FLATLIBS = $(libcflat)
> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(cstart.o)
> +%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>  	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
> @@ -87,7 +87,7 @@ FLATLIBS = $(libcflat)
>  	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
>  
>  arch_clean: asm_offsets_clean
> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
> +	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d $(TEST_DIR)/asm/*.{o,elf,bin} $(TEST_DIR)/asm/.*.d
>  
>  generated-files = $(asm-offsets)
> -$(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files)
> +$(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)

Did you check this with both, in-tree and out-of-tree builds?
(I wonder whether that new asm directory needs some special handling for
out-of-tree builds?)

> diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
> new file mode 100644
> index 0000000..4d78ec6
> --- /dev/null
> +++ b/s390x/asm/lib.S
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * s390x assembly library
> + *
> + * Copyright (c) 2019 IBM Corp.
> + *
> + * Authors:
> + *    Janosch Frank <frankja@linux.ibm.com>
> + */
> +#include <asm/asm-offsets.h>
> +#include <asm/sigp.h>
> +
> +#include "macros.S"
> +
> +/*
> + * load_reset calling convention:
> + * %r2 subcode (0 or 1)
> + */
> +.globl diag308_load_reset
> +diag308_load_reset:

Thinking about this twice ... this function is only used by s390x/diag308.c,
so it's not really a library function, but rather part of a single test ...
I think it would be cleaner to put it into a separate file instead, what do
you think?

 Thomas

Re: [kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Janosch Frank]

On 12/11/20 1:18 PM, Thomas Huth wrote:
> On 11/12/2020 11.00, Janosch Frank wrote:
>> I've added too much to cstart64.S which is not start related
>> already. Now that I want to add even more code it's time to split
>> cstart64.S. lib.S has functions that are used in tests. macros.S
>> contains macros which are used in cstart64.S and lib.S
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/Makefile             |   8 +--
>>  s390x/{ => asm}/cstart64.S | 119 ++-----------------------------------
>>  s390x/asm/lib.S            |  65 ++++++++++++++++++++
>>  s390x/asm/macros.S         |  77 ++++++++++++++++++++++++
>>  4 files changed, 150 insertions(+), 119 deletions(-)
>>  rename s390x/{ => asm}/cstart64.S (50%)
>>  create mode 100644 s390x/asm/lib.S
>>  create mode 100644 s390x/asm/macros.S
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index b079a26..fb62e87 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -66,10 +66,10 @@ cflatobjs += lib/s390x/css_lib.o
>>  
>>  OBJDIRS += lib/s390x
>>  
>> -cstart.o = $(TEST_DIR)/cstart64.o
>> +asmlib = $(TEST_DIR)/asm/cstart64.o $(TEST_DIR)/asm/lib.o
>>  
>>  FLATLIBS = $(libcflat)
>> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(cstart.o)
>> +%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>>  	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) \
>>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
>>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
>> @@ -87,7 +87,7 @@ FLATLIBS = $(libcflat)
>>  	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT) --no-verify --image $< -o $@
>>  
>>  arch_clean: asm_offsets_clean
>> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d
>> +	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d lib/s390x/.*.d $(TEST_DIR)/asm/*.{o,elf,bin} $(TEST_DIR)/asm/.*.d
>>  
>>  generated-files = $(asm-offsets)
>> -$(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files)
>> +$(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)
> 
> Did you check this with both, in-tree and out-of-tree builds?
> (I wonder whether that new asm directory needs some special handling for
> out-of-tree builds?)

I'm not a big fan of out-of-tree builds, so I didn't check.
To get those builds working we would need to create the asm directory in
the $testdir

> 
>> diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
>> new file mode 100644
>> index 0000000..4d78ec6
>> --- /dev/null
>> +++ b/s390x/asm/lib.S
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * s390x assembly library
>> + *
>> + * Copyright (c) 2019 IBM Corp.
>> + *
>> + * Authors:
>> + *    Janosch Frank <frankja@linux.ibm.com>
>> + */
>> +#include <asm/asm-offsets.h>
>> +#include <asm/sigp.h>
>> +
>> +#include "macros.S"
>> +
>> +/*
>> + * load_reset calling convention:
>> + * %r2 subcode (0 or 1)
>> + */
>> +.globl diag308_load_reset
>> +diag308_load_reset:
> 
> Thinking about this twice ... this function is only used by s390x/diag308.c,
> so it's not really a library function, but rather part of a single test ...
> I think it would be cleaner to put it into a separate file instead, what do
> you think?

I don't really want to split this any further.
Moving the asm files into an own directory already improves readability
a lot for me and I don't need more files if they aren't absolutely
necessary.

> 
>  Thomas
> 
> 

Re: [kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:35 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> I've added too much to cstart64.S which is not start related
> already. Now that I want to add even more code it's time to split
> cstart64.S. lib.S has functions that are used in tests. macros.S
> contains macros which are used in cstart64.S and lib.S

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile             |   8 +--
>  s390x/{ => asm}/cstart64.S | 119
> ++----------------------------------- s390x/asm/lib.S            |
> 65 ++++++++++++++++++++ s390x/asm/macros.S         |  77
> ++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 119
> deletions(-) rename s390x/{ => asm}/cstart64.S (50%)
>  create mode 100644 s390x/asm/lib.S
>  create mode 100644 s390x/asm/macros.S
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b079a26..fb62e87 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -66,10 +66,10 @@ cflatobjs += lib/s390x/css_lib.o
>  
>  OBJDIRS += lib/s390x
>  
> -cstart.o = $(TEST_DIR)/cstart64.o
> +asmlib = $(TEST_DIR)/asm/cstart64.o $(TEST_DIR)/asm/lib.o
>  
>  FLATLIBS = $(libcflat)
> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(cstart.o)
> +%.elf: %.o $(FLATLIBS) $(SRCDIR)/s390x/flat.lds $(asmlib)
>  	$(CC) $(CFLAGS) -c -o $(@:.elf=.aux.o) \
>  		$(SRCDIR)/lib/auxinfo.c -DPROGNAME=\"$@\"
>  	$(CC) $(LDFLAGS) -o $@ -T $(SRCDIR)/s390x/flat.lds \
> @@ -87,7 +87,7 @@ FLATLIBS = $(libcflat)
>  	$(GENPROTIMG) --host-key-document $(HOST_KEY_DOCUMENT)
> --no-verify --image $< -o $@ 
>  arch_clean: asm_offsets_clean
> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d
> lib/s390x/.*.d
> +	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(TEST_DIR)/.*.d
> lib/s390x/.*.d $(TEST_DIR)/asm/*.{o,elf,bin} $(TEST_DIR)/asm/.*.d 
>  generated-files = $(asm-offsets)
> -$(tests:.elf=.o) $(cstart.o) $(cflatobjs): $(generated-files)
> +$(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)
> diff --git a/s390x/cstart64.S b/s390x/asm/cstart64.S
> similarity index 50%
> rename from s390x/cstart64.S
> rename to s390x/asm/cstart64.S
> index cc86fc7..ace0c0d 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/asm/cstart64.S
> @@ -3,14 +3,17 @@
>   * s390x startup code
>   *
>   * Copyright (c) 2017 Red Hat Inc
> + * Copyright (c) 2019 IBM Corp.
>   *
>   * Authors:
>   *  Thomas Huth <thuth@redhat.com>
>   *  David Hildenbrand <david@redhat.com>
> + *  Janosch Frank <frankja@linux.ibm.com>
>   */
>  #include <asm/asm-offsets.h>
>  #include <asm/sigp.h>
>  
> +#include "macros.S"
>  .section .init
>  
>  /*
> @@ -87,120 +90,7 @@ clear_bss_remainder:
>  memsetxc:
>  	xc 0(1,%r1),0(%r1)
>  
> -	.macro SAVE_REGS
> -	/* save grs 0-15 */
> -	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
> -	/* save crs 0-15 */
> -	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
> -	/* load a cr0 that has the AFP control bit which enables all
> FPRs */
> -	larl	%r1, initial_cr0
> -	lctlg	%c0, %c0, 0(%r1)
> -	/* save fprs 0-15 + fpc */
> -	la	%r1, GEN_LC_SW_INT_FPRS
> -	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -	std	\i, \i * 8(%r1)
> -	.endr
> -	stfpc	GEN_LC_SW_INT_FPC
> -	.endm
> -
> -	.macro RESTORE_REGS
> -	/* restore fprs 0-15 + fpc */
> -	la	%r1, GEN_LC_SW_INT_FPRS
> -	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -	ld	\i, \i * 8(%r1)
> -	.endr
> -	lfpc	GEN_LC_SW_INT_FPC
> -	/* restore crs 0-15 */
> -	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
> -	/* restore grs 0-15 */
> -	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
> -	.endm
> -
> -/* Save registers on the stack (r15), so we can have stacked
> interrupts. */
> -	.macro SAVE_REGS_STACK
> -	/* Allocate a stack frame for 15 general registers */
> -	slgfi   %r15, 15 * 8
> -	/* Store registers r0 to r14 on the stack */
> -	stmg    %r0, %r14, 0(%r15)
> -	/* Allocate a stack frame for 16 floating point registers */
> -	/* The size of a FP register is the size of an double word */
> -	slgfi   %r15, 16 * 8
> -	/* Save fp register on stack: offset to SP is multiple of
> reg number */
> -	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -	std	\i, \i * 8(%r15)
> -	.endr
> -	/* Save fpc, but keep stack aligned on 64bits */
> -	slgfi   %r15, 8
> -	efpc	%r0
> -	stg	%r0, 0(%r15)
> -	.endm
> -
> -/* Restore the register in reverse order */
> -	.macro RESTORE_REGS_STACK
> -	/* Restore fpc */
> -	lfpc	0(%r15)
> -	algfi	%r15, 8
> -	/* Restore fp register from stack: SP still where it was
> left */
> -	/* and offset to SP is a multiple of reg number */
> -	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -	ld	\i, \i * 8(%r15)
> -	.endr
> -	/* Now that we're done, rewind the stack pointer by 16
> double word */
> -	algfi   %r15, 16 * 8
> -	/* Load the registers from stack */
> -	lmg     %r0, %r14, 0(%r15)
> -	/* Rewind the stack by 15 double word */
> -	algfi   %r15, 15 * 8
> -	.endm
> -
>  .section .text
> -/*
> - * load_reset calling convention:
> - * %r2 subcode (0 or 1)
> - */
> -.globl diag308_load_reset
> -diag308_load_reset:
> -	SAVE_REGS
> -	/* Backup current PSW mask, as we have to restore it on
> success */
> -	epsw	%r0, %r1
> -	st	%r0, GEN_LC_SW_INT_PSW
> -	st	%r1, GEN_LC_SW_INT_PSW + 4
> -	/* Load reset psw mask (short psw, 64 bit) */
> -	lg	%r0, reset_psw
> -	/* Load the success label address */
> -	larl    %r1, 0f
> -	/* Or it to the mask */
> -	ogr	%r0, %r1
> -	/* Store it at the reset PSW location (real 0x0) */
> -	stg	%r0, 0
> -	/* Do the reset */
> -	diag    %r0,%r2,0x308
> -	/* Failure path */
> -	xgr	%r2, %r2
> -	br	%r14
> -	/* Success path */
> -	/* load a cr0 that has the AFP control bit which enables all
> FPRs */ -0:	larl	%r1, initial_cr0
> -	lctlg	%c0, %c0, 0(%r1)
> -	RESTORE_REGS
> -	lhi	%r2, 1
> -	larl	%r0, 1f
> -	stg	%r0, GEN_LC_SW_INT_PSW + 8
> -	lpswe	GEN_LC_SW_INT_PSW
> -1:	br	%r14
> -
> -.globl smp_cpu_setup_state
> -smp_cpu_setup_state:
> -	xgr	%r1, %r1
> -	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
> -	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
> -	/* We should only go once through cpu setup and not for
> every restart */
> -	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
> -	larl	%r14, 0f
> -	lpswe	GEN_LC_SW_INT_PSW
> -	/* If the function returns, just loop here */
> -0:	j	0
> -
>  pgm_int:
>  	SAVE_REGS
>  	brasl	%r14, handle_pgm_int
> @@ -232,8 +122,6 @@ svc_int:
>  	lpswe	GEN_LC_SVC_OLD_PSW
>  
>  	.align	8
> -reset_psw:
> -	.quad	0x0008000180000000
>  initial_psw:
>  	.quad	0x0000000180000000, clear_bss_start
>  pgm_int_psw:
> @@ -246,6 +134,7 @@ io_int_psw:
>  	.quad	0x0000000180000000, io_int
>  svc_int_psw:
>  	.quad	0x0000000180000000, svc_int
> +.globl initial_cr0
>  initial_cr0:
>  	/* enable AFP-register control, so FP regs (+BFP instr) can
> be used */ .quad	0x0000000000040000
> diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
> new file mode 100644
> index 0000000..4d78ec6
> --- /dev/null
> +++ b/s390x/asm/lib.S
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * s390x assembly library
> + *
> + * Copyright (c) 2019 IBM Corp.
> + *
> + * Authors:
> + *    Janosch Frank <frankja@linux.ibm.com>
> + */
> +#include <asm/asm-offsets.h>
> +#include <asm/sigp.h>
> +
> +#include "macros.S"
> +
> +/*
> + * load_reset calling convention:
> + * %r2 subcode (0 or 1)
> + */
> +.globl diag308_load_reset
> +diag308_load_reset:
> +	SAVE_REGS
> +	/* Backup current PSW mask, as we have to restore it on
> success */
> +	epsw	%r0, %r1
> +	st	%r0, GEN_LC_SW_INT_PSW
> +	st	%r1, GEN_LC_SW_INT_PSW + 4
> +	/* Load reset psw mask (short psw, 64 bit) */
> +	lg	%r0, reset_psw
> +	/* Load the success label address */
> +	larl    %r1, 0f
> +	/* Or it to the mask */
> +	ogr	%r0, %r1
> +	/* Store it at the reset PSW location (real 0x0) */
> +	stg	%r0, 0
> +	/* Do the reset */
> +	diag    %r0,%r2,0x308
> +	/* Failure path */
> +	xgr	%r2, %r2
> +	br	%r14
> +	/* Success path */
> +	/* load a cr0 that has the AFP control bit which enables all
> FPRs */ +0:	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
> +	RESTORE_REGS
> +	lhi	%r2, 1
> +	larl	%r0, 1f
> +	stg	%r0, GEN_LC_SW_INT_PSW + 8
> +	lpswe	GEN_LC_SW_INT_PSW
> +1:	br	%r14
> +
> +/* Sets up general registers and cr0 when a new cpu is brought
> online. */ +.globl smp_cpu_setup_state
> +smp_cpu_setup_state:
> +	xgr	%r1, %r1
> +	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
> +	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
> +	/* We should only go once through cpu setup and not for
> every restart */
> +	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
> +	larl	%r14, 0f
> +	lpswe	GEN_LC_SW_INT_PSW
> +	/* If the function returns, just loop here */
> +0:	j	0
> +
> +	.align	8
> +reset_psw:
> +	.quad	0x0008000180000000
> diff --git a/s390x/asm/macros.S b/s390x/asm/macros.S
> new file mode 100644
> index 0000000..37a6a63
> --- /dev/null
> +++ b/s390x/asm/macros.S
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * s390x assembly macros
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + * Copyright (c) 2020 IBM Corp.
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *  David Hildenbrand <david@redhat.com>
> + */
> +#include <asm/asm-offsets.h>
> +	.macro SAVE_REGS
> +	/* save grs 0-15 */
> +	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
> +	/* save crs 0-15 */
> +	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
> +	/* load a cr0 that has the AFP control bit which enables all
> FPRs */
> +	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
> +	/* save fprs 0-15 + fpc */
> +	la	%r1, GEN_LC_SW_INT_FPRS
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	std	\i, \i * 8(%r1)
> +	.endr
> +	stfpc	GEN_LC_SW_INT_FPC
> +	.endm
> +
> +	.macro RESTORE_REGS
> +	/* restore fprs 0-15 + fpc */
> +	la	%r1, GEN_LC_SW_INT_FPRS
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r1)
> +	.endr
> +	lfpc	GEN_LC_SW_INT_FPC
> +	/* restore crs 0-15 */
> +	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
> +	/* restore grs 0-15 */
> +	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
> +	.endm
> +
> +/* Save registers on the stack (r15), so we can have stacked
> interrupts. */
> +	.macro SAVE_REGS_STACK
> +	/* Allocate a stack frame for 15 general registers */
> +	slgfi   %r15, 15 * 8
> +	/* Store registers r0 to r14 on the stack */
> +	stmg    %r0, %r14, 0(%r15)
> +	/* Allocate a stack frame for 16 floating point registers */
> +	/* The size of a FP register is the size of an double word */
> +	slgfi   %r15, 16 * 8
> +	/* Save fp register on stack: offset to SP is multiple of
> reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	std	\i, \i * 8(%r15)
> +	.endr
> +	/* Save fpc, but keep stack aligned on 64bits */
> +	slgfi   %r15, 8
> +	efpc	%r0
> +	stg	%r0, 0(%r15)
> +	.endm
> +
> +/* Restore the register in reverse order */
> +	.macro RESTORE_REGS_STACK
> +	/* Restore fpc */
> +	lfpc	0(%r15)
> +	algfi	%r15, 8
> +	/* Restore fp register from stack: SP still where it was
> left */
> +	/* and offset to SP is a multiple of reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r15)
> +	.endr
> +	/* Now that we're done, rewind the stack pointer by 16
> double word */
> +	algfi   %r15, 16 * 8
> +	/* Load the registers from stack */
> +	lmg     %r0, %r14, 0(%r15)
> +	/* Rewind the stack by 15 double word */
> +	algfi   %r15, 15 * 8
> +	.endm

Re: [kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:35 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> I've added too much to cstart64.S which is not start related
> already. Now that I want to add even more code it's time to split
> cstart64.S. lib.S has functions that are used in tests. macros.S
> contains macros which are used in cstart64.S and lib.S
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile             |   8 +--
>  s390x/{ => asm}/cstart64.S | 119
> ++----------------------------------- s390x/asm/lib.S            |
> 65 ++++++++++++++++++++ s390x/asm/macros.S         |  77
> ++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 119
> deletions(-) rename s390x/{ => asm}/cstart64.S (50%)
>  create mode 100644 s390x/asm/lib.S
>  create mode 100644 s390x/asm/macros.S 

[...]

> diff --git a/s390x/cstart64.S b/s390x/asm/cstart64.S
> similarity index 50%
> rename from s390x/cstart64.S
> rename to s390x/asm/cstart64.S
> index cc86fc7..ace0c0d 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/asm/cstart64.S
> @@ -3,14 +3,17 @@
>   * s390x startup code
>   *
>   * Copyright (c) 2017 Red Hat Inc
> + * Copyright (c) 2019 IBM Corp.

2020 ?

>   *
>   * Authors:
>   *  Thomas Huth <thuth@redhat.com>
>   *  David Hildenbrand <david@redhat.com>
> + *  Janosch Frank <frankja@linux.ibm.com>
>   */
>  #include <asm/asm-offsets.h>
>  #include <asm/sigp.h>

[...]

> diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
> new file mode 100644
> index 0000000..4d78ec6
> --- /dev/null
> +++ b/s390x/asm/lib.S
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * s390x assembly library
> + *
> + * Copyright (c) 2019 IBM Corp.

also 2020?

> + *
> + * Authors:
> + *    Janosch Frank <frankja@linux.ibm.com>
> + */
> +#include <asm/asm-offsets.h>
> +#include <asm/sigp.h>
> +
> +#include "macros.S"
> +
> +/*
> + * load_reset calling convention:
> + * %r2 subcode (0 or 1)
> + */
> +.globl diag308_load_reset
> +diag308_load_reset:
> +	SAVE_REGS
> +	/* Backup current PSW mask, as we have to restore it on
> success */
> +	epsw	%r0, %r1
> +	st	%r0, GEN_LC_SW_INT_PSW
> +	st	%r1, GEN_LC_SW_INT_PSW + 4
> +	/* Load reset psw mask (short psw, 64 bit) */
> +	lg	%r0, reset_psw
> +	/* Load the success label address */
> +	larl    %r1, 0f
> +	/* Or it to the mask */
> +	ogr	%r0, %r1
> +	/* Store it at the reset PSW location (real 0x0) */
> +	stg	%r0, 0
> +	/* Do the reset */
> +	diag    %r0,%r2,0x308
> +	/* Failure path */
> +	xgr	%r2, %r2
> +	br	%r14
> +	/* Success path */
> +	/* load a cr0 that has the AFP control bit which enables all
> FPRs */ +0:	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
> +	RESTORE_REGS
> +	lhi	%r2, 1
> +	larl	%r0, 1f
> +	stg	%r0, GEN_LC_SW_INT_PSW + 8
> +	lpswe	GEN_LC_SW_INT_PSW
> +1:	br	%r14
> +
> +/* Sets up general registers and cr0 when a new cpu is brought
> online. */ +.globl smp_cpu_setup_state
> +smp_cpu_setup_state:
> +	xgr	%r1, %r1
> +	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
> +	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
> +	/* We should only go once through cpu setup and not for
> every restart */
> +	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
> +	larl	%r14, 0f
> +	lpswe	GEN_LC_SW_INT_PSW
> +	/* If the function returns, just loop here */
> +0:	j	0
> +
> +	.align	8
> +reset_psw:
> +	.quad	0x0008000180000000
> diff --git a/s390x/asm/macros.S b/s390x/asm/macros.S
> new file mode 100644
> index 0000000..37a6a63
> --- /dev/null
> +++ b/s390x/asm/macros.S
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * s390x assembly macros
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + * Copyright (c) 2020 IBM Corp.
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *  David Hildenbrand <david@redhat.com>
> + */
> +#include <asm/asm-offsets.h>
> +	.macro SAVE_REGS
> +	/* save grs 0-15 */
> +	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
> +	/* save crs 0-15 */
> +	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
> +	/* load a cr0 that has the AFP control bit which enables all
> FPRs */
> +	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
> +	/* save fprs 0-15 + fpc */
> +	la	%r1, GEN_LC_SW_INT_FPRS
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	std	\i, \i * 8(%r1)
> +	.endr
> +	stfpc	GEN_LC_SW_INT_FPC
> +	.endm
> +
> +	.macro RESTORE_REGS
> +	/* restore fprs 0-15 + fpc */
> +	la	%r1, GEN_LC_SW_INT_FPRS
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r1)
> +	.endr
> +	lfpc	GEN_LC_SW_INT_FPC
> +	/* restore crs 0-15 */
> +	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
> +	/* restore grs 0-15 */
> +	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
> +	.endm
> +
> +/* Save registers on the stack (r15), so we can have stacked
> interrupts. */
> +	.macro SAVE_REGS_STACK
> +	/* Allocate a stack frame for 15 general registers */
> +	slgfi   %r15, 15 * 8
> +	/* Store registers r0 to r14 on the stack */
> +	stmg    %r0, %r14, 0(%r15)
> +	/* Allocate a stack frame for 16 floating point registers */
> +	/* The size of a FP register is the size of an double word */
> +	slgfi   %r15, 16 * 8
> +	/* Save fp register on stack: offset to SP is multiple of
> reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	std	\i, \i * 8(%r15)
> +	.endr
> +	/* Save fpc, but keep stack aligned on 64bits */
> +	slgfi   %r15, 8
> +	efpc	%r0
> +	stg	%r0, 0(%r15)
> +	.endm
> +
> +/* Restore the register in reverse order */
> +	.macro RESTORE_REGS_STACK
> +	/* Restore fpc */
> +	lfpc	0(%r15)
> +	algfi	%r15, 8
> +	/* Restore fp register from stack: SP still where it was
> left */
> +	/* and offset to SP is a multiple of reg number */
> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> +	ld	\i, \i * 8(%r15)
> +	.endr
> +	/* Now that we're done, rewind the stack pointer by 16
> double word */
> +	algfi   %r15, 16 * 8
> +	/* Load the registers from stack */
> +	lmg     %r0, %r14, 0(%r15)
> +	/* Rewind the stack by 15 double word */
> +	algfi   %r15, 15 * 8
> +	.endm

Re: [kvm-unit-tests PATCH v3 4/8] s390x: Split assembly and move to s390x/asm/

[Janosch Frank]

On 12/17/20 2:14 PM, Claudio Imbrenda wrote:
> On Fri, 11 Dec 2020 05:00:35 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> I've added too much to cstart64.S which is not start related
>> already. Now that I want to add even more code it's time to split
>> cstart64.S. lib.S has functions that are used in tests. macros.S
>> contains macros which are used in cstart64.S and lib.S
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  s390x/Makefile             |   8 +--
>>  s390x/{ => asm}/cstart64.S | 119
>> ++----------------------------------- s390x/asm/lib.S            |
>> 65 ++++++++++++++++++++ s390x/asm/macros.S         |  77
>> ++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 119
>> deletions(-) rename s390x/{ => asm}/cstart64.S (50%)
>>  create mode 100644 s390x/asm/lib.S
>>  create mode 100644 s390x/asm/macros.S 
> 
> [...]
> 
>> diff --git a/s390x/cstart64.S b/s390x/asm/cstart64.S
>> similarity index 50%
>> rename from s390x/cstart64.S
>> rename to s390x/asm/cstart64.S
>> index cc86fc7..ace0c0d 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/asm/cstart64.S
>> @@ -3,14 +3,17 @@
>>   * s390x startup code
>>   *
>>   * Copyright (c) 2017 Red Hat Inc
>> + * Copyright (c) 2019 IBM Corp.
> 
> 2020 ?

Moving stuff changes the copyright?

> 
>>   *
>>   * Authors:
>>   *  Thomas Huth <thuth@redhat.com>
>>   *  David Hildenbrand <david@redhat.com>
>> + *  Janosch Frank <frankja@linux.ibm.com>
>>   */
>>  #include <asm/asm-offsets.h>
>>  #include <asm/sigp.h>
> 
> [...]
> 
>> diff --git a/s390x/asm/lib.S b/s390x/asm/lib.S
>> new file mode 100644
>> index 0000000..4d78ec6
>> --- /dev/null
>> +++ b/s390x/asm/lib.S
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * s390x assembly library
>> + *
>> + * Copyright (c) 2019 IBM Corp.
> 
> also 2020?
> 
>> + *
>> + * Authors:
>> + *    Janosch Frank <frankja@linux.ibm.com>
>> + */
>> +#include <asm/asm-offsets.h>
>> +#include <asm/sigp.h>
>> +
>> +#include "macros.S"
>> +
>> +/*
>> + * load_reset calling convention:
>> + * %r2 subcode (0 or 1)
>> + */
>> +.globl diag308_load_reset
>> +diag308_load_reset:
>> +	SAVE_REGS
>> +	/* Backup current PSW mask, as we have to restore it on
>> success */
>> +	epsw	%r0, %r1
>> +	st	%r0, GEN_LC_SW_INT_PSW
>> +	st	%r1, GEN_LC_SW_INT_PSW + 4
>> +	/* Load reset psw mask (short psw, 64 bit) */
>> +	lg	%r0, reset_psw
>> +	/* Load the success label address */
>> +	larl    %r1, 0f
>> +	/* Or it to the mask */
>> +	ogr	%r0, %r1
>> +	/* Store it at the reset PSW location (real 0x0) */
>> +	stg	%r0, 0
>> +	/* Do the reset */
>> +	diag    %r0,%r2,0x308
>> +	/* Failure path */
>> +	xgr	%r2, %r2
>> +	br	%r14
>> +	/* Success path */
>> +	/* load a cr0 that has the AFP control bit which enables all
>> FPRs */ +0:	larl	%r1, initial_cr0
>> +	lctlg	%c0, %c0, 0(%r1)
>> +	RESTORE_REGS
>> +	lhi	%r2, 1
>> +	larl	%r0, 1f
>> +	stg	%r0, GEN_LC_SW_INT_PSW + 8
>> +	lpswe	GEN_LC_SW_INT_PSW
>> +1:	br	%r14
>> +
>> +/* Sets up general registers and cr0 when a new cpu is brought
>> online. */ +.globl smp_cpu_setup_state
>> +smp_cpu_setup_state:
>> +	xgr	%r1, %r1
>> +	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
>> +	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
>> +	/* We should only go once through cpu setup and not for
>> every restart */
>> +	stg	%r14, GEN_LC_RESTART_NEW_PSW + 8
>> +	larl	%r14, 0f
>> +	lpswe	GEN_LC_SW_INT_PSW
>> +	/* If the function returns, just loop here */
>> +0:	j	0
>> +
>> +	.align	8
>> +reset_psw:
>> +	.quad	0x0008000180000000
>> diff --git a/s390x/asm/macros.S b/s390x/asm/macros.S
>> new file mode 100644
>> index 0000000..37a6a63
>> --- /dev/null
>> +++ b/s390x/asm/macros.S
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * s390x assembly macros
>> + *
>> + * Copyright (c) 2017 Red Hat Inc
>> + * Copyright (c) 2020 IBM Corp.
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *  David Hildenbrand <david@redhat.com>
>> + */
>> +#include <asm/asm-offsets.h>
>> +	.macro SAVE_REGS
>> +	/* save grs 0-15 */
>> +	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>> +	/* save crs 0-15 */
>> +	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
>> +	/* load a cr0 that has the AFP control bit which enables all
>> FPRs */
>> +	larl	%r1, initial_cr0
>> +	lctlg	%c0, %c0, 0(%r1)
>> +	/* save fprs 0-15 + fpc */
>> +	la	%r1, GEN_LC_SW_INT_FPRS
>> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>> +	std	\i, \i * 8(%r1)
>> +	.endr
>> +	stfpc	GEN_LC_SW_INT_FPC
>> +	.endm
>> +
>> +	.macro RESTORE_REGS
>> +	/* restore fprs 0-15 + fpc */
>> +	la	%r1, GEN_LC_SW_INT_FPRS
>> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>> +	ld	\i, \i * 8(%r1)
>> +	.endr
>> +	lfpc	GEN_LC_SW_INT_FPC
>> +	/* restore crs 0-15 */
>> +	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
>> +	/* restore grs 0-15 */
>> +	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
>> +	.endm
>> +
>> +/* Save registers on the stack (r15), so we can have stacked
>> interrupts. */
>> +	.macro SAVE_REGS_STACK
>> +	/* Allocate a stack frame for 15 general registers */
>> +	slgfi   %r15, 15 * 8
>> +	/* Store registers r0 to r14 on the stack */
>> +	stmg    %r0, %r14, 0(%r15)
>> +	/* Allocate a stack frame for 16 floating point registers */
>> +	/* The size of a FP register is the size of an double word */
>> +	slgfi   %r15, 16 * 8
>> +	/* Save fp register on stack: offset to SP is multiple of
>> reg number */
>> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>> +	std	\i, \i * 8(%r15)
>> +	.endr
>> +	/* Save fpc, but keep stack aligned on 64bits */
>> +	slgfi   %r15, 8
>> +	efpc	%r0
>> +	stg	%r0, 0(%r15)
>> +	.endm
>> +
>> +/* Restore the register in reverse order */
>> +	.macro RESTORE_REGS_STACK
>> +	/* Restore fpc */
>> +	lfpc	0(%r15)
>> +	algfi	%r15, 8
>> +	/* Restore fp register from stack: SP still where it was
>> left */
>> +	/* and offset to SP is a multiple of reg number */
>> +	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>> +	ld	\i, \i * 8(%r15)
>> +	.endr
>> +	/* Now that we're done, rewind the stack pointer by 16
>> double word */
>> +	algfi   %r15, 16 * 8
>> +	/* Load the registers from stack */
>> +	lmg     %r0, %r14, 0(%r15)
>> +	/* Rewind the stack by 15 double word */
>> +	algfi   %r15, 15 * 8
>> +	.endm
> 

[kvm-unit-tests PATCH v3 5/8] 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/asm/lib.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 ee94ed3..35697de 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -8,6 +8,7 @@
 #include <libcflat.h>
 #include <kbuild.h>
 #include <asm/arch_def.h>
+#include <sie.h>
 
 int main(void)
 {
@@ -69,6 +70,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 f3ab830..5a13cf2 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -8,6 +8,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 bac8862..3858096 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -11,6 +11,7 @@
 #include <asm/barrier.h>
 #include <sclp.h>
 #include <interrupt.h>
+#include <sie.h>
 
 static bool pgm_int_expected;
 static bool ext_int_expected;
@@ -57,6 +58,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/asm/lib.S b/s390x/asm/lib.S
index 4d78ec6..5267f02 100644
--- a/s390x/asm/lib.S
+++ b/s390x/asm/lib.S
@@ -60,6 +60,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
+
 	.align	8
 reset_psw:
 	.quad	0x0008000180000000
-- 
2.25.1

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

[Thomas Huth]

On 11/12/2020 11.00, Janosch Frank wrote:
> 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/asm/lib.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 ee94ed3..35697de 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -8,6 +8,7 @@
>  #include <libcflat.h>
>  #include <kbuild.h>
>  #include <asm/arch_def.h>
> +#include <sie.h>
>  
>  int main(void)
>  {
> @@ -69,6 +70,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 f3ab830..5a13cf2 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -8,6 +8,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];

I think you can drop empty2 ?

> +};
> +
>  struct psw {
>  	uint64_t	mask;
>  	uint64_t	addr;
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index bac8862..3858096 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -11,6 +11,7 @@
>  #include <asm/barrier.h>
>  #include <sclp.h>
>  #include <interrupt.h>
> +#include <sie.h>
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> @@ -57,6 +58,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) {

Can you please explain that "magic" number 10 in the comment?

> +		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
[...]
> +extern u64 sie_entry;
> +extern u64 sie_exit;

Maybe better:

extern uint16_t sie_entry[];
extern uint16_t sie_exit[];

?

Or even:

extern void sie_entry();
extern void sie_exit();

?

 Thomas

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

[Janosch Frank]

On 12/17/20 10:37 AM, Thomas Huth wrote:
> On 11/12/2020 11.00, Janosch Frank wrote:
>> 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/asm/lib.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 ee94ed3..35697de 100644
>> --- a/lib/s390x/asm-offsets.c
>> +++ b/lib/s390x/asm-offsets.c
>> @@ -8,6 +8,7 @@
>>  #include <libcflat.h>
>>  #include <kbuild.h>
>>  #include <asm/arch_def.h>
>> +#include <sie.h>
>>  
>>  int main(void)
>>  {
>> @@ -69,6 +70,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 f3ab830..5a13cf2 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -8,6 +8,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];
> 
> I think you can drop empty2 ?

Since I don't need to allocate it I could also remove the gprs. We only
use empty1 right now as far as I know.

> 
>> +};
>> +
>>  struct psw {
>>  	uint64_t	mask;
>>  	uint64_t	addr;
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index bac8862..3858096 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -11,6 +11,7 @@
>>  #include <asm/barrier.h>
>>  #include <sclp.h>
>>  #include <interrupt.h>
>> +#include <sie.h>
>>  
>>  static bool pgm_int_expected;
>>  static bool ext_int_expected;
>> @@ -57,6 +58,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) {
> 
> Can you please explain that "magic" number 10 in the comment?

I think using sie_exit would make more sense than explaining that
sie_entry + 10 bytes is the location of sie_exit.


> 
>> +		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
> [...]
>> +extern u64 sie_entry;
>> +extern u64 sie_exit;
> 
> Maybe better:
> 
> extern uint16_t sie_entry[];
> extern uint16_t sie_exit[];
> 
> ?
> 
> Or even:
> 
> extern void sie_entry();
> extern void sie_exit();

Definitely better since I don't return values in sie_exit anymore (I
used to before).

> 
> ?
> 
>  Thomas
> 

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

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:36 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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/asm/lib.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 ee94ed3..35697de 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -8,6 +8,7 @@
>  #include <libcflat.h>
>  #include <kbuild.h>
>  #include <asm/arch_def.h>
> +#include <sie.h>
>  
>  int main(void)
>  {
> @@ -69,6 +70,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 f3ab830..5a13cf2 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -8,6 +8,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 bac8862..3858096 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -11,6 +11,7 @@
>  #include <asm/barrier.h>
>  #include <sclp.h>
>  #include <interrupt.h>
> +#include <sie.h>
>  
>  static bool pgm_int_expected;
>  static bool ext_int_expected;
> @@ -57,6 +58,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) {

can't you use sie_exit?

> +		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/asm/lib.S b/s390x/asm/lib.S
> index 4d78ec6..5267f02 100644
> --- a/s390x/asm/lib.S
> +++ b/s390x/asm/lib.S
> @@ -60,6 +60,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
> +
>  	.align	8
>  reset_psw:
>  	.quad	0x0008000180000000

[kvm-unit-tests PATCH v3 6/8] 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         | 113 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   3 ++
 3 files changed, 117 insertions(+)
 create mode 100644 s390x/sie.c

diff --git a/s390x/Makefile b/s390x/Makefile
index fb62e87..8e1b4e9 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..cfc746f
--- /dev/null
+++ b/s390x/sie.c
@@ -0,0 +1,113 @@
+#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 == ICPT_VALIDITY)
+			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 test_diag(u32 instr)
+{
+	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 == ICPT_INST &&
+	       vm.sblk->ipa == instr >> 16 && vm.sblk->ipb == instr << 16,
+	       "Intercept data");
+	sblk_cleanup(&vm);
+}
+
+static struct {
+	const char *name;
+	u32 instr;
+} tests[] = {
+	{ "10", 0x83020010 },
+	{ "44", 0x83020044 },
+	{ "9c", 0x8302009c },
+	{ NULL, 0 }
+};
+
+static void test_diags(void)
+{
+	int i;
+
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		test_diag(tests[i].instr);
+		report_prefix_pop();
+	}
+}
+
+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();
+	test_diags();
+done:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 3feb8bc..2298be6 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -96,3 +96,6 @@ smp = 2
 
 [uv-guest]
 file = uv-guest.elf
+
+[sie]
+file = sie.elf
-- 
2.25.1

Re: [kvm-unit-tests PATCH v3 6/8] s390x: sie: Add first SIE test

[Thomas Huth]

On 11/12/2020 11.00, Janosch Frank wrote:
> 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         | 113 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 ++
>  3 files changed, 117 insertions(+)
>  create mode 100644 s390x/sie.c

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

Re: [kvm-unit-tests PATCH v3 6/8] s390x: sie: Add first SIE test

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:37 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

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

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  s390x/Makefile      |   1 +
>  s390x/sie.c         | 113
> ++++++++++++++++++++++++++++++++++++++++++++ s390x/unittests.cfg |
> 3 ++ 3 files changed, 117 insertions(+)
>  create mode 100644 s390x/sie.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index fb62e87..8e1b4e9 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..cfc746f
> --- /dev/null
> +++ b/s390x/sie.c
> @@ -0,0 +1,113 @@
> +#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 == ICPT_VALIDITY)
> +			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 test_diag(u32 instr)
> +{
> +	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 == ICPT_INST &&
> +	       vm.sblk->ipa == instr >> 16 && vm.sblk->ipb == instr
> << 16,
> +	       "Intercept data");
> +	sblk_cleanup(&vm);
> +}
> +
> +static struct {
> +	const char *name;
> +	u32 instr;
> +} tests[] = {
> +	{ "10", 0x83020010 },
> +	{ "44", 0x83020044 },
> +	{ "9c", 0x8302009c },
> +	{ NULL, 0 }
> +};
> +
> +static void test_diags(void)
> +{
> +	int i;
> +
> +	for (i = 0; tests[i].name; i++) {
> +		report_prefix_push(tests[i].name);
> +		test_diag(tests[i].instr);
> +		report_prefix_pop();
> +	}
> +}
> +
> +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();
> +	test_diags();
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3feb8bc..2298be6 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -96,3 +96,6 @@ smp = 2
>  
>  [uv-guest]
>  file = uv-guest.elf
> +
> +[sie]
> +file = sie.elf

Re: [kvm-unit-tests PATCH v3 6/8] s390x: sie: Add first SIE test

[Cornelia Huck]

On Fri, 11 Dec 2020 05:00:37 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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         | 113 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   3 ++
>  3 files changed, 117 insertions(+)
>  create mode 100644 s390x/sie.c

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

[kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Janosch Frank]

Not much to test except for the privilege and specification
exceptions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/sclp.c  |  2 ++
 lib/s390x/sclp.h  |  6 +++++-
 s390x/intercept.c | 19 +++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index cf6ea7c..0001993 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
 
 	assert(read_info);
 
+	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
+
 	cpu = (void *)read_info + read_info->offset_cpu;
 	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
 		if (cpu->address == cpu0_addr) {
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 6c86037..58f8e54 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
 
 struct sclp_facilities {
 	uint64_t has_sief2 : 1;
-	uint64_t : 63;
+	uint64_t has_diag318 : 1;
+	uint64_t : 62;
 };
 
 typedef struct ReadInfo {
@@ -130,6 +131,9 @@ typedef struct ReadInfo {
     uint16_t highest_cpu;
     uint8_t  _reserved5[124 - 122];     /* 122-123 */
     uint32_t hmfai;
+    uint8_t reserved7[134 - 128];
+    uint8_t byte_134_diag318 : 1;
+    uint8_t : 7;
     struct CPUEntry entries[0];
 } __attribute__((packed)) ReadInfo;
 
diff --git a/s390x/intercept.c b/s390x/intercept.c
index cde2f5f..86e57e1 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -8,6 +8,7 @@
  *  Thomas Huth <thuth@redhat.com>
  */
 #include <libcflat.h>
+#include <sclp.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
@@ -152,6 +153,23 @@ static void test_testblock(void)
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
+static void test_diag318(void)
+{
+	expect_pgm_int();
+	enter_pstate();
+	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
+	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
+
+	if (!sclp_facilities.has_diag318)
+		expect_pgm_int();
+
+	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
+
+	if (!sclp_facilities.has_diag318)
+		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+
+}
+
 struct {
 	const char *name;
 	void (*func)(void);
@@ -162,6 +180,7 @@ struct {
 	{ "stap", test_stap, false },
 	{ "stidp", test_stidp, false },
 	{ "testblock", test_testblock, false },
+	{ "diag318", test_diag318, false },
 	{ NULL, NULL, false }
 };
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Thomas Huth]

On 11/12/2020 11.00, Janosch Frank wrote:
> Not much to test except for the privilege and specification
> exceptions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/sclp.c  |  2 ++
>  lib/s390x/sclp.h  |  6 +++++-
>  s390x/intercept.c | 19 +++++++++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index cf6ea7c..0001993 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>  
>  	assert(read_info);
>  
> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
> +
>  	cpu = (void *)read_info + read_info->offset_cpu;
>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>  		if (cpu->address == cpu0_addr) {
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 6c86037..58f8e54 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>  
>  struct sclp_facilities {
>  	uint64_t has_sief2 : 1;
> -	uint64_t : 63;
> +	uint64_t has_diag318 : 1;
> +	uint64_t : 62;
>  };
>  
>  typedef struct ReadInfo {
> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>      uint16_t highest_cpu;
>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>      uint32_t hmfai;
> +    uint8_t reserved7[134 - 128];
> +    uint8_t byte_134_diag318 : 1;
> +    uint8_t : 7;
>      struct CPUEntry entries[0];

... the entries[] array can be moved around here without any further ado?
Looks confusing to me. Should there be a CPUEntry array here at all, or only
in ReadCpuInfo?

>  } __attribute__((packed)) ReadInfo;
>  

 Thomas

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Christian Borntraeger]

On 17.12.20 10:53, Thomas Huth wrote:
> On 11/12/2020 11.00, Janosch Frank wrote:
>> Not much to test except for the privilege and specification
>> exceptions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/s390x/sclp.c  |  2 ++
>>  lib/s390x/sclp.h  |  6 +++++-
>>  s390x/intercept.c | 19 +++++++++++++++++++
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index cf6ea7c..0001993 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>  
>>  	assert(read_info);
>>  
>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>> +
>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>  		if (cpu->address == cpu0_addr) {
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index 6c86037..58f8e54 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>  
>>  struct sclp_facilities {
>>  	uint64_t has_sief2 : 1;
>> -	uint64_t : 63;
>> +	uint64_t has_diag318 : 1;
>> +	uint64_t : 62;
>>  };
>>  
>>  typedef struct ReadInfo {
>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>      uint16_t highest_cpu;
>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>      uint32_t hmfai;
>> +    uint8_t reserved7[134 - 128];
>> +    uint8_t byte_134_diag318 : 1;
>> +    uint8_t : 7;
>>      struct CPUEntry entries[0];
> 
> ... the entries[] array can be moved around here without any further ado?
> Looks confusing to me. Should there be a CPUEntry array here at all, or only
> in ReadCpuInfo?

there is offset_cpu for the cpu entries at the beginning of the structure.

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Thomas Huth]

On 17/12/2020 10.59, Christian Borntraeger wrote:
> 
> 
> On 17.12.20 10:53, Thomas Huth wrote:
>> On 11/12/2020 11.00, Janosch Frank wrote:
>>> Not much to test except for the privilege and specification
>>> exceptions.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  lib/s390x/sclp.c  |  2 ++
>>>  lib/s390x/sclp.h  |  6 +++++-
>>>  s390x/intercept.c | 19 +++++++++++++++++++
>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>> index cf6ea7c..0001993 100644
>>> --- a/lib/s390x/sclp.c
>>> +++ b/lib/s390x/sclp.c
>>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>>  
>>>  	assert(read_info);
>>>  
>>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>>> +
>>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>>  		if (cpu->address == cpu0_addr) {
>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>> index 6c86037..58f8e54 100644
>>> --- a/lib/s390x/sclp.h
>>> +++ b/lib/s390x/sclp.h
>>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>>  
>>>  struct sclp_facilities {
>>>  	uint64_t has_sief2 : 1;
>>> -	uint64_t : 63;
>>> +	uint64_t has_diag318 : 1;
>>> +	uint64_t : 62;
>>>  };
>>>  
>>>  typedef struct ReadInfo {
>>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>>      uint16_t highest_cpu;
>>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>>      uint32_t hmfai;
>>> +    uint8_t reserved7[134 - 128];
>>> +    uint8_t byte_134_diag318 : 1;
>>> +    uint8_t : 7;
>>>      struct CPUEntry entries[0];
>>
>> ... the entries[] array can be moved around here without any further ado?
>> Looks confusing to me. Should there be a CPUEntry array here at all, or only
>> in ReadCpuInfo?
> 
> there is offset_cpu for the cpu entries at the beginning of the structure.

Ah, thanks, right, this was used earlier in the patch series, now I
remember. But I think the "struct CPUEntry entries[0]" here is rather
confusing, since there is no guarantee that the entries are really at this
location ... I think this line should rather be replaced by a comment saying
that offset_cpu should be used instead.

 Thomas

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Janosch Frank]

On 12/17/20 11:34 AM, Thomas Huth wrote:
> On 17/12/2020 10.59, Christian Borntraeger wrote:
>>
>>
>> On 17.12.20 10:53, Thomas Huth wrote:
>>> On 11/12/2020 11.00, Janosch Frank wrote:
>>>> Not much to test except for the privilege and specification
>>>> exceptions.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  lib/s390x/sclp.c  |  2 ++
>>>>  lib/s390x/sclp.h  |  6 +++++-
>>>>  s390x/intercept.c | 19 +++++++++++++++++++
>>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>>> index cf6ea7c..0001993 100644
>>>> --- a/lib/s390x/sclp.c
>>>> +++ b/lib/s390x/sclp.c
>>>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>>>  
>>>>  	assert(read_info);
>>>>  
>>>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>>>> +
>>>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>>>  		if (cpu->address == cpu0_addr) {
>>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>>> index 6c86037..58f8e54 100644
>>>> --- a/lib/s390x/sclp.h
>>>> +++ b/lib/s390x/sclp.h
>>>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>>>  
>>>>  struct sclp_facilities {
>>>>  	uint64_t has_sief2 : 1;
>>>> -	uint64_t : 63;
>>>> +	uint64_t has_diag318 : 1;
>>>> +	uint64_t : 62;
>>>>  };
>>>>  
>>>>  typedef struct ReadInfo {
>>>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>>>      uint16_t highest_cpu;
>>>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>>>      uint32_t hmfai;
>>>> +    uint8_t reserved7[134 - 128];
>>>> +    uint8_t byte_134_diag318 : 1;
>>>> +    uint8_t : 7;
>>>>      struct CPUEntry entries[0];
>>>
>>> ... the entries[] array can be moved around here without any further ado?
>>> Looks confusing to me. Should there be a CPUEntry array here at all, or only
>>> in ReadCpuInfo?
>>
>> there is offset_cpu for the cpu entries at the beginning of the structure.
> 
> Ah, thanks, right, this was used earlier in the patch series, now I
> remember. But I think the "struct CPUEntry entries[0]" here is rather
> confusing, since there is no guarantee that the entries are really at this
> location ... I think this line should rather be replaced by a comment saying
> that offset_cpu should be used instead.

Sure, as long as it's clear that there's something at the end, I'm fine
with it.

> 
>  Thomas
> 

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Janosch Frank]

On 12/17/20 3:31 PM, Janosch Frank wrote:
> On 12/17/20 11:34 AM, Thomas Huth wrote:
>> On 17/12/2020 10.59, Christian Borntraeger wrote:
>>>
>>>
>>> On 17.12.20 10:53, Thomas Huth wrote:
>>>> On 11/12/2020 11.00, Janosch Frank wrote:
>>>>> Not much to test except for the privilege and specification
>>>>> exceptions.
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  lib/s390x/sclp.c  |  2 ++
>>>>>  lib/s390x/sclp.h  |  6 +++++-
>>>>>  s390x/intercept.c | 19 +++++++++++++++++++
>>>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>>>> index cf6ea7c..0001993 100644
>>>>> --- a/lib/s390x/sclp.c
>>>>> +++ b/lib/s390x/sclp.c
>>>>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>>>>  
>>>>>  	assert(read_info);
>>>>>  
>>>>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>>>>> +
>>>>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>>>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>>>>  		if (cpu->address == cpu0_addr) {
>>>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>>>> index 6c86037..58f8e54 100644
>>>>> --- a/lib/s390x/sclp.h
>>>>> +++ b/lib/s390x/sclp.h
>>>>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>>>>  
>>>>>  struct sclp_facilities {
>>>>>  	uint64_t has_sief2 : 1;
>>>>> -	uint64_t : 63;
>>>>> +	uint64_t has_diag318 : 1;
>>>>> +	uint64_t : 62;
>>>>>  };
>>>>>  
>>>>>  typedef struct ReadInfo {
>>>>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>>>>      uint16_t highest_cpu;
>>>>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>>>>      uint32_t hmfai;
>>>>> +    uint8_t reserved7[134 - 128];
>>>>> +    uint8_t byte_134_diag318 : 1;
>>>>> +    uint8_t : 7;
>>>>>      struct CPUEntry entries[0];
>>>>
>>>> ... the entries[] array can be moved around here without any further ado?
>>>> Looks confusing to me. Should there be a CPUEntry array here at all, or only
>>>> in ReadCpuInfo?
>>>
>>> there is offset_cpu for the cpu entries at the beginning of the structure.
>>
>> Ah, thanks, right, this was used earlier in the patch series, now I
>> remember. But I think the "struct CPUEntry entries[0]" here is rather
>> confusing, since there is no guarantee that the entries are really at this
>> location ... I think this line should rather be replaced by a comment saying
>> that offset_cpu should be used instead.
> 
> Sure, as long as it's clear that there's something at the end, I'm fine
> with it.

I would add that to the "fix style issues" patch or into an own patch.
Any preferences?

-       struct CPUEntry entries[0];
+       /*
+        * The cpu entries follow, they start at the offset specified
+        * in offset_cpu.
+        */




> 
>>
>>  Thomas
>>
> 

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Thomas Huth]

On 17/12/2020 16.31, Janosch Frank wrote:
> On 12/17/20 3:31 PM, Janosch Frank wrote:
>> On 12/17/20 11:34 AM, Thomas Huth wrote:
>>> On 17/12/2020 10.59, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 17.12.20 10:53, Thomas Huth wrote:
>>>>> On 11/12/2020 11.00, Janosch Frank wrote:
>>>>>> Not much to test except for the privilege and specification
>>>>>> exceptions.
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  lib/s390x/sclp.c  |  2 ++
>>>>>>  lib/s390x/sclp.h  |  6 +++++-
>>>>>>  s390x/intercept.c | 19 +++++++++++++++++++
>>>>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>>>>>> index cf6ea7c..0001993 100644
>>>>>> --- a/lib/s390x/sclp.c
>>>>>> +++ b/lib/s390x/sclp.c
>>>>>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>>>>>  
>>>>>>  	assert(read_info);
>>>>>>  
>>>>>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>>>>>> +
>>>>>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>>>>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>>>>>  		if (cpu->address == cpu0_addr) {
>>>>>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>>>>>> index 6c86037..58f8e54 100644
>>>>>> --- a/lib/s390x/sclp.h
>>>>>> +++ b/lib/s390x/sclp.h
>>>>>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>>>>>  
>>>>>>  struct sclp_facilities {
>>>>>>  	uint64_t has_sief2 : 1;
>>>>>> -	uint64_t : 63;
>>>>>> +	uint64_t has_diag318 : 1;
>>>>>> +	uint64_t : 62;
>>>>>>  };
>>>>>>  
>>>>>>  typedef struct ReadInfo {
>>>>>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>>>>>      uint16_t highest_cpu;
>>>>>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>>>>>      uint32_t hmfai;
>>>>>> +    uint8_t reserved7[134 - 128];
>>>>>> +    uint8_t byte_134_diag318 : 1;
>>>>>> +    uint8_t : 7;
>>>>>>      struct CPUEntry entries[0];
>>>>>
>>>>> ... the entries[] array can be moved around here without any further ado?
>>>>> Looks confusing to me. Should there be a CPUEntry array here at all, or only
>>>>> in ReadCpuInfo?
>>>>
>>>> there is offset_cpu for the cpu entries at the beginning of the structure.
>>>
>>> Ah, thanks, right, this was used earlier in the patch series, now I
>>> remember. But I think the "struct CPUEntry entries[0]" here is rather
>>> confusing, since there is no guarantee that the entries are really at this
>>> location ... I think this line should rather be replaced by a comment saying
>>> that offset_cpu should be used instead.
>>
>> Sure, as long as it's clear that there's something at the end, I'm fine
>> with it.
> 
> I would add that to the "fix style issues" patch or into an own patch.
> Any preferences?

I think a separate patch is cleaner.

> -       struct CPUEntry entries[0];
> +       /*
> +        * The cpu entries follow, they start at the offset specified
> +        * in offset_cpu.
> +        */

Sounds good, thanks!

 Thomas

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:38 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Not much to test except for the privilege and specification
> exceptions.

This patch looks fine. But I wonder what is it doing in this series?
The series is about SIE testing, and this seems to be an unrelated
improvement in an existing testcase?

anyway, looks good to me

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/sclp.c  |  2 ++
>  lib/s390x/sclp.h  |  6 +++++-
>  s390x/intercept.c | 19 +++++++++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index cf6ea7c..0001993 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>  
>  	assert(read_info);
>  
> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
> +
>  	cpu = (void *)read_info + read_info->offset_cpu;
>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>  		if (cpu->address == cpu0_addr) {
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 6c86037..58f8e54 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>  
>  struct sclp_facilities {
>  	uint64_t has_sief2 : 1;
> -	uint64_t : 63;
> +	uint64_t has_diag318 : 1;
> +	uint64_t : 62;
>  };
>  
>  typedef struct ReadInfo {
> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>      uint16_t highest_cpu;
>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>      uint32_t hmfai;
> +    uint8_t reserved7[134 - 128];
> +    uint8_t byte_134_diag318 : 1;
> +    uint8_t : 7;
>      struct CPUEntry entries[0];
>  } __attribute__((packed)) ReadInfo;
>  
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index cde2f5f..86e57e1 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -8,6 +8,7 @@
>   *  Thomas Huth <thuth@redhat.com>
>   */
>  #include <libcflat.h>
> +#include <sclp.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/interrupt.h>
>  #include <asm/page.h>
> @@ -152,6 +153,23 @@ static void test_testblock(void)
>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>  }
>  
> +static void test_diag318(void)
> +{
> +	expect_pgm_int();
> +	enter_pstate();
> +	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
> +
> +	if (!sclp_facilities.has_diag318)
> +		expect_pgm_int();
> +
> +	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
> +
> +	if (!sclp_facilities.has_diag318)
> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +
> +}
> +
>  struct {
>  	const char *name;
>  	void (*func)(void);
> @@ -162,6 +180,7 @@ struct {
>  	{ "stap", test_stap, false },
>  	{ "stidp", test_stidp, false },
>  	{ "testblock", test_testblock, false },
> +	{ "diag318", test_diag318, false },
>  	{ NULL, NULL, false }
>  };
>  

Re: [kvm-unit-tests PATCH v3 7/8] s390x: Add diag318 intercept test

[Janosch Frank]

On 12/17/20 3:58 PM, Claudio Imbrenda wrote:
> On Fri, 11 Dec 2020 05:00:38 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Not much to test except for the privilege and specification
>> exceptions.
> 
> This patch looks fine. But I wonder what is it doing in this series?
> The series is about SIE testing, and this seems to be an unrelated
> improvement in an existing testcase?

I didn't want to split my patches up in multiple series and this depends
on the sclp changes.

> 
> anyway, looks good to me
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks

> 
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/s390x/sclp.c  |  2 ++
>>  lib/s390x/sclp.h  |  6 +++++-
>>  s390x/intercept.c | 19 +++++++++++++++++++
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
>> index cf6ea7c..0001993 100644
>> --- a/lib/s390x/sclp.c
>> +++ b/lib/s390x/sclp.c
>> @@ -138,6 +138,8 @@ void sclp_facilities_setup(void)
>>  
>>  	assert(read_info);
>>  
>> +	sclp_facilities.has_diag318 = read_info->byte_134_diag318;
>> +
>>  	cpu = (void *)read_info + read_info->offset_cpu;
>>  	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
>>  		if (cpu->address == cpu0_addr) {
>> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
>> index 6c86037..58f8e54 100644
>> --- a/lib/s390x/sclp.h
>> +++ b/lib/s390x/sclp.h
>> @@ -105,7 +105,8 @@ extern struct sclp_facilities sclp_facilities;
>>  
>>  struct sclp_facilities {
>>  	uint64_t has_sief2 : 1;
>> -	uint64_t : 63;
>> +	uint64_t has_diag318 : 1;
>> +	uint64_t : 62;
>>  };
>>  
>>  typedef struct ReadInfo {
>> @@ -130,6 +131,9 @@ typedef struct ReadInfo {
>>      uint16_t highest_cpu;
>>      uint8_t  _reserved5[124 - 122];     /* 122-123 */
>>      uint32_t hmfai;
>> +    uint8_t reserved7[134 - 128];
>> +    uint8_t byte_134_diag318 : 1;
>> +    uint8_t : 7;
>>      struct CPUEntry entries[0];
>>  } __attribute__((packed)) ReadInfo;
>>  
>> diff --git a/s390x/intercept.c b/s390x/intercept.c
>> index cde2f5f..86e57e1 100644
>> --- a/s390x/intercept.c
>> +++ b/s390x/intercept.c
>> @@ -8,6 +8,7 @@
>>   *  Thomas Huth <thuth@redhat.com>
>>   */
>>  #include <libcflat.h>
>> +#include <sclp.h>
>>  #include <asm/asm-offsets.h>
>>  #include <asm/interrupt.h>
>>  #include <asm/page.h>
>> @@ -152,6 +153,23 @@ static void test_testblock(void)
>>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>>  }
>>  
>> +static void test_diag318(void)
>> +{
>> +	expect_pgm_int();
>> +	enter_pstate();
>> +	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
>> +	check_pgm_int_code(PGM_INT_CODE_PRIVILEGED_OPERATION);
>> +
>> +	if (!sclp_facilities.has_diag318)
>> +		expect_pgm_int();
>> +
>> +	asm volatile("diag %0,0,0x318\n" : : "d" (0x42));
>> +
>> +	if (!sclp_facilities.has_diag318)
>> +		check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +
>> +}
>> +
>>  struct {
>>  	const char *name;
>>  	void (*func)(void);
>> @@ -162,6 +180,7 @@ struct {
>>  	{ "stap", test_stap, false },
>>  	{ "stidp", test_stidp, false },
>>  	{ "testblock", test_testblock, false },
>> +	{ "diag318", test_diag318, false },
>>  	{ NULL, NULL, false }
>>  };
>>  
> 

[kvm-unit-tests PATCH v3 8/8] s390x: Fix sclp.h style issues

[Janosch Frank]

Fix indentation

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 lib/s390x/sclp.h | 172 +++++++++++++++++++++++------------------------
 1 file changed, 86 insertions(+), 86 deletions(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 58f8e54..dccbaa8 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -79,10 +79,10 @@
 #define SCCB_SIZE 4096
 
 typedef struct SCCBHeader {
-    uint16_t length;
-    uint8_t function_code;
-    uint8_t control_mask[3];
-    uint16_t response_code;
+	uint16_t length;
+	uint8_t function_code;
+	uint8_t control_mask[3];
+	uint16_t response_code;
 } __attribute__((packed)) SCCBHeader;
 
 #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
@@ -90,15 +90,15 @@ typedef struct SCCBHeader {
 
 /* CPU information */
 typedef struct CPUEntry {
-    uint8_t address;
-    uint8_t reserved0;
-    uint8_t : 4;
-    uint8_t feat_sief2 : 1;
-    uint8_t : 3;
-    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
-    uint8_t reserved2[6];
-    uint8_t type;
-    uint8_t reserved1;
+	uint8_t address;
+	uint8_t reserved0;
+	uint8_t : 4;
+	uint8_t feat_sief2 : 1;
+	uint8_t : 3;
+	uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
+	uint8_t reserved2[6];
+	uint8_t type;
+	uint8_t reserved1;
 } __attribute__((packed)) CPUEntry;
 
 extern struct sclp_facilities sclp_facilities;
@@ -110,77 +110,77 @@ struct sclp_facilities {
 };
 
 typedef struct ReadInfo {
-    SCCBHeader h;
-    uint16_t rnmax;
-    uint8_t rnsize;
-    uint8_t  _reserved1[16 - 11];       /* 11-15 */
-    uint16_t entries_cpu;               /* 16-17 */
-    uint16_t offset_cpu;                /* 18-19 */
-    uint8_t  _reserved2[24 - 20];       /* 20-23 */
-    uint8_t  loadparm[8];               /* 24-31 */
-    uint8_t  _reserved3[48 - 32];       /* 32-47 */
-    uint64_t facilities;                /* 48-55 */
-    uint8_t  _reserved0[76 - 56];       /* 56-75 */
-    uint32_t ibc_val;
-    uint8_t  conf_char[99 - 80];        /* 80-98 */
-    uint8_t mha_pow;
-    uint32_t rnsize2;
-    uint64_t rnmax2;
-    uint8_t  _reserved6[116 - 112];     /* 112-115 */
-    uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
-    uint16_t highest_cpu;
-    uint8_t  _reserved5[124 - 122];     /* 122-123 */
-    uint32_t hmfai;
-    uint8_t reserved7[134 - 128];
-    uint8_t byte_134_diag318 : 1;
-    uint8_t : 7;
-    struct CPUEntry entries[0];
+	SCCBHeader h;
+	uint16_t rnmax;
+	uint8_t rnsize;
+	uint8_t  _reserved1[16 - 11];       /* 11-15 */
+	uint16_t entries_cpu;               /* 16-17 */
+	uint16_t offset_cpu;                /* 18-19 */
+	uint8_t  _reserved2[24 - 20];       /* 20-23 */
+	uint8_t  loadparm[8];               /* 24-31 */
+	uint8_t  _reserved3[48 - 32];       /* 32-47 */
+	uint64_t facilities;                /* 48-55 */
+	uint8_t  _reserved0[76 - 56];       /* 56-75 */
+	uint32_t ibc_val;
+	uint8_t  conf_char[99 - 80];        /* 80-98 */
+	uint8_t mha_pow;
+	uint32_t rnsize2;
+	uint64_t rnmax2;
+	uint8_t  _reserved6[116 - 112];     /* 112-115 */
+	uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
+	uint16_t highest_cpu;
+	uint8_t  _reserved5[124 - 122];     /* 122-123 */
+	uint32_t hmfai;
+	uint8_t reserved7[134 - 128];
+	uint8_t byte_134_diag318 : 1;
+	uint8_t : 7;
+	struct CPUEntry entries[0];
 } __attribute__((packed)) ReadInfo;
 
 typedef struct ReadCpuInfo {
-    SCCBHeader h;
-    uint16_t nr_configured;         /* 8-9 */
-    uint16_t offset_configured;     /* 10-11 */
-    uint16_t nr_standby;            /* 12-13 */
-    uint16_t offset_standby;        /* 14-15 */
-    uint8_t reserved0[24-16];       /* 16-23 */
-    struct CPUEntry entries[0];
+	SCCBHeader h;
+	uint16_t nr_configured;         /* 8-9 */
+	uint16_t offset_configured;     /* 10-11 */
+	uint16_t nr_standby;            /* 12-13 */
+	uint16_t offset_standby;        /* 14-15 */
+	uint8_t reserved0[24-16];       /* 16-23 */
+	struct CPUEntry entries[0];
 } __attribute__((packed)) ReadCpuInfo;
 
 typedef struct ReadStorageElementInfo {
-    SCCBHeader h;
-    uint16_t max_id;
-    uint16_t assigned;
-    uint16_t standby;
-    uint8_t _reserved0[16 - 14]; /* 14-15 */
-    uint32_t entries[0];
+	SCCBHeader h;
+	uint16_t max_id;
+	uint16_t assigned;
+	uint16_t standby;
+	uint8_t _reserved0[16 - 14]; /* 14-15 */
+	uint32_t entries[0];
 } __attribute__((packed)) ReadStorageElementInfo;
 
 typedef struct AttachStorageElement {
-    SCCBHeader h;
-    uint8_t _reserved0[10 - 8];  /* 8-9 */
-    uint16_t assigned;
-    uint8_t _reserved1[16 - 12]; /* 12-15 */
-    uint32_t entries[0];
+	SCCBHeader h;
+	uint8_t _reserved0[10 - 8];  /* 8-9 */
+	uint16_t assigned;
+	uint8_t _reserved1[16 - 12]; /* 12-15 */
+	uint32_t entries[0];
 } __attribute__((packed)) AttachStorageElement;
 
 typedef struct AssignStorage {
-    SCCBHeader h;
-    uint16_t rn;
+	SCCBHeader h;
+	uint16_t rn;
 } __attribute__((packed)) AssignStorage;
 
 typedef struct IoaCfgSccb {
-    SCCBHeader header;
-    uint8_t atype;
-    uint8_t reserved1;
-    uint16_t reserved2;
-    uint32_t aid;
+	SCCBHeader header;
+	uint8_t atype;
+	uint8_t reserved1;
+	uint16_t reserved2;
+	uint32_t aid;
 } __attribute__((packed)) IoaCfgSccb;
 
 typedef struct SCCB {
-    SCCBHeader h;
-    char data[SCCB_DATA_LEN];
- } __attribute__((packed)) SCCB;
+	SCCBHeader h;
+	char data[SCCB_DATA_LEN];
+} __attribute__((packed)) SCCB;
 
 /* SCLP event types */
 #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
@@ -195,13 +195,13 @@ typedef struct SCCB {
 #define SCLP_SELECTIVE_READ                     0x01
 
 typedef struct WriteEventMask {
-    SCCBHeader h;
-    uint16_t _reserved;
-    uint16_t mask_length;
-    uint32_t cp_receive_mask;
-    uint32_t cp_send_mask;
-    uint32_t send_mask;
-    uint32_t receive_mask;
+	SCCBHeader h;
+	uint16_t _reserved;
+	uint16_t mask_length;
+	uint32_t cp_receive_mask;
+	uint32_t cp_send_mask;
+	uint32_t send_mask;
+	uint32_t receive_mask;
 } __attribute__((packed)) WriteEventMask;
 
 #define MDBTYP_GO               0x0001
@@ -254,25 +254,25 @@ struct mdb {
 } __attribute__((packed));
 
 typedef struct EventBufferHeader {
-    uint16_t length;
-    uint8_t  type;
-    uint8_t  flags;
-    uint16_t _reserved;
+	uint16_t length;
+	uint8_t  type;
+	uint8_t  flags;
+	uint16_t _reserved;
 } __attribute__((packed)) EventBufferHeader;
 
 typedef struct WriteEventData {
-    SCCBHeader h;
-    EventBufferHeader ebh;
-    union {
-	char data[0];
-	struct mdb mdb;
-    } msg;
+	SCCBHeader h;
+	EventBufferHeader ebh;
+	union {
+		char data[0];
+		struct mdb mdb;
+	} msg;
 } __attribute__((packed)) WriteEventData;
 
 typedef struct ReadEventData {
-    SCCBHeader h;
-    EventBufferHeader ebh;
-    uint32_t mask;
+	SCCBHeader h;
+	EventBufferHeader ebh;
+	uint32_t mask;
 } __attribute__((packed)) ReadEventData;
 
 extern char _sccb[];
-- 
2.25.1

Re: [kvm-unit-tests PATCH v3 8/8] s390x: Fix sclp.h style issues

[Claudio Imbrenda]

On Fri, 11 Dec 2020 05:00:39 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Fix indentation
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>m>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/sclp.h | 172
> +++++++++++++++++++++++------------------------ 1 file changed, 86
> insertions(+), 86 deletions(-)
> 
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 58f8e54..dccbaa8 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -79,10 +79,10 @@
>  #define SCCB_SIZE 4096
>  
>  typedef struct SCCBHeader {
> -    uint16_t length;
> -    uint8_t function_code;
> -    uint8_t control_mask[3];
> -    uint16_t response_code;
> +	uint16_t length;
> +	uint8_t function_code;
> +	uint8_t control_mask[3];
> +	uint16_t response_code;
>  } __attribute__((packed)) SCCBHeader;
>  
>  #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> @@ -90,15 +90,15 @@ typedef struct SCCBHeader {
>  
>  /* CPU information */
>  typedef struct CPUEntry {
> -    uint8_t address;
> -    uint8_t reserved0;
> -    uint8_t : 4;
> -    uint8_t feat_sief2 : 1;
> -    uint8_t : 3;
> -    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
> -    uint8_t reserved2[6];
> -    uint8_t type;
> -    uint8_t reserved1;
> +	uint8_t address;
> +	uint8_t reserved0;
> +	uint8_t : 4;
> +	uint8_t feat_sief2 : 1;
> +	uint8_t : 3;
> +	uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
> +	uint8_t reserved2[6];
> +	uint8_t type;
> +	uint8_t reserved1;
>  } __attribute__((packed)) CPUEntry;
>  
>  extern struct sclp_facilities sclp_facilities;
> @@ -110,77 +110,77 @@ struct sclp_facilities {
>  };
>  
>  typedef struct ReadInfo {
> -    SCCBHeader h;
> -    uint16_t rnmax;
> -    uint8_t rnsize;
> -    uint8_t  _reserved1[16 - 11];       /* 11-15 */
> -    uint16_t entries_cpu;               /* 16-17 */
> -    uint16_t offset_cpu;                /* 18-19 */
> -    uint8_t  _reserved2[24 - 20];       /* 20-23 */
> -    uint8_t  loadparm[8];               /* 24-31 */
> -    uint8_t  _reserved3[48 - 32];       /* 32-47 */
> -    uint64_t facilities;                /* 48-55 */
> -    uint8_t  _reserved0[76 - 56];       /* 56-75 */
> -    uint32_t ibc_val;
> -    uint8_t  conf_char[99 - 80];        /* 80-98 */
> -    uint8_t mha_pow;
> -    uint32_t rnsize2;
> -    uint64_t rnmax2;
> -    uint8_t  _reserved6[116 - 112];     /* 112-115 */
> -    uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
> -    uint16_t highest_cpu;
> -    uint8_t  _reserved5[124 - 122];     /* 122-123 */
> -    uint32_t hmfai;
> -    uint8_t reserved7[134 - 128];
> -    uint8_t byte_134_diag318 : 1;
> -    uint8_t : 7;
> -    struct CPUEntry entries[0];
> +	SCCBHeader h;
> +	uint16_t rnmax;
> +	uint8_t rnsize;
> +	uint8_t  _reserved1[16 - 11];       /* 11-15 */
> +	uint16_t entries_cpu;               /* 16-17 */
> +	uint16_t offset_cpu;                /* 18-19 */
> +	uint8_t  _reserved2[24 - 20];       /* 20-23 */
> +	uint8_t  loadparm[8];               /* 24-31 */
> +	uint8_t  _reserved3[48 - 32];       /* 32-47 */
> +	uint64_t facilities;                /* 48-55 */
> +	uint8_t  _reserved0[76 - 56];       /* 56-75 */
> +	uint32_t ibc_val;
> +	uint8_t  conf_char[99 - 80];        /* 80-98 */
> +	uint8_t mha_pow;
> +	uint32_t rnsize2;
> +	uint64_t rnmax2;
> +	uint8_t  _reserved6[116 - 112];     /* 112-115 */
> +	uint8_t  conf_char_ext[120 - 116];   /* 116-119 */
> +	uint16_t highest_cpu;
> +	uint8_t  _reserved5[124 - 122];     /* 122-123 */
> +	uint32_t hmfai;
> +	uint8_t reserved7[134 - 128];
> +	uint8_t byte_134_diag318 : 1;
> +	uint8_t : 7;
> +	struct CPUEntry entries[0];
>  } __attribute__((packed)) ReadInfo;
>  
>  typedef struct ReadCpuInfo {
> -    SCCBHeader h;
> -    uint16_t nr_configured;         /* 8-9 */
> -    uint16_t offset_configured;     /* 10-11 */
> -    uint16_t nr_standby;            /* 12-13 */
> -    uint16_t offset_standby;        /* 14-15 */
> -    uint8_t reserved0[24-16];       /* 16-23 */
> -    struct CPUEntry entries[0];
> +	SCCBHeader h;
> +	uint16_t nr_configured;         /* 8-9 */
> +	uint16_t offset_configured;     /* 10-11 */
> +	uint16_t nr_standby;            /* 12-13 */
> +	uint16_t offset_standby;        /* 14-15 */
> +	uint8_t reserved0[24-16];       /* 16-23 */
> +	struct CPUEntry entries[0];
>  } __attribute__((packed)) ReadCpuInfo;
>  
>  typedef struct ReadStorageElementInfo {
> -    SCCBHeader h;
> -    uint16_t max_id;
> -    uint16_t assigned;
> -    uint16_t standby;
> -    uint8_t _reserved0[16 - 14]; /* 14-15 */
> -    uint32_t entries[0];
> +	SCCBHeader h;
> +	uint16_t max_id;
> +	uint16_t assigned;
> +	uint16_t standby;
> +	uint8_t _reserved0[16 - 14]; /* 14-15 */
> +	uint32_t entries[0];
>  } __attribute__((packed)) ReadStorageElementInfo;
>  
>  typedef struct AttachStorageElement {
> -    SCCBHeader h;
> -    uint8_t _reserved0[10 - 8];  /* 8-9 */
> -    uint16_t assigned;
> -    uint8_t _reserved1[16 - 12]; /* 12-15 */
> -    uint32_t entries[0];
> +	SCCBHeader h;
> +	uint8_t _reserved0[10 - 8];  /* 8-9 */
> +	uint16_t assigned;
> +	uint8_t _reserved1[16 - 12]; /* 12-15 */
> +	uint32_t entries[0];
>  } __attribute__((packed)) AttachStorageElement;
>  
>  typedef struct AssignStorage {
> -    SCCBHeader h;
> -    uint16_t rn;
> +	SCCBHeader h;
> +	uint16_t rn;
>  } __attribute__((packed)) AssignStorage;
>  
>  typedef struct IoaCfgSccb {
> -    SCCBHeader header;
> -    uint8_t atype;
> -    uint8_t reserved1;
> -    uint16_t reserved2;
> -    uint32_t aid;
> +	SCCBHeader header;
> +	uint8_t atype;
> +	uint8_t reserved1;
> +	uint16_t reserved2;
> +	uint32_t aid;
>  } __attribute__((packed)) IoaCfgSccb;
>  
>  typedef struct SCCB {
> -    SCCBHeader h;
> -    char data[SCCB_DATA_LEN];
> - } __attribute__((packed)) SCCB;
> +	SCCBHeader h;
> +	char data[SCCB_DATA_LEN];
> +} __attribute__((packed)) SCCB;
>  
>  /* SCLP event types */
>  #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
> @@ -195,13 +195,13 @@ typedef struct SCCB {
>  #define SCLP_SELECTIVE_READ                     0x01
>  
>  typedef struct WriteEventMask {
> -    SCCBHeader h;
> -    uint16_t _reserved;
> -    uint16_t mask_length;
> -    uint32_t cp_receive_mask;
> -    uint32_t cp_send_mask;
> -    uint32_t send_mask;
> -    uint32_t receive_mask;
> +	SCCBHeader h;
> +	uint16_t _reserved;
> +	uint16_t mask_length;
> +	uint32_t cp_receive_mask;
> +	uint32_t cp_send_mask;
> +	uint32_t send_mask;
> +	uint32_t receive_mask;
>  } __attribute__((packed)) WriteEventMask;
>  
>  #define MDBTYP_GO               0x0001
> @@ -254,25 +254,25 @@ struct mdb {
>  } __attribute__((packed));
>  
>  typedef struct EventBufferHeader {
> -    uint16_t length;
> -    uint8_t  type;
> -    uint8_t  flags;
> -    uint16_t _reserved;
> +	uint16_t length;
> +	uint8_t  type;
> +	uint8_t  flags;
> +	uint16_t _reserved;
>  } __attribute__((packed)) EventBufferHeader;
>  
>  typedef struct WriteEventData {
> -    SCCBHeader h;
> -    EventBufferHeader ebh;
> -    union {
> -	char data[0];
> -	struct mdb mdb;
> -    } msg;
> +	SCCBHeader h;
> +	EventBufferHeader ebh;
> +	union {
> +		char data[0];
> +		struct mdb mdb;
> +	} msg;
>  } __attribute__((packed)) WriteEventData;
>  
>  typedef struct ReadEventData {
> -    SCCBHeader h;
> -    EventBufferHeader ebh;
> -    uint32_t mask;
> +	SCCBHeader h;
> +	EventBufferHeader ebh;
> +	uint32_t mask;
>  } __attribute__((packed)) ReadEventData;
>  
>  extern char _sccb[];