[kvm-unit-tests PATCH 0/5] s390x: sie and uv cleanups

[kvm-unit-tests PATCH 0/5] s390x: sie and uv cleanups

[Janosch Frank]

The UV and SIE additions brought in some minor problems which I want
to address now.

Janosch Frank (5):
  s390x: sie: Add missing includes
  s390x: sie: Fix sie.h integer types
  lib: s390x: uv: Int type cleanup
  lib: s390x: uv: Add offset comments to uv_query and extend it
  lib: s390x: Print if a pgm happened while in SIE

 lib/s390x/asm/uv.h    | 145 +++++++++++++++++++++---------------------
 lib/s390x/interrupt.c |  17 ++++-
 lib/s390x/sie.h       |  11 ++--
 3 files changed, 95 insertions(+), 78 deletions(-)

-- 
2.30.2

[kvm-unit-tests PATCH 1/5] s390x: sie: Add missing includes

[Janosch Frank]

arch_def.h is needed for struct psw.
stdint.h is needed for the uint*_t types.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sie.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index db30d61..b4bb78c 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -2,6 +2,9 @@
 #ifndef _S390X_SIE_H_
 #define _S390X_SIE_H_
 
+#include <stdint.h>
+#include <asm/arch_def.h>
+
 #define CPUSTAT_STOPPED    0x80000000
 #define CPUSTAT_WAIT       0x10000000
 #define CPUSTAT_ECALL_PEND 0x08000000
-- 
2.30.2

[kvm-unit-tests PATCH 2/5] s390x: sie: Fix sie.h integer types

[Janosch Frank]

Let's only use the uint*_t types.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sie.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index b4bb78c..6ba858a 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -173,9 +173,9 @@ struct kvm_s390_sie_block {
 } __attribute__((packed));
 
 struct vm_save_regs {
-	u64 grs[16];
-	u64 fprs[16];
-	u32 fpc;
+	uint64_t grs[16];
+	uint64_t fprs[16];
+	uint32_t fpc;
 };
 
 /* We might be able to nestle all of this into the stack frame. But
@@ -191,7 +191,7 @@ struct vm {
 	struct kvm_s390_sie_block *sblk;
 	struct vm_save_area save_area;
 	/* Ptr to first guest page */
-	u8 *guest_mem;
+	uint8_t *guest_mem;
 };
 
 extern void sie_entry(void);
-- 
2.30.2

[kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Janosch Frank]

These structs have largely been copied from the kernel so they still
have the old uint short types which we want to avoid in favor of the
uint*_t ones.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h | 142 +++++++++++++++++++++++----------------------
 1 file changed, 72 insertions(+), 70 deletions(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index dc3e02d..96a2a7e 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -12,6 +12,8 @@
 #ifndef _ASMS390X_UV_H_
 #define _ASMS390X_UV_H_
 
+#include <stdint.h>
+
 #define UVC_RC_EXECUTED		0x0001
 #define UVC_RC_INV_CMD		0x0002
 #define UVC_RC_INV_STATE	0x0003
@@ -68,73 +70,73 @@ enum uv_cmds_inst {
 };
 
 struct uv_cb_header {
-	u16 len;
-	u16 cmd;	/* Command Code */
-	u16 rc;		/* Response Code */
-	u16 rrc;	/* Return Reason Code */
+	uint16_t len;
+	uint16_t cmd;	/* Command Code */
+	uint16_t rc;	/* Response Code */
+	uint16_t rrc;	/* Return Reason Code */
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_init {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 stor_origin;
-	u64 stor_len;
-	u64 reserved28[4];
+	uint64_t reserved08[2];
+	uint64_t stor_origin;
+	uint64_t stor_len;
+	uint64_t reserved28[4];
 
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_qui {
 	struct uv_cb_header header;
-	u64 reserved08;
-	u64 inst_calls_list[4];
-	u64 reserved30[2];
-	u64 uv_base_stor_len;
-	u64 reserved48;
-	u64 conf_base_phys_stor_len;
-	u64 conf_base_virt_stor_len;
-	u64 conf_virt_var_stor_len;
-	u64 cpu_stor_len;
-	u32 reserved70[3];
-	u32 max_num_sec_conf;
-	u64 max_guest_stor_addr;
-	u8  reserved88[158 - 136];
-	u16 max_guest_cpus;
-	u8  reserveda0[200 - 160];
+	uint64_t reserved08;
+	uint64_t inst_calls_list[4];
+	uint64_t reserved30[2];
+	uint64_t uv_base_stor_len;
+	uint64_t reserved48;
+	uint64_t conf_base_phys_stor_len;
+	uint64_t conf_base_virt_stor_len;
+	uint64_t conf_virt_var_stor_len;
+	uint64_t cpu_stor_len;
+	uint32_t reserved70[3];
+	uint32_t max_num_sec_conf;
+	uint64_t max_guest_stor_addr;
+	uint8_t  reserved88[158 - 136];
+	uint16_t max_guest_cpus;
+	uint8_t  reserveda0[200 - 160];
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_cgc {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 guest_handle;
-	u64 conf_base_stor_origin;
-	u64 conf_var_stor_origin;
-	u64 reserved30;
-	u64 guest_stor_origin;
-	u64 guest_stor_len;
-	u64 guest_sca;
-	u64 guest_asce;
-	u64 reserved60[5];
+	uint64_t reserved08[2];
+	uint64_t guest_handle;
+	uint64_t conf_base_stor_origin;
+	uint64_t conf_var_stor_origin;
+	uint64_t reserved30;
+	uint64_t guest_stor_origin;
+	uint64_t guest_stor_len;
+	uint64_t guest_sca;
+	uint64_t guest_asce;
+	uint64_t reserved60[5];
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_csc {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 cpu_handle;
-	u64 guest_handle;
-	u64 stor_origin;
-	u8  reserved30[6];
-	u16 num;
-	u64 state_origin;
-	u64 reserved[4];
+	uint64_t reserved08[2];
+	uint64_t cpu_handle;
+	uint64_t guest_handle;
+	uint64_t stor_origin;
+	uint8_t  reserved30[6];
+	uint16_t num;
+	uint64_t state_origin;
+	uint64_t reserved[4];
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_unp {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 guest_handle;
-	u64 gaddr;
-	u64 tweak[2];
-	u64 reserved38[3];
+	uint64_t reserved08[2];
+	uint64_t guest_handle;
+	uint64_t gaddr;
+	uint64_t tweak[2];
+	uint64_t reserved38[3];
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 /*
@@ -144,42 +146,42 @@ struct uv_cb_unp {
  */
 struct uv_cb_nodata {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 handle;
-	u64 reserved20[4];
+	uint64_t reserved08[2];
+	uint64_t handle;
+	uint64_t reserved20[4];
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_share {
 	struct uv_cb_header header;
-	u64 reserved08[3];
-	u64 paddr;
-	u64 reserved28;
+	uint64_t reserved08[3];
+	uint64_t paddr;
+	uint64_t reserved28;
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 /* Convert to Secure */
 struct uv_cb_cts {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 guest_handle;
-	u64 gaddr;
+	uint64_t reserved08[2];
+	uint64_t guest_handle;
+	uint64_t gaddr;
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 /* Convert from Secure / Pin Page Shared */
 struct uv_cb_cfs {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 paddr;
+	uint64_t reserved08[2];
+	uint64_t paddr;
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 /* Set Secure Config Parameter */
 struct uv_cb_ssc {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 guest_handle;
-	u64 sec_header_origin;
-	u32 sec_header_len;
-	u32 reserved2c;
-	u64 reserved30[4];
+	uint64_t reserved08[2];
+	uint64_t guest_handle;
+	uint64_t sec_header_origin;
+	uint32_t sec_header_len;
+	uint32_t reserved2c;
+	uint64_t reserved30[4];
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 static inline int uv_call_once(unsigned long r1, unsigned long r2)
@@ -211,7 +213,7 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
 	return cc;
 }
 
-static inline int share(unsigned long addr, u16 cmd)
+static inline int share(unsigned long addr, uint16_t cmd)
 {
 	struct uv_cb_share uvcb = {
 		.header.cmd = cmd,
@@ -220,7 +222,7 @@ static inline int share(unsigned long addr, u16 cmd)
 	};
 	int cc;
 
-	cc = uv_call(0, (u64)&uvcb);
+	cc = uv_call(0, (uint64_t)&uvcb);
 	if (!cc && uvcb.header.rc == UVC_RC_EXECUTED)
 		return 0;
 
@@ -252,11 +254,11 @@ static inline int uv_remove_shared(unsigned long addr)
 
 struct uv_cb_cpu_set_state {
 	struct uv_cb_header header;
-	u64 reserved08[2];
-	u64 cpu_handle;
-	u8  reserved20[7];
-	u8  state;
-	u64 reserved28[5];
+	uint64_t reserved08[2];
+	uint64_t cpu_handle;
+	uint8_t  reserved20[7];
+	uint8_t  state;
+	uint64_t reserved28[5];
 };
 
 #define PV_CPU_STATE_OPR	1
-- 
2.30.2

[kvm-unit-tests PATCH 4/5] lib: s390x: uv: Add offset comments to uv_query and extend it

[Janosch Frank]

The struct is getting longer, let's add offset comments so we know
where we change things when we add struct members.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/uv.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
index 96a2a7e..5ff98b8 100644
--- a/lib/s390x/asm/uv.h
+++ b/lib/s390x/asm/uv.h
@@ -86,22 +86,23 @@ struct uv_cb_init {
 } __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_qui {
-	struct uv_cb_header header;
-	uint64_t reserved08;
-	uint64_t inst_calls_list[4];
-	uint64_t reserved30[2];
-	uint64_t uv_base_stor_len;
-	uint64_t reserved48;
-	uint64_t conf_base_phys_stor_len;
-	uint64_t conf_base_virt_stor_len;
-	uint64_t conf_virt_var_stor_len;
-	uint64_t cpu_stor_len;
-	uint32_t reserved70[3];
-	uint32_t max_num_sec_conf;
-	uint64_t max_guest_stor_addr;
-	uint8_t  reserved88[158 - 136];
-	uint16_t max_guest_cpus;
-	uint8_t  reserveda0[200 - 160];
+	struct uv_cb_header header;		/* 0x0000 */
+	uint64_t reserved08;			/* 0x0008 */
+	uint64_t inst_calls_list[4];		/* 0x0010 */
+	uint64_t reserved30[2];			/* 0x0030 */
+	uint64_t uv_base_stor_len;		/* 0x0040 */
+	uint64_t reserved48;			/* 0x0048 */
+	uint64_t conf_base_phys_stor_len;	/* 0x0050 */
+	uint64_t conf_base_virt_stor_len;	/* 0x0058 */
+	uint64_t conf_virt_var_stor_len;	/* 0x0060 */
+	uint64_t cpu_stor_len;			/* 0x0068 */
+	uint32_t reserved70[3];			/* 0x0070 */
+	uint32_t max_num_sec_conf;		/* 0x007c */
+	uint64_t max_guest_stor_addr;		/* 0x0080 */
+	uint8_t  reserved88[158 - 136];		/* 0x0088 */
+	uint16_t max_guest_cpus;		/* 0x009e */
+	uint64_t uv_feature_indications;	/* 0x00a0 */
+	uint8_t  reserveda0[200 - 168];		/* 0x00a8 */
 }  __attribute__((packed))  __attribute__((aligned(8)));
 
 struct uv_cb_cgc {
-- 
2.30.2

[kvm-unit-tests PATCH 5/5] lib: s390x: Print if a pgm happened while in SIE

[Janosch Frank]

For debugging it helps if you know if the PGM happened while being in
SIE or not.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/interrupt.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index b627942..76015b1 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -141,10 +141,21 @@ static void print_int_regs(struct stack_frame_int *stack)
 static void print_pgm_info(struct stack_frame_int *stack)
 
 {
+	bool in_sie;
+
+	in_sie = (lc->pgm_old_psw.addr >= (uintptr_t)sie_entry &&
+		  lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
+
 	printf("\n");
-	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
-	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
-	       lc->pgm_int_id);
+	if (!in_sie)
+		printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
+		       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
+		       lc->pgm_int_id);
+	else
+		printf("Unexpected program interrupt in SIE: %d on cpu %d at %#lx, ilen %d\n",
+		       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
+		       lc->pgm_int_id);
+
 	print_int_regs(stack);
 	dump_stack();
 	report_summary();
-- 
2.30.2

Re: [kvm-unit-tests PATCH 1/5] s390x: sie: Add missing includes

[Cornelia Huck]

On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:

> arch_def.h is needed for struct psw.
> stdint.h is needed for the uint*_t types.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/sie.h | 3 +++
>  1 file changed, 3 insertions(+)

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

Re: [kvm-unit-tests PATCH 2/5] s390x: sie: Fix sie.h integer types

[Cornelia Huck]

On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's only use the uint*_t types.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/sie.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

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

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Cornelia Huck]

On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:

> These structs have largely been copied from the kernel so they still
> have the old uint short types which we want to avoid in favor of the
> uint*_t ones.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h | 142 +++++++++++++++++++++++----------------------
>  1 file changed, 72 insertions(+), 70 deletions(-)

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

Re: [kvm-unit-tests PATCH 4/5] lib: s390x: uv: Add offset comments to uv_query and extend it

[Cornelia Huck]

On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:

> The struct is getting longer, let's add offset comments so we know
> where we change things when we add struct members.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm/uv.h | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 96a2a7e..5ff98b8 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -86,22 +86,23 @@ struct uv_cb_init {
>  } __attribute__((packed))  __attribute__((aligned(8)));
>  
>  struct uv_cb_qui {
> -	struct uv_cb_header header;
> -	uint64_t reserved08;
> -	uint64_t inst_calls_list[4];
> -	uint64_t reserved30[2];
> -	uint64_t uv_base_stor_len;
> -	uint64_t reserved48;
> -	uint64_t conf_base_phys_stor_len;
> -	uint64_t conf_base_virt_stor_len;
> -	uint64_t conf_virt_var_stor_len;
> -	uint64_t cpu_stor_len;
> -	uint32_t reserved70[3];
> -	uint32_t max_num_sec_conf;
> -	uint64_t max_guest_stor_addr;
> -	uint8_t  reserved88[158 - 136];
> -	uint16_t max_guest_cpus;
> -	uint8_t  reserveda0[200 - 160];
> +	struct uv_cb_header header;		/* 0x0000 */
> +	uint64_t reserved08;			/* 0x0008 */
> +	uint64_t inst_calls_list[4];		/* 0x0010 */
> +	uint64_t reserved30[2];			/* 0x0030 */
> +	uint64_t uv_base_stor_len;		/* 0x0040 */
> +	uint64_t reserved48;			/* 0x0048 */
> +	uint64_t conf_base_phys_stor_len;	/* 0x0050 */
> +	uint64_t conf_base_virt_stor_len;	/* 0x0058 */
> +	uint64_t conf_virt_var_stor_len;	/* 0x0060 */
> +	uint64_t cpu_stor_len;			/* 0x0068 */
> +	uint32_t reserved70[3];			/* 0x0070 */
> +	uint32_t max_num_sec_conf;		/* 0x007c */
> +	uint64_t max_guest_stor_addr;		/* 0x0080 */
> +	uint8_t  reserved88[158 - 136];		/* 0x0088 */
> +	uint16_t max_guest_cpus;		/* 0x009e */
> +	uint64_t uv_feature_indications;	/* 0x00a0 */
> +	uint8_t  reserveda0[200 - 168];		/* 0x00a8 */

Should this rather be reserveda8? The other reserveds encode their
position properly.

Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Print if a pgm happened while in SIE

[Cornelia Huck]

On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:

> For debugging it helps if you know if the PGM happened while being in
> SIE or not.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index b627942..76015b1 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -141,10 +141,21 @@ static void print_int_regs(struct stack_frame_int *stack)
>  static void print_pgm_info(struct stack_frame_int *stack)
>  
>  {
> +	bool in_sie;
> +
> +	in_sie = (lc->pgm_old_psw.addr >= (uintptr_t)sie_entry &&
> +		  lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
> +
>  	printf("\n");
> -	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
> -	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
> -	       lc->pgm_int_id);
> +	if (!in_sie)
> +		printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
> +		       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
> +		       lc->pgm_int_id);
> +	else
> +		printf("Unexpected program interrupt in SIE: %d on cpu %d at %#lx, ilen %d\n",
> +		       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
> +		       lc->pgm_int_id);

Hm...

		printf("Unexpected program interrupt%s: %d on cpu %d at %#lx, ilen %d\n",
		       in_sie ? " in SIE" : "",
		       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
		       lc->pgm_int_id);

Matter of taste, I guess.

Re: [kvm-unit-tests PATCH 4/5] lib: s390x: uv: Add offset comments to uv_query and extend it

[Janosch Frank]

On 6/30/21 11:06 AM, Cornelia Huck wrote:
> On Tue, Jun 29 2021, Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The struct is getting longer, let's add offset comments so we know
>> where we change things when we add struct members.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  lib/s390x/asm/uv.h | 33 +++++++++++++++++----------------
>>  1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index 96a2a7e..5ff98b8 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -86,22 +86,23 @@ struct uv_cb_init {
>>  } __attribute__((packed))  __attribute__((aligned(8)));
>>  
>>  struct uv_cb_qui {
>> -	struct uv_cb_header header;
>> -	uint64_t reserved08;
>> -	uint64_t inst_calls_list[4];
>> -	uint64_t reserved30[2];
>> -	uint64_t uv_base_stor_len;
>> -	uint64_t reserved48;
>> -	uint64_t conf_base_phys_stor_len;
>> -	uint64_t conf_base_virt_stor_len;
>> -	uint64_t conf_virt_var_stor_len;
>> -	uint64_t cpu_stor_len;
>> -	uint32_t reserved70[3];
>> -	uint32_t max_num_sec_conf;
>> -	uint64_t max_guest_stor_addr;
>> -	uint8_t  reserved88[158 - 136];
>> -	uint16_t max_guest_cpus;
>> -	uint8_t  reserveda0[200 - 160];
>> +	struct uv_cb_header header;		/* 0x0000 */
>> +	uint64_t reserved08;			/* 0x0008 */
>> +	uint64_t inst_calls_list[4];		/* 0x0010 */
>> +	uint64_t reserved30[2];			/* 0x0030 */
>> +	uint64_t uv_base_stor_len;		/* 0x0040 */
>> +	uint64_t reserved48;			/* 0x0048 */
>> +	uint64_t conf_base_phys_stor_len;	/* 0x0050 */
>> +	uint64_t conf_base_virt_stor_len;	/* 0x0058 */
>> +	uint64_t conf_virt_var_stor_len;	/* 0x0060 */
>> +	uint64_t cpu_stor_len;			/* 0x0068 */
>> +	uint32_t reserved70[3];			/* 0x0070 */
>> +	uint32_t max_num_sec_conf;		/* 0x007c */
>> +	uint64_t max_guest_stor_addr;		/* 0x0080 */
>> +	uint8_t  reserved88[158 - 136];		/* 0x0088 */
>> +	uint16_t max_guest_cpus;		/* 0x009e */
>> +	uint64_t uv_feature_indications;	/* 0x00a0 */
>> +	uint8_t  reserveda0[200 - 168];		/* 0x00a8 */
> 
> Should this rather be reserveda8? The other reserveds encode their
> position properly.
> 


Oops

Re: [kvm-unit-tests PATCH 2/5] s390x: sie: Fix sie.h integer types

[Thomas Huth]

On 29/06/2021 15.33, Janosch Frank wrote:
> Let's only use the uint*_t types.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/sie.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index b4bb78c..6ba858a 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -173,9 +173,9 @@ struct kvm_s390_sie_block {
>   } __attribute__((packed));
>   
>   struct vm_save_regs {
> -	u64 grs[16];
> -	u64 fprs[16];
> -	u32 fpc;
> +	uint64_t grs[16];
> +	uint64_t fprs[16];
> +	uint32_t fpc;
>   };
>   
>   /* We might be able to nestle all of this into the stack frame. But
> @@ -191,7 +191,7 @@ struct vm {
>   	struct kvm_s390_sie_block *sblk;
>   	struct vm_save_area save_area;
>   	/* Ptr to first guest page */
> -	u8 *guest_mem;
> +	uint8_t *guest_mem;
>   };

Yes, that's more consistent in this file.

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

Re: [kvm-unit-tests PATCH 1/5] s390x: sie: Add missing includes

[Thomas Huth]

On 29/06/2021 15.33, Janosch Frank wrote:
> arch_def.h is needed for struct psw.
> stdint.h is needed for the uint*_t types.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/sie.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index db30d61..b4bb78c 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -2,6 +2,9 @@
>   #ifndef _S390X_SIE_H_
>   #define _S390X_SIE_H_
>   
> +#include <stdint.h>
> +#include <asm/arch_def.h>
> +
>   #define CPUSTAT_STOPPED    0x80000000
>   #define CPUSTAT_WAIT       0x10000000
>   #define CPUSTAT_ECALL_PEND 0x08000000
> 

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

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Thomas Huth]

On 29/06/2021 15.33, Janosch Frank wrote:
> These structs have largely been copied from the kernel so they still
> have the old uint short types which we want to avoid in favor of the
> uint*_t ones.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/asm/uv.h | 142 +++++++++++++++++++++++----------------------
>   1 file changed, 72 insertions(+), 70 deletions(-)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index dc3e02d..96a2a7e 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -12,6 +12,8 @@
>   #ifndef _ASMS390X_UV_H_
>   #define _ASMS390X_UV_H_
>   
> +#include <stdint.h>
> +
>   #define UVC_RC_EXECUTED		0x0001
>   #define UVC_RC_INV_CMD		0x0002
>   #define UVC_RC_INV_STATE	0x0003
> @@ -68,73 +70,73 @@ enum uv_cmds_inst {
>   };
>   
>   struct uv_cb_header {
> -	u16 len;
> -	u16 cmd;	/* Command Code */
> -	u16 rc;		/* Response Code */
> -	u16 rrc;	/* Return Reason Code */
> +	uint16_t len;
> +	uint16_t cmd;	/* Command Code */
> +	uint16_t rc;	/* Response Code */
> +	uint16_t rrc;	/* Return Reason Code */
>   } __attribute__((packed))  __attribute__((aligned(8)));

Hmm, for files that are more or less a copy from the corresponding kernel 
header, I'm not sure whether it makes sense to convert them to the stdint.h 
types? It might be better to keep the kernel types so that updates to this 
header can be ported more easily to the kvm-unit-tests later?

  Thomas

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Janosch Frank]

On 7/4/21 9:51 AM, Thomas Huth wrote:
> On 29/06/2021 15.33, Janosch Frank wrote:
>> These structs have largely been copied from the kernel so they still
>> have the old uint short types which we want to avoid in favor of the
>> uint*_t ones.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/asm/uv.h | 142 +++++++++++++++++++++++----------------------
>>   1 file changed, 72 insertions(+), 70 deletions(-)
>>
>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>> index dc3e02d..96a2a7e 100644
>> --- a/lib/s390x/asm/uv.h
>> +++ b/lib/s390x/asm/uv.h
>> @@ -12,6 +12,8 @@
>>   #ifndef _ASMS390X_UV_H_
>>   #define _ASMS390X_UV_H_
>>   
>> +#include <stdint.h>
>> +
>>   #define UVC_RC_EXECUTED		0x0001
>>   #define UVC_RC_INV_CMD		0x0002
>>   #define UVC_RC_INV_STATE	0x0003
>> @@ -68,73 +70,73 @@ enum uv_cmds_inst {
>>   };
>>   
>>   struct uv_cb_header {
>> -	u16 len;
>> -	u16 cmd;	/* Command Code */
>> -	u16 rc;		/* Response Code */
>> -	u16 rrc;	/* Return Reason Code */
>> +	uint16_t len;
>> +	uint16_t cmd;	/* Command Code */
>> +	uint16_t rc;	/* Response Code */
>> +	uint16_t rrc;	/* Return Reason Code */
>>   } __attribute__((packed))  __attribute__((aligned(8)));
> 
> Hmm, for files that are more or less a copy from the corresponding kernel 
> header, I'm not sure whether it makes sense to convert them to the stdint.h 
> types? It might be better to keep the kernel types so that updates to this 
> header can be ported more easily to the kvm-unit-tests later?

sie.h contents are 90% sblk which came directly from KVM...
Do you really want to have exceptions for one file? Because if that's
the case then I see no sense in changing other things over since I
prefer using short types.


> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Thomas Huth]

On 05/07/2021 11.33, Janosch Frank wrote:
> On 7/4/21 9:51 AM, Thomas Huth wrote:
>> On 29/06/2021 15.33, Janosch Frank wrote:
>>> These structs have largely been copied from the kernel so they still
>>> have the old uint short types which we want to avoid in favor of the
>>> uint*_t ones.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    lib/s390x/asm/uv.h | 142 +++++++++++++++++++++++----------------------
>>>    1 file changed, 72 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
>>> index dc3e02d..96a2a7e 100644
>>> --- a/lib/s390x/asm/uv.h
>>> +++ b/lib/s390x/asm/uv.h
>>> @@ -12,6 +12,8 @@
>>>    #ifndef _ASMS390X_UV_H_
>>>    #define _ASMS390X_UV_H_
>>>    
>>> +#include <stdint.h>
>>> +
>>>    #define UVC_RC_EXECUTED		0x0001
>>>    #define UVC_RC_INV_CMD		0x0002
>>>    #define UVC_RC_INV_STATE	0x0003
>>> @@ -68,73 +70,73 @@ enum uv_cmds_inst {
>>>    };
>>>    
>>>    struct uv_cb_header {
>>> -	u16 len;
>>> -	u16 cmd;	/* Command Code */
>>> -	u16 rc;		/* Response Code */
>>> -	u16 rrc;	/* Return Reason Code */
>>> +	uint16_t len;
>>> +	uint16_t cmd;	/* Command Code */
>>> +	uint16_t rc;	/* Response Code */
>>> +	uint16_t rrc;	/* Return Reason Code */
>>>    } __attribute__((packed))  __attribute__((aligned(8)));
>>
>> Hmm, for files that are more or less a copy from the corresponding kernel
>> header, I'm not sure whether it makes sense to convert them to the stdint.h
>> types? It might be better to keep the kernel types so that updates to this
>> header can be ported more easily to the kvm-unit-tests later?
> 
> sie.h contents are 90% sblk which came directly from KVM...
> Do you really want to have exceptions for one file? Because if that's
> the case then I see no sense in changing other things over since I
> prefer using short types.

Completely inaccurate checks with the lib directory of the kvm-unit-tests:

$ grep -r u64 lib/ | wc -l
234
$ grep -r uint64 lib/ | wc -l
245

$ grep -r u8 lib/ | wc -l
137
$ grep -r uint8 lib/ | wc -l
193

... I guess that's an indication that we do not really have a prevailing 
style here?
I personally prefer the stdint.h types, I'm just not sure whether it makes 
sense to keep some headers close to the kernel or not...?

  Thomas

Re: [kvm-unit-tests PATCH 1/5] s390x: sie: Add missing includes

[Claudio Imbrenda]

On Tue, 29 Jun 2021 13:33:18 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> arch_def.h is needed for struct psw.
> stdint.h is needed for the uint*_t types.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/sie.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index db30d61..b4bb78c 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -2,6 +2,9 @@
>  #ifndef _S390X_SIE_H_
>  #define _S390X_SIE_H_
>  
> +#include <stdint.h>
> +#include <asm/arch_def.h>
> +
>  #define CPUSTAT_STOPPED    0x80000000
>  #define CPUSTAT_WAIT       0x10000000
>  #define CPUSTAT_ECALL_PEND 0x08000000

Re: [kvm-unit-tests PATCH 2/5] s390x: sie: Fix sie.h integer types

[Claudio Imbrenda]

On Tue, 29 Jun 2021 13:33:19 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's only use the uint*_t types.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  lib/s390x/sie.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index b4bb78c..6ba858a 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -173,9 +173,9 @@ struct kvm_s390_sie_block {
>  } __attribute__((packed));
>  
>  struct vm_save_regs {
> -	u64 grs[16];
> -	u64 fprs[16];
> -	u32 fpc;
> +	uint64_t grs[16];
> +	uint64_t fprs[16];
> +	uint32_t fpc;
>  };
>  
>  /* We might be able to nestle all of this into the stack frame. But
> @@ -191,7 +191,7 @@ struct vm {
>  	struct kvm_s390_sie_block *sblk;
>  	struct vm_save_area save_area;
>  	/* Ptr to first guest page */
> -	u8 *guest_mem;
> +	uint8_t *guest_mem;
>  };
>  
>  extern void sie_entry(void);

Re: [kvm-unit-tests PATCH 3/5] lib: s390x: uv: Int type cleanup

[Claudio Imbrenda]

On Mon, 5 Jul 2021 11:41:57 +0200
Thomas Huth <thuth@redhat.com> wrote:

[snip]

> 
> Completely inaccurate checks with the lib directory of the
> kvm-unit-tests:
> 
> $ grep -r u64 lib/ | wc -l
> 234
> $ grep -r uint64 lib/ | wc -l
> 245
> 
> $ grep -r u8 lib/ | wc -l
> 137
> $ grep -r uint8 lib/ | wc -l
> 193
> 
> ... I guess that's an indication that we do not really have a
> prevailing style here?
> I personally prefer the stdint.h types, I'm just not sure whether it
> makes sense to keep some headers close to the kernel or not...?
> 
>   Thomas
> 

I agree, the project as a whole needs to decide the policy regarding
stdint.h types. Do we want them always? only for stuff that doesn't
need synchronization with the kernel? or maybe we just don't care?

I don't care which way we go, but I think we need to decide on one way
to go.

Re: [kvm-unit-tests PATCH 4/5] lib: s390x: uv: Add offset comments to uv_query and extend it

[Claudio Imbrenda]

On Tue, 29 Jun 2021 13:33:21 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> The struct is getting longer, let's add offset comments so we know
> where we change things when we add struct members.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

with reserveda0 fixed:

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

> ---
>  lib/s390x/asm/uv.h | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/s390x/asm/uv.h b/lib/s390x/asm/uv.h
> index 96a2a7e..5ff98b8 100644
> --- a/lib/s390x/asm/uv.h
> +++ b/lib/s390x/asm/uv.h
> @@ -86,22 +86,23 @@ struct uv_cb_init {
>  } __attribute__((packed))  __attribute__((aligned(8)));
>  
>  struct uv_cb_qui {
> -	struct uv_cb_header header;
> -	uint64_t reserved08;
> -	uint64_t inst_calls_list[4];
> -	uint64_t reserved30[2];
> -	uint64_t uv_base_stor_len;
> -	uint64_t reserved48;
> -	uint64_t conf_base_phys_stor_len;
> -	uint64_t conf_base_virt_stor_len;
> -	uint64_t conf_virt_var_stor_len;
> -	uint64_t cpu_stor_len;
> -	uint32_t reserved70[3];
> -	uint32_t max_num_sec_conf;
> -	uint64_t max_guest_stor_addr;
> -	uint8_t  reserved88[158 - 136];
> -	uint16_t max_guest_cpus;
> -	uint8_t  reserveda0[200 - 160];
> +	struct uv_cb_header header;		/* 0x0000 */
> +	uint64_t reserved08;			/* 0x0008 */
> +	uint64_t inst_calls_list[4];		/* 0x0010 */
> +	uint64_t reserved30[2];			/* 0x0030 */
> +	uint64_t uv_base_stor_len;		/* 0x0040 */
> +	uint64_t reserved48;			/* 0x0048 */
> +	uint64_t conf_base_phys_stor_len;	/* 0x0050 */
> +	uint64_t conf_base_virt_stor_len;	/* 0x0058 */
> +	uint64_t conf_virt_var_stor_len;	/* 0x0060 */
> +	uint64_t cpu_stor_len;			/* 0x0068 */
> +	uint32_t reserved70[3];			/* 0x0070 */
> +	uint32_t max_num_sec_conf;		/* 0x007c */
> +	uint64_t max_guest_stor_addr;		/* 0x0080 */
> +	uint8_t  reserved88[158 - 136];		/* 0x0088 */
> +	uint16_t max_guest_cpus;		/* 0x009e */
> +	uint64_t uv_feature_indications;	/* 0x00a0 */
> +	uint8_t  reserveda0[200 - 168];		/* 0x00a8 */
>  }  __attribute__((packed))  __attribute__((aligned(8)));
>  
>  struct uv_cb_cgc {

Re: [kvm-unit-tests PATCH 5/5] lib: s390x: Print if a pgm happened while in SIE

[Claudio Imbrenda]

On Tue, 29 Jun 2021 13:33:22 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> For debugging it helps if you know if the PGM happened while being in
> SIE or not.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

I agree with Conny regarding the style, unless you want to extend the
SIE printf to provide more information (maybe about the guest?)

> ---
>  lib/s390x/interrupt.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index b627942..76015b1 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -141,10 +141,21 @@ static void print_int_regs(struct
> stack_frame_int *stack) static void print_pgm_info(struct
> stack_frame_int *stack) 
>  {
> +	bool in_sie;
> +
> +	in_sie = (lc->pgm_old_psw.addr >= (uintptr_t)sie_entry &&
> +		  lc->pgm_old_psw.addr <= (uintptr_t)sie_exit);
> +
>  	printf("\n");
> -	printf("Unexpected program interrupt: %d on cpu %d at %#lx,
> ilen %d\n",
> -	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
> -	       lc->pgm_int_id);
> +	if (!in_sie)
> +		printf("Unexpected program interrupt: %d on cpu %d
> at %#lx, ilen %d\n",
> +		       lc->pgm_int_code, stap(),
> lc->pgm_old_psw.addr,
> +		       lc->pgm_int_id);
> +	else
> +		printf("Unexpected program interrupt in SIE: %d on
> cpu %d at %#lx, ilen %d\n",
> +		       lc->pgm_int_code, stap(),
> lc->pgm_old_psw.addr,
> +		       lc->pgm_int_id);
> +
>  	print_int_regs(stack);
>  	dump_stack();
>  	report_summary();