[PATCH v1 0/2] s390: Two small fixes and improvements

[PATCH v1 0/2] s390: Two small fixes and improvements

[Claudio Imbrenda]

The main goal of this small series is to do some clean-up and remove some
paper cuts (or at least clear the way for papercuts to be removed in the
future).

Heiko: this can go through the s390 tree, as agreed.

Claudio Imbrenda (2):
  s390/entry: Pass the asce as parameter to sie64a()
  s390/kvm: Move bitfields for dat tables

 arch/s390/include/asm/dat-bits.h   | 170 +++++++++++++++++++++++++++++
 arch/s390/include/asm/kvm_host.h   |   7 +-
 arch/s390/include/asm/stacktrace.h |   1 +
 arch/s390/kernel/asm-offsets.c     |   1 +
 arch/s390/kernel/entry.S           |   8 +-
 arch/s390/kvm/gaccess.c            | 163 +--------------------------
 arch/s390/kvm/kvm-s390.c           |   3 +-
 arch/s390/kvm/vsie.c               |   2 +-
 8 files changed, 185 insertions(+), 170 deletions(-)
 create mode 100644 arch/s390/include/asm/dat-bits.h

-- 
2.45.2

[PATCH v1 1/2] s390/entry: Pass the asce as parameter to sie64a()

[Claudio Imbrenda]

Pass the guest ASCE explicitly as parameter, instead of having sie64a()
take it from lowcore.

This removes hidden state from lowcore, and makes things look cleaner.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h   | 7 ++++---
 arch/s390/include/asm/stacktrace.h | 1 +
 arch/s390/kernel/asm-offsets.c     | 1 +
 arch/s390/kernel/entry.S           | 8 +++-----
 arch/s390/kvm/kvm-s390.c           | 3 ++-
 arch/s390/kvm/vsie.c               | 2 +-
 6 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 95990461888f..2d4e3f50a823 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -1029,11 +1029,12 @@ void kvm_arch_crypto_clear_masks(struct kvm *kvm);
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm);
 
-int __sie64a(phys_addr_t sie_block_phys, struct kvm_s390_sie_block *sie_block, u64 *rsa);
+int __sie64a(phys_addr_t sie_block_phys, struct kvm_s390_sie_block *sie_block, u64 *rsa,
+	     unsigned long gasce);
 
-static inline int sie64a(struct kvm_s390_sie_block *sie_block, u64 *rsa)
+static inline int sie64a(struct kvm_s390_sie_block *sie_block, u64 *rsa, unsigned long gasce)
 {
-	return __sie64a(virt_to_phys(sie_block), sie_block, rsa);
+	return __sie64a(virt_to_phys(sie_block), sie_block, rsa, gasce);
 }
 
 extern char sie_exit;
diff --git a/arch/s390/include/asm/stacktrace.h b/arch/s390/include/asm/stacktrace.h
index 85b6738b826a..1d5ca13dc90f 100644
--- a/arch/s390/include/asm/stacktrace.h
+++ b/arch/s390/include/asm/stacktrace.h
@@ -65,6 +65,7 @@ struct stack_frame {
 			unsigned long sie_reason;
 			unsigned long sie_flags;
 			unsigned long sie_control_block_phys;
+			unsigned long sie_guest_asce;
 		};
 	};
 	unsigned long gprs[10];
diff --git a/arch/s390/kernel/asm-offsets.c b/arch/s390/kernel/asm-offsets.c
index f55979f64d49..26bb45d0e6f1 100644
--- a/arch/s390/kernel/asm-offsets.c
+++ b/arch/s390/kernel/asm-offsets.c
@@ -63,6 +63,7 @@ int main(void)
 	OFFSET(__SF_SIE_REASON, stack_frame, sie_reason);
 	OFFSET(__SF_SIE_FLAGS, stack_frame, sie_flags);
 	OFFSET(__SF_SIE_CONTROL_PHYS, stack_frame, sie_control_block_phys);
+	OFFSET(__SF_SIE_GUEST_ASCE, stack_frame, sie_guest_asce);
 	DEFINE(STACK_FRAME_OVERHEAD, sizeof(struct stack_frame));
 	BLANK();
 	OFFSET(__SFUSER_BACKCHAIN, stack_frame_user, back_chain);
diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index 60cf917a7122..454b6b92c7f8 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -179,6 +179,7 @@ SYM_FUNC_END(__switch_to_asm)
  * %r2 pointer to sie control block phys
  * %r3 pointer to sie control block virt
  * %r4 guest register save area
+ * %r5 guest asce
  */
 SYM_FUNC_START(__sie64a)
 	stmg	%r6,%r14,__SF_GPRS(%r15)	# save kernel registers
@@ -186,15 +187,12 @@ SYM_FUNC_START(__sie64a)
 	stg	%r2,__SF_SIE_CONTROL_PHYS(%r15)	# save sie block physical..
 	stg	%r3,__SF_SIE_CONTROL(%r15)	# ...and virtual addresses
 	stg	%r4,__SF_SIE_SAVEAREA(%r15)	# save guest register save area
+	stg	%r5,__SF_SIE_GUEST_ASCE(%r15)	# save guest asce
 	xc	__SF_SIE_REASON(8,%r15),__SF_SIE_REASON(%r15) # reason code = 0
 	mvc	__SF_SIE_FLAGS(8,%r15),__TI_flags(%r12) # copy thread flags
 	lmg	%r0,%r13,0(%r4)			# load guest gprs 0-13
-	lg	%r14,__LC_GMAP			# get gmap pointer
-	ltgr	%r14,%r14
-	jz	.Lsie_gmap
 	oi	__LC_CPU_FLAGS+7,_CIF_SIE
-	lctlg	%c1,%c1,__GMAP_ASCE(%r14)	# load primary asce
-.Lsie_gmap:
+	lctlg	%c1,%c1,__SF_SIE_GUEST_ASCE(%r15) # load primary asce
 	lg	%r14,__SF_SIE_CONTROL(%r15)	# get control block pointer
 	oi	__SIE_PROG0C+3(%r14),1		# we are going into SIE now
 	tm	__SIE_PROG20+3(%r14),3		# last exit...
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 82e9631cd9ef..148dff562386 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4829,7 +4829,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			       sizeof(sie_page->pv_grregs));
 		}
 		exit_reason = sie64a(vcpu->arch.sie_block,
-				     vcpu->run->s.regs.gprs);
+				     vcpu->run->s.regs.gprs,
+				     gmap_get_enabled()->asce);
 		if (kvm_s390_pv_cpu_is_protected(vcpu)) {
 			memcpy(vcpu->run->s.regs.gprs,
 			       sie_page->pv_grregs,
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index c9ecae830634..97a70c2b83ee 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -1150,7 +1150,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	vcpu->arch.sie_block->prog0c |= PROG_IN_SIE;
 	barrier();
 	if (!kvm_s390_vcpu_sie_inhibited(vcpu))
-		rc = sie64a(scb_s, vcpu->run->s.regs.gprs);
+		rc = sie64a(scb_s, vcpu->run->s.regs.gprs, gmap_get_enabled()->asce);
 	barrier();
 	vcpu->arch.sie_block->prog0c &= ~PROG_IN_SIE;
 
-- 
2.45.2

[PATCH v1 2/2] s390/kvm: Move bitfields for dat tables

[Claudio Imbrenda]

Move and improve the struct definitions for DAT tables from gaccess.c
to a new header.

Once in a separate header, the structs become available everywhere. One
possible usecase is to merge them in the s390 pte_t and p?d_t
definitions, which is left as an exercise for the reader.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/dat-bits.h | 170 +++++++++++++++++++++++++++++++
 arch/s390/kvm/gaccess.c          | 163 +----------------------------
 2 files changed, 173 insertions(+), 160 deletions(-)
 create mode 100644 arch/s390/include/asm/dat-bits.h

diff --git a/arch/s390/include/asm/dat-bits.h b/arch/s390/include/asm/dat-bits.h
new file mode 100644
index 000000000000..d8afd13be48c
--- /dev/null
+++ b/arch/s390/include/asm/dat-bits.h
@@ -0,0 +1,170 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * DAT table and related structures
+ *
+ * Copyright IBM Corp. 2024
+ *
+ */
+
+#ifndef _S390_DAT_BITS_H
+#define _S390_DAT_BITS_H
+
+union asce {
+	unsigned long val;
+	struct {
+		 unsigned long rsto: 52;/* Region- or Segment-Table Origin */
+		 unsigned long     : 2;
+		 unsigned long g   : 1; /* Subspace Group control */
+		 unsigned long p   : 1; /* Private Space control */
+		 unsigned long s   : 1; /* Storage-Alteration-Event control */
+		 unsigned long x   : 1; /* Space-Switch-Event control */
+		 unsigned long r   : 1; /* Real-Space control */
+		 unsigned long     : 1;
+		 unsigned long dt  : 2; /* Designation-Type control */
+		 unsigned long tl  : 2; /* Region- or Segment-Table Length */
+	};
+};
+
+enum {
+	ASCE_TYPE_SEGMENT = 0,
+	ASCE_TYPE_REGION3 = 1,
+	ASCE_TYPE_REGION2 = 2,
+	ASCE_TYPE_REGION1 = 3
+};
+
+union region1_table_entry {
+	unsigned long val;
+	struct {
+		unsigned long rto: 52;/* Region-Table Origin */
+		unsigned long    : 2;
+		unsigned long p  : 1; /* DAT-Protection Bit */
+		unsigned long    : 1;
+		unsigned long tf : 2; /* Region-Second-Table Offset */
+		unsigned long i  : 1; /* Region-Invalid Bit */
+		unsigned long    : 1;
+		unsigned long tt : 2; /* Table-Type Bits */
+		unsigned long tl : 2; /* Region-Second-Table Length */
+	};
+};
+
+union region2_table_entry {
+	unsigned long val;
+	struct {
+		unsigned long rto: 52;/* Region-Table Origin */
+		unsigned long    : 2;
+		unsigned long p  : 1; /* DAT-Protection Bit */
+		unsigned long    : 1;
+		unsigned long tf : 2; /* Region-Third-Table Offset */
+		unsigned long i  : 1; /* Region-Invalid Bit */
+		unsigned long    : 1;
+		unsigned long tt : 2; /* Table-Type Bits */
+		unsigned long tl : 2; /* Region-Third-Table Length */
+	};
+};
+
+struct region3_table_entry_fc0 {
+	unsigned long sto: 52;/* Segment-Table Origin */
+	unsigned long    : 1;
+	unsigned long fc : 1; /* Format-Control */
+	unsigned long p  : 1; /* DAT-Protection Bit */
+	unsigned long    : 1;
+	unsigned long tf : 2; /* Segment-Table Offset */
+	unsigned long i  : 1; /* Region-Invalid Bit */
+	unsigned long cr : 1; /* Common-Region Bit */
+	unsigned long tt : 2; /* Table-Type Bits */
+	unsigned long tl : 2; /* Segment-Table Length */
+};
+
+struct region3_table_entry_fc1 {
+	unsigned long rfaa: 33;/* Region-Frame Absolute Address */
+	unsigned long     : 14;
+	unsigned long av  : 1; /* ACCF-Validity Control */
+	unsigned long acc : 4; /* Access-Control Bits */
+	unsigned long f   : 1; /* Fetch-Protection Bit */
+	unsigned long fc  : 1; /* Format-Control */
+	unsigned long p   : 1; /* DAT-Protection Bit */
+	unsigned long iep : 1; /* Instruction-Execution-Protection */
+	unsigned long     : 2;
+	unsigned long i   : 1; /* Region-Invalid Bit */
+	unsigned long cr  : 1; /* Common-Region Bit */
+	unsigned long tt  : 2; /* Table-Type Bits */
+	unsigned long     : 2;
+};
+
+union region3_table_entry {
+	unsigned long val;
+	struct region3_table_entry_fc0 fc0;
+	struct region3_table_entry_fc1 fc1;
+	struct {
+		unsigned long   : 53;
+		unsigned long fc: 1; /* Format-Control */
+		unsigned long   : 4;
+		unsigned long i : 1; /* Region-Invalid Bit */
+		unsigned long cr: 1; /* Common-Region Bit */
+		unsigned long tt: 2; /* Table-Type Bits */
+		unsigned long   : 2;
+	};
+};
+
+struct segment_table_entry_fc0 {
+	unsigned long pto: 53;/* Page-Table Origin */
+	unsigned long fc : 1; /* Format-Control */
+	unsigned long p  : 1; /* DAT-Protection Bit */
+	unsigned long    : 3;
+	unsigned long i  : 1; /* Segment-Invalid Bit */
+	unsigned long cs : 1; /* Common-Segment Bit */
+	unsigned long tt : 2; /* Table-Type Bits */
+	unsigned long    : 2;
+};
+
+struct segment_table_entry_fc1 {
+	unsigned long sfaa: 44;/* Segment-Frame Absolute Address */
+	unsigned long     : 3;
+	unsigned long av  : 1; /* ACCF-Validity Control */
+	unsigned long acc : 4; /* Access-Control Bits */
+	unsigned long f   : 1; /* Fetch-Protection Bit */
+	unsigned long fc  : 1; /* Format-Control */
+	unsigned long p   : 1; /* DAT-Protection Bit */
+	unsigned long iep : 1; /* Instruction-Execution-Protection */
+	unsigned long     : 2;
+	unsigned long i   : 1; /* Segment-Invalid Bit */
+	unsigned long cs  : 1; /* Common-Segment Bit */
+	unsigned long tt  : 2; /* Table-Type Bits */
+	unsigned long     : 2;
+};
+
+union segment_table_entry {
+	unsigned long val;
+	struct segment_table_entry_fc0 fc0;
+	struct segment_table_entry_fc1 fc1;
+	struct {
+		unsigned long   : 53;
+		unsigned long fc: 1; /* Format-Control */
+		unsigned long   : 4;
+		unsigned long i : 1; /* Segment-Invalid Bit */
+		unsigned long cs: 1; /* Common-Segment Bit */
+		unsigned long tt: 2; /* Table-Type Bits */
+		unsigned long   : 2;
+	};
+};
+
+union page_table_entry {
+	unsigned long val;
+	struct {
+		unsigned long pfra: 52;/* Page-Frame Real Address */
+		unsigned long z   : 1; /* Zero Bit */
+		unsigned long i   : 1; /* Page-Invalid Bit */
+		unsigned long p   : 1; /* DAT-Protection Bit */
+		unsigned long iep : 1; /* Instruction-Execution-Protection */
+		unsigned long     : 8;
+	};
+};
+
+enum {
+	TABLE_TYPE_SEGMENT = 0,
+	TABLE_TYPE_REGION3 = 1,
+	TABLE_TYPE_REGION2 = 2,
+	TABLE_TYPE_REGION1 = 3
+};
+
+#endif /* _S390_DAT_BITS_H */
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 5bf3d94e9dda..e65f597e3044 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -14,167 +14,10 @@
 #include <asm/access-regs.h>
 #include <asm/fault.h>
 #include <asm/gmap.h>
+#include <asm/dat-bits.h>
 #include "kvm-s390.h"
 #include "gaccess.h"
 
-union asce {
-	unsigned long val;
-	struct {
-		unsigned long origin : 52; /* Region- or Segment-Table Origin */
-		unsigned long	 : 2;
-		unsigned long g  : 1; /* Subspace Group Control */
-		unsigned long p  : 1; /* Private Space Control */
-		unsigned long s  : 1; /* Storage-Alteration-Event Control */
-		unsigned long x  : 1; /* Space-Switch-Event Control */
-		unsigned long r  : 1; /* Real-Space Control */
-		unsigned long	 : 1;
-		unsigned long dt : 2; /* Designation-Type Control */
-		unsigned long tl : 2; /* Region- or Segment-Table Length */
-	};
-};
-
-enum {
-	ASCE_TYPE_SEGMENT = 0,
-	ASCE_TYPE_REGION3 = 1,
-	ASCE_TYPE_REGION2 = 2,
-	ASCE_TYPE_REGION1 = 3
-};
-
-union region1_table_entry {
-	unsigned long val;
-	struct {
-		unsigned long rto: 52;/* Region-Table Origin */
-		unsigned long	 : 2;
-		unsigned long p  : 1; /* DAT-Protection Bit */
-		unsigned long	 : 1;
-		unsigned long tf : 2; /* Region-Second-Table Offset */
-		unsigned long i  : 1; /* Region-Invalid Bit */
-		unsigned long	 : 1;
-		unsigned long tt : 2; /* Table-Type Bits */
-		unsigned long tl : 2; /* Region-Second-Table Length */
-	};
-};
-
-union region2_table_entry {
-	unsigned long val;
-	struct {
-		unsigned long rto: 52;/* Region-Table Origin */
-		unsigned long	 : 2;
-		unsigned long p  : 1; /* DAT-Protection Bit */
-		unsigned long	 : 1;
-		unsigned long tf : 2; /* Region-Third-Table Offset */
-		unsigned long i  : 1; /* Region-Invalid Bit */
-		unsigned long	 : 1;
-		unsigned long tt : 2; /* Table-Type Bits */
-		unsigned long tl : 2; /* Region-Third-Table Length */
-	};
-};
-
-struct region3_table_entry_fc0 {
-	unsigned long sto: 52;/* Segment-Table Origin */
-	unsigned long	 : 1;
-	unsigned long fc : 1; /* Format-Control */
-	unsigned long p  : 1; /* DAT-Protection Bit */
-	unsigned long	 : 1;
-	unsigned long tf : 2; /* Segment-Table Offset */
-	unsigned long i  : 1; /* Region-Invalid Bit */
-	unsigned long cr : 1; /* Common-Region Bit */
-	unsigned long tt : 2; /* Table-Type Bits */
-	unsigned long tl : 2; /* Segment-Table Length */
-};
-
-struct region3_table_entry_fc1 {
-	unsigned long rfaa : 33; /* Region-Frame Absolute Address */
-	unsigned long	 : 14;
-	unsigned long av : 1; /* ACCF-Validity Control */
-	unsigned long acc: 4; /* Access-Control Bits */
-	unsigned long f  : 1; /* Fetch-Protection Bit */
-	unsigned long fc : 1; /* Format-Control */
-	unsigned long p  : 1; /* DAT-Protection Bit */
-	unsigned long iep: 1; /* Instruction-Execution-Protection */
-	unsigned long	 : 2;
-	unsigned long i  : 1; /* Region-Invalid Bit */
-	unsigned long cr : 1; /* Common-Region Bit */
-	unsigned long tt : 2; /* Table-Type Bits */
-	unsigned long	 : 2;
-};
-
-union region3_table_entry {
-	unsigned long val;
-	struct region3_table_entry_fc0 fc0;
-	struct region3_table_entry_fc1 fc1;
-	struct {
-		unsigned long	 : 53;
-		unsigned long fc : 1; /* Format-Control */
-		unsigned long	 : 4;
-		unsigned long i  : 1; /* Region-Invalid Bit */
-		unsigned long cr : 1; /* Common-Region Bit */
-		unsigned long tt : 2; /* Table-Type Bits */
-		unsigned long	 : 2;
-	};
-};
-
-struct segment_entry_fc0 {
-	unsigned long pto: 53;/* Page-Table Origin */
-	unsigned long fc : 1; /* Format-Control */
-	unsigned long p  : 1; /* DAT-Protection Bit */
-	unsigned long	 : 3;
-	unsigned long i  : 1; /* Segment-Invalid Bit */
-	unsigned long cs : 1; /* Common-Segment Bit */
-	unsigned long tt : 2; /* Table-Type Bits */
-	unsigned long	 : 2;
-};
-
-struct segment_entry_fc1 {
-	unsigned long sfaa : 44; /* Segment-Frame Absolute Address */
-	unsigned long	 : 3;
-	unsigned long av : 1; /* ACCF-Validity Control */
-	unsigned long acc: 4; /* Access-Control Bits */
-	unsigned long f  : 1; /* Fetch-Protection Bit */
-	unsigned long fc : 1; /* Format-Control */
-	unsigned long p  : 1; /* DAT-Protection Bit */
-	unsigned long iep: 1; /* Instruction-Execution-Protection */
-	unsigned long	 : 2;
-	unsigned long i  : 1; /* Segment-Invalid Bit */
-	unsigned long cs : 1; /* Common-Segment Bit */
-	unsigned long tt : 2; /* Table-Type Bits */
-	unsigned long	 : 2;
-};
-
-union segment_table_entry {
-	unsigned long val;
-	struct segment_entry_fc0 fc0;
-	struct segment_entry_fc1 fc1;
-	struct {
-		unsigned long	 : 53;
-		unsigned long fc : 1; /* Format-Control */
-		unsigned long	 : 4;
-		unsigned long i  : 1; /* Segment-Invalid Bit */
-		unsigned long cs : 1; /* Common-Segment Bit */
-		unsigned long tt : 2; /* Table-Type Bits */
-		unsigned long	 : 2;
-	};
-};
-
-enum {
-	TABLE_TYPE_SEGMENT = 0,
-	TABLE_TYPE_REGION3 = 1,
-	TABLE_TYPE_REGION2 = 2,
-	TABLE_TYPE_REGION1 = 3
-};
-
-union page_table_entry {
-	unsigned long val;
-	struct {
-		unsigned long pfra : 52; /* Page-Frame Real Address */
-		unsigned long z  : 1; /* Zero Bit */
-		unsigned long i  : 1; /* Page-Invalid Bit */
-		unsigned long p  : 1; /* DAT-Protection Bit */
-		unsigned long iep: 1; /* Instruction-Execution-Protection */
-		unsigned long	 : 8;
-	};
-};
-
 /*
  * vaddress union in order to easily decode a virtual address into its
  * region first index, region second index etc. parts.
@@ -632,7 +475,7 @@ static unsigned long guest_translate(struct kvm_vcpu *vcpu, unsigned long gva,
 	iep = ctlreg0.iep && test_kvm_facility(vcpu->kvm, 130);
 	if (asce.r)
 		goto real_address;
-	ptr = asce.origin * PAGE_SIZE;
+	ptr = asce.rsto * PAGE_SIZE;
 	switch (asce.dt) {
 	case ASCE_TYPE_REGION1:
 		if (vaddr.rfx01 > asce.tl)
@@ -1379,7 +1222,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
 	parent = sg->parent;
 	vaddr.addr = saddr;
 	asce.val = sg->orig_asce;
-	ptr = asce.origin * PAGE_SIZE;
+	ptr = asce.rsto * PAGE_SIZE;
 	if (asce.r) {
 		*fake = 1;
 		ptr = 0;
-- 
2.45.2

Re: [PATCH v1 2/2] s390/kvm: Move bitfields for dat tables

[Nico Boehr]

Quoting Claudio Imbrenda (2024-07-03 17:59:00)
> Move and improve the struct definitions for DAT tables from gaccess.c
> to a new header.
> 
> Once in a separate header, the structs become available everywhere. One
> possible usecase is to merge them in the s390 pte_t and p?d_t
> definitions, which is left as an exercise for the reader.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH v1 1/2] s390/entry: Pass the asce as parameter to sie64a()

[Nico Boehr]

Quoting Claudio Imbrenda (2024-07-03 17:58:59)
> Pass the guest ASCE explicitly as parameter, instead of having sie64a()
> take it from lowcore.
> 
> This removes hidden state from lowcore, and makes things look cleaner.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [PATCH v1 0/2] s390: Two small fixes and improvements

[Heiko Carstens]

On Wed, Jul 03, 2024 at 05:58:58PM +0200, Claudio Imbrenda wrote:
> The main goal of this small series is to do some clean-up and remove some
> paper cuts (or at least clear the way for papercuts to be removed in the
> future).
> 
> Heiko: this can go through the s390 tree, as agreed.
> 
> Claudio Imbrenda (2):
>   s390/entry: Pass the asce as parameter to sie64a()
>   s390/kvm: Move bitfields for dat tables
> 
>  arch/s390/include/asm/dat-bits.h   | 170 +++++++++++++++++++++++++++++
>  arch/s390/include/asm/kvm_host.h   |   7 +-
>  arch/s390/include/asm/stacktrace.h |   1 +
>  arch/s390/kernel/asm-offsets.c     |   1 +
>  arch/s390/kernel/entry.S           |   8 +-
>  arch/s390/kvm/gaccess.c            | 163 +--------------------------
>  arch/s390/kvm/kvm-s390.c           |   3 +-
>  arch/s390/kvm/vsie.c               |   2 +-
>  8 files changed, 185 insertions(+), 170 deletions(-)
>  create mode 100644 arch/s390/include/asm/dat-bits.h

Applied, thanks!

Re: [PATCH v1 1/2] s390/entry: Pass the asce as parameter to sie64a()

[Alexander Gordeev]

On Wed, Jul 03, 2024 at 05:58:59PM +0200, Claudio Imbrenda wrote:
> Pass the guest ASCE explicitly as parameter, instead of having sie64a()
> take it from lowcore.
> 
> This removes hidden state from lowcore, and makes things look cleaner.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h   | 7 ++++---
>  arch/s390/include/asm/stacktrace.h | 1 +
>  arch/s390/kernel/asm-offsets.c     | 1 +
>  arch/s390/kernel/entry.S           | 8 +++-----
>  arch/s390/kvm/kvm-s390.c           | 3 ++-
>  arch/s390/kvm/vsie.c               | 2 +-
>  6 files changed, 12 insertions(+), 10 deletions(-)

Acked-by: Alexander Gordeev <agordeev@linux.ibm.com>

Re: [PATCH v1 2/2] s390/kvm: Move bitfields for dat tables

[Alexander Gordeev]

On Wed, Jul 03, 2024 at 05:59:00PM +0200, Claudio Imbrenda wrote:

Hi Claudio,

> Once in a separate header, the structs become available everywhere. One
> possible usecase is to merge them in the s390 
> definitions, which is left as an exercise for the reader.

Is my understanding correct that you potentially see page_table_entry::val /
region?_table_entry.*::val / segment_table_entry.* merged with pte_t::pte /
p?d_t::p?d?

Thanks!

Re: [PATCH v1 2/2] s390/kvm: Move bitfields for dat tables

[Claudio Imbrenda]

On Thu, 11 Jul 2024 17:16:23 +0200
Alexander Gordeev <agordeev@linux.ibm.com> wrote:

> On Wed, Jul 03, 2024 at 05:59:00PM +0200, Claudio Imbrenda wrote:
> 
> Hi Claudio,
> 
> > Once in a separate header, the structs become available everywhere. One
> > possible usecase is to merge them in the s390 
> > definitions, which is left as an exercise for the reader.  
> 
> Is my understanding correct that you potentially see page_table_entry::val /
> region?_table_entry.*::val / segment_table_entry.* merged with pte_t::pte /
> p?d_t::p?d?
> 
> Thanks!

that depends on how you want to do the merge

you could do:

typedef union {
	unsigned long pte;
	union page_table_entry hw;
	union page_table_entry_softbits sw;
} pte_t;

then you would have pte_t::pte and pte_t::hw::val; unfortunately it's
not possible to anonymously merge a named type.. 

this would be great but can't be done*:

typedef union {
	unsigned long pte;
	union page_table_entry;
} pte_t;

[*] gcc actually supports it with an additional feature switch, but
it's not standard C and I seriously doubt we should even think about
doing it

another possibility is a plain

typedef union page_table_entry pte_t;

and then fix pte_val() and similar, but then you won't have the
softbits.


in the end, it's up to you how you want to merge them. I will
have my own unions that I will use only inside KVM, that's enough for
me.

Re: [PATCH v1 2/2] s390/kvm: Move bitfields for dat tables

[Heiko Carstens]

On Thu, Jul 11, 2024 at 06:23:48PM +0200, Claudio Imbrenda wrote:
> On Thu, 11 Jul 2024 17:16:23 +0200
> Alexander Gordeev <agordeev@linux.ibm.com> wrote:
> > Hi Claudio,
> > 
> > > Once in a separate header, the structs become available everywhere. One
> > > possible usecase is to merge them in the s390 
> > > definitions, which is left as an exercise for the reader.  
> > 
> > Is my understanding correct that you potentially see page_table_entry::val /
> > region?_table_entry.*::val / segment_table_entry.* merged with pte_t::pte /
> > p?d_t::p?d?
> 
> that depends on how you want to do the merge
> 
> you could do:
> 
> typedef union {
> 	unsigned long pte;
> 	union page_table_entry hw;
> 	union page_table_entry_softbits sw;
> } pte_t;
> 
> then you would have pte_t::pte and pte_t::hw::val; unfortunately it's
> not possible to anonymously merge a named type.. 
> 
> this would be great but can't be done*:
> 
> typedef union {
> 	unsigned long pte;
> 	union page_table_entry;
> } pte_t;
> 
> [*] gcc actually supports it with an additional feature switch, but
> it's not standard C and I seriously doubt we should even think about
> doing it
> 
> another possibility is a plain
> 
> typedef union page_table_entry pte_t;
> 
> and then fix pte_val() and similar, but then you won't have the
> softbits.
> 
> 
> in the end, it's up to you how you want to merge them. I will
> have my own unions that I will use only inside KVM, that's enough for
> me.

We discussed this, and I really don't think we want this for the software
defined bits. In general I do like the idea of having bit fields and let
gcc do the decoding. But we have so many masks we use every for ptes and
friends that a single assignment won't work in most cases. So we eiter have
to do several assignments (which sometimes is a no-go when block concurrent
updates are required), or we have to stay with using both the existing
defines plus the new unions - which makes things even more complicated.

There might be uses cases where the hardware structures are also useful for
s390 core code, but I don't think we should go the route outlined above.