[kvm-unit-tests PATCH v2 0/3] s390x: Rework TEID decoding and usage

[kvm-unit-tests PATCH v2 0/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch]

The translation-exception identification (TEID) contains information to
identify the cause of certain program exceptions, including translation
exceptions occurring during dynamic address translation, as well as
protection exceptions.
The meaning of fields in the TEID is complex, depending on the exception
occurring and various potentially installed facilities.

Add function to query which suppression-on-protection facility is
installed.
Rework the type describing the TEID, in order to ease decoding.
Change the existing code interpreting the TEID and extend it to take the
installed suppression-on-protection facility into account.

Also fix the sclp bit order.
This series is based on  v3 of series s390x: Avoid gcc 12 warnings .
The sclp fix is taken from v2 More skey instr. emulation test,
and could be picked independently from the rest of the series.

v1 -> v2
 * pick up r-b
 * get rid of esop1 alias of sop_teid_predictable
 * assert that the esop2 protection code is valid
 * use string literal array and indexing for protection code printing
 * fix protection exception check in edat.c

Janis Schoetterl-Glausch (3):
  s390x: Fix sclp facility bit numbers
  s390x: lib: SOP facility query function
  s390x: Rework TEID decoding and usage

 lib/s390x/asm/facility.h  | 21 +++++++++++++
 lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
 lib/s390x/fault.h         | 30 +++++-------------
 lib/s390x/sclp.h          | 18 ++++++-----
 lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
 lib/s390x/interrupt.c     |  2 +-
 lib/s390x/sclp.c          |  2 ++
 s390x/edat.c              | 26 ++++++++++------
 8 files changed, 149 insertions(+), 76 deletions(-)

Range-diff against v1:
1:  abb9b37a = 1:  6427944a s390x: Fix sclp facility bit numbers
2:  5662f4bc ! 2:  a08fce3b s390x: lib: SOP facility query function
    @@ Commit message
         installed.
     
         Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    +    Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
     
      ## lib/s390x/asm/facility.h ##
     @@
3:  b716e5ef ! 3:  eb268af1 s390x: Rework TEID decoding and usage
    @@ lib/s390x/asm/interrupt.h
     +			uint64_t addr			: 52 -  0;
     +			uint64_t acc_exc_f_s		: 54 - 52;
     +			uint64_t side_effect_acc	: 55 - 54;
    -+			uint64_t /* reserved */		: 55 - 54;
    ++			uint64_t /* reserved */		: 62 - 55;
     +			uint64_t asce_id		: 64 - 62;
     +		};
     +		/* DAT exc */
    @@ lib/s390x/asm/interrupt.h
     +			uint64_t sop_acc_list		: 61 - 60;
     +			uint64_t sop_teid_predictable	: 62 - 61;
     +		};
    -+		/* enhanced suppression on protection 1 */
    -+		struct {
    -+			uint64_t /* pad */		: 61 -  0;
    -+			uint64_t esop1_acc_list_or_dat	: 62 - 61;
    -+		};
     +		/* enhanced suppression on protection 2 */
     +		struct {
     +			uint64_t /* pad */		: 56 -  0;
    @@ lib/s390x/asm/interrupt.h
     +
     +static inline enum prot_code teid_esop2_prot_code(union teid teid)
     +{
    -+	int code = 0;
    ++	int code = (teid.esop2_prot_code_0 << 2 |
    ++		    teid.esop2_prot_code_1 << 1 |
    ++		    teid.esop2_prot_code_2);
     +
    -+	code = code << 1 | teid.esop2_prot_code_0;
    -+	code = code << 1 | teid.esop2_prot_code_1;
    -+	code = code << 1 | teid.esop2_prot_code_2;
    ++	assert(code < 6);
     +	return (enum prot_code)code;
     +}
     +
    @@ lib/s390x/fault.c
     -		printf("Type: LAP\n");
     -		return;
     -	}
    --
    + 
     -	if (prot_is_iep(teid)) {
     -		printf("Type: IEP\n");
     -		return;
     -	}
    - 
    --	if (prot_is_datp(teid)) {
    --		printf("Type: DAT\n");
    --		return;
     +static void print_decode_pgm_prot(union teid teid, bool dat)
     +{
     +	switch (get_supp_on_prot_facility()) {
    @@ lib/s390x/fault.c
     +			printf("Type: ?\n");
     +		break;
     +	case SOP_ENHANCED_1:
    -+		if (teid.esop1_acc_list_or_dat) {
    ++		if (teid.sop_teid_predictable) {/* implies access list or DAT */
     +			if (teid.sop_acc_list)
     +				printf("Type: ACC\n");
     +			else
    @@ lib/s390x/fault.c
     +			printf("Type: KEY or LAP\n");
     +		}
     +		break;
    -+	case SOP_ENHANCED_2:
    -+		switch (teid_esop2_prot_code(teid)) {
    -+		case PROT_KEY_LAP:
    -+			printf("Type: KEY or LAP\n");
    -+			break;
    -+		case PROT_DAT:
    -+			printf("Type: DAT\n");
    -+			break;
    -+		case PROT_KEY:
    -+			printf("Type: KEY\n");
    -+			break;
    -+		case PROT_ACC_LIST:
    -+			printf("Type: ACC\n");
    -+			break;
    -+		case PROT_LAP:
    -+			printf("Type: LAP\n");
    -+			break;
    -+		case PROT_IEP:
    -+			printf("Type: IEP\n");
    -+			break;
    ++	case SOP_ENHANCED_2: {
    ++		static const char * const prot_str[] = {
    ++			"KEY or LAP",
    ++			"DAT",
    ++			"KEY",
    ++			"ACC",
    ++			"LAP",
    ++			"IEP",
    ++		};
    ++		int prot_code = teid_esop2_prot_code(teid);
    + 
    +-	if (prot_is_datp(teid)) {
    +-		printf("Type: DAT\n");
    +-		return;
    ++		assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));
    ++		printf("Type: %s\n", prot_str[prot_code]);
     +		}
      	}
      }
    @@ lib/s390x/interrupt.c: static void fixup_pgm_int(struct stack_frame_int *stack)
      			 * the exception so we can use the return
     
      ## s390x/edat.c ##
    +@@ s390x/edat.c: static void *root, *mem, *m;
    + volatile unsigned int *p;
    + 
    + /*
    +- * Check if a non-access-list protection exception happened for the given
    +- * address, in the primary address space.
    ++ * Check if the exception is consistent with DAT protection and has the correct
    ++ * address and primary address space.
    +  */
    + static bool check_pgm_prot(void *ptr)
    + {
     @@ s390x/edat.c: static bool check_pgm_prot(void *ptr)
      		return false;
      
    @@ s390x/edat.c: static bool check_pgm_prot(void *ptr)
     +	case SOP_BASIC:
     +		if (!teid.sop_teid_predictable)
     +			return true;
    ++		break;
     +	case SOP_ENHANCED_1:
    -+		if (!teid.esop1_acc_list_or_dat)
    ++		if (!teid.sop_teid_predictable) /* implies key or low addr */
     +			return false;
    ++		break;
     +	case SOP_ENHANCED_2:
    -+		if (teid_esop2_prot_code(teid) != 1)
    ++		if (teid_esop2_prot_code(teid) != PROT_DAT)
     +			return false;
     +	}
     +	return (!teid.sop_acc_list && !teid.asce_id &&

base-commit: 2eed0bf1096077144cc3a0dd9974689487f9511a
prerequisite-patch-id: aa682f50e4eba0e9b6cacd245d568f5bcca05e0f
prerequisite-patch-id: 79a88ac3faff3ae2ef214bf4a90de7463e2fdc8a
-- 
2.33.1

[kvm-unit-tests PATCH v2 1/3] s390x: Fix sclp facility bit numbers

[Janis Schoetterl-Glausch]

sclp_feat_check takes care of adjusting the bit numbering such that they
can be defined as they are in the documentation.

Fixes: 4dd649c8 ("lib: s390x: sclp: Extend feature probing")
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/sclp.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index e48a5a3d..3488f4d2 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -134,13 +134,13 @@ struct sclp_facilities {
 };
 
 /* bit number within a certain byte */
-#define SCLP_FEAT_85_BIT_GSLS		7
-#define SCLP_FEAT_98_BIT_KSS		0
-#define SCLP_FEAT_116_BIT_64BSCAO	7
-#define SCLP_FEAT_116_BIT_CMMA		6
-#define SCLP_FEAT_116_BIT_ESCA		3
-#define SCLP_FEAT_117_BIT_PFMFI		6
-#define SCLP_FEAT_117_BIT_IBS		5
+#define SCLP_FEAT_85_BIT_GSLS		0
+#define SCLP_FEAT_98_BIT_KSS		7
+#define SCLP_FEAT_116_BIT_64BSCAO	0
+#define SCLP_FEAT_116_BIT_CMMA		1
+#define SCLP_FEAT_116_BIT_ESCA		4
+#define SCLP_FEAT_117_BIT_PFMFI		1
+#define SCLP_FEAT_117_BIT_IBS		2
 
 typedef struct ReadInfo {
 	SCCBHeader h;
-- 
2.33.1

[kvm-unit-tests PATCH v2 2/3] s390x: lib: SOP facility query function

[Janis Schoetterl-Glausch]

Add function returning which suppression-on-protection facility is
installed.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/asm/facility.h | 21 +++++++++++++++++++++
 lib/s390x/sclp.h         |  4 ++++
 lib/s390x/sclp.c         |  2 ++
 3 files changed, 27 insertions(+)

diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
index 49380203..a66fe56a 100644
--- a/lib/s390x/asm/facility.h
+++ b/lib/s390x/asm/facility.h
@@ -12,6 +12,7 @@
 #include <asm/facility.h>
 #include <asm/arch_def.h>
 #include <bitops.h>
+#include <sclp.h>
 
 #define NB_STFL_DOUBLEWORDS 32
 extern uint64_t stfl_doublewords[];
@@ -42,4 +43,24 @@ static inline void setup_facilities(void)
 		stfle(stfl_doublewords, NB_STFL_DOUBLEWORDS);
 }
 
+enum supp_on_prot_facility {
+	SOP_NONE,
+	SOP_BASIC,
+	SOP_ENHANCED_1,
+	SOP_ENHANCED_2,
+};
+
+static inline enum supp_on_prot_facility get_supp_on_prot_facility(void)
+{
+	if (sclp_facilities.has_esop) {
+		if (test_facility(131)) /* side-effect-access facility */
+			return SOP_ENHANCED_2;
+		else
+			return SOP_ENHANCED_1;
+	}
+	if (sclp_facilities.has_sop)
+		return SOP_BASIC;
+	return SOP_NONE;
+}
+
 #endif
diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
index 3488f4d2..853529bf 100644
--- a/lib/s390x/sclp.h
+++ b/lib/s390x/sclp.h
@@ -123,7 +123,9 @@ struct sclp_facilities {
 	uint64_t has_cei : 1;
 
 	uint64_t has_diag318 : 1;
+	uint64_t has_sop : 1;
 	uint64_t has_gsls : 1;
+	uint64_t has_esop : 1;
 	uint64_t has_cmma : 1;
 	uint64_t has_64bscao : 1;
 	uint64_t has_esca : 1;
@@ -134,7 +136,9 @@ struct sclp_facilities {
 };
 
 /* bit number within a certain byte */
+#define SCLP_FEAT_80_BIT_SOP		2
 #define SCLP_FEAT_85_BIT_GSLS		0
+#define SCLP_FEAT_85_BIT_ESOP		6
 #define SCLP_FEAT_98_BIT_KSS		7
 #define SCLP_FEAT_116_BIT_64BSCAO	0
 #define SCLP_FEAT_116_BIT_CMMA		1
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index b8204c5f..e6017f64 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -152,7 +152,9 @@ void sclp_facilities_setup(void)
 	cpu = sclp_get_cpu_entries();
 	if (read_info->offset_cpu > 134)
 		sclp_facilities.has_diag318 = read_info->byte_134_diag318;
+	sclp_facilities.has_sop = sclp_feat_check(80, SCLP_FEAT_80_BIT_SOP);
 	sclp_facilities.has_gsls = sclp_feat_check(85, SCLP_FEAT_85_BIT_GSLS);
+	sclp_facilities.has_esop = sclp_feat_check(85, SCLP_FEAT_85_BIT_ESOP);
 	sclp_facilities.has_kss = sclp_feat_check(98, SCLP_FEAT_98_BIT_KSS);
 	sclp_facilities.has_cmma = sclp_feat_check(116, SCLP_FEAT_116_BIT_CMMA);
 	sclp_facilities.has_64bscao = sclp_feat_check(116, SCLP_FEAT_116_BIT_64BSCAO);
-- 
2.33.1

[kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch]

The translation-exception identification (TEID) contains information to
identify the cause of certain program exceptions, including translation
exceptions occurring during dynamic address translation, as well as
protection exceptions.
The meaning of fields in the TEID is complex, depending on the exception
occurring and various potentially installed facilities.

Rework the type describing the TEID, in order to ease decoding.
Change the existing code interpreting the TEID and extend it to take the
installed suppression-on-protection facility into account.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
 lib/s390x/fault.h         | 30 +++++-------------
 lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
 lib/s390x/interrupt.c     |  2 +-
 s390x/edat.c              | 26 ++++++++++------
 5 files changed, 115 insertions(+), 69 deletions(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index d9ab0bd7..3ca6bf76 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -20,23 +20,56 @@
 
 union teid {
 	unsigned long val;
-	struct {
-		unsigned long addr:52;
-		unsigned long fetch:1;
-		unsigned long store:1;
-		unsigned long reserved:6;
-		unsigned long acc_list_prot:1;
-		/*
-		 * depending on the exception and the installed facilities,
-		 * the m field can indicate several different things,
-		 * including whether the exception was triggered by a MVPG
-		 * instruction, or whether the addr field is meaningful
-		 */
-		unsigned long m:1;
-		unsigned long asce_id:2;
+	union {
+		/* common fields DAT exc & protection exc */
+		struct {
+			uint64_t addr			: 52 -  0;
+			uint64_t acc_exc_f_s		: 54 - 52;
+			uint64_t side_effect_acc	: 55 - 54;
+			uint64_t /* reserved */		: 62 - 55;
+			uint64_t asce_id		: 64 - 62;
+		};
+		/* DAT exc */
+		struct {
+			uint64_t /* pad */		: 61 -  0;
+			uint64_t dat_move_page		: 62 - 61;
+		};
+		/* suppression on protection */
+		struct {
+			uint64_t /* pad */		: 60 -  0;
+			uint64_t sop_acc_list		: 61 - 60;
+			uint64_t sop_teid_predictable	: 62 - 61;
+		};
+		/* enhanced suppression on protection 2 */
+		struct {
+			uint64_t /* pad */		: 56 -  0;
+			uint64_t esop2_prot_code_0	: 57 - 56;
+			uint64_t /* pad */		: 60 - 57;
+			uint64_t esop2_prot_code_1	: 61 - 60;
+			uint64_t esop2_prot_code_2	: 62 - 61;
+		};
 	};
 };
 
+enum prot_code {
+	PROT_KEY_LAP,
+	PROT_DAT,
+	PROT_KEY,
+	PROT_ACC_LIST,
+	PROT_LAP,
+	PROT_IEP,
+};
+
+static inline enum prot_code teid_esop2_prot_code(union teid teid)
+{
+	int code = (teid.esop2_prot_code_0 << 2 |
+		    teid.esop2_prot_code_1 << 1 |
+		    teid.esop2_prot_code_2);
+
+	assert(code < 6);
+	return (enum prot_code)code;
+}
+
 void register_pgm_cleanup_func(void (*f)(void));
 void handle_pgm_int(struct stack_frame_int *stack);
 void handle_ext_int(struct stack_frame_int *stack);
diff --git a/lib/s390x/fault.h b/lib/s390x/fault.h
index 726da2f0..867997f2 100644
--- a/lib/s390x/fault.h
+++ b/lib/s390x/fault.h
@@ -11,32 +11,16 @@
 #define _S390X_FAULT_H_
 
 #include <bitops.h>
+#include <asm/facility.h>
+#include <asm/interrupt.h>
 
 /* Instruction execution prevention, i.e. no-execute, 101 */
-static inline bool prot_is_iep(uint64_t teid)
+static inline bool prot_is_iep(union teid teid)
 {
-	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid))
-		return true;
-
-	return false;
-}
-
-/* Standard DAT exception, 001 */
-static inline bool prot_is_datp(uint64_t teid)
-{
-	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid))
-		return true;
-
-	return false;
-}
-
-/* Low-address protection exception, 100 */
-static inline bool prot_is_lap(uint64_t teid)
-{
-	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid))
-		return true;
-
-	return false;
+	if (!test_facility(130))
+		return false;
+	/* IEP installed -> ESOP2 installed */
+	return teid_esop2_prot_code(teid) == PROT_IEP;
 }
 
 void print_decode_teid(uint64_t teid);
diff --git a/lib/s390x/fault.c b/lib/s390x/fault.c
index efa62fcb..b75152f5 100644
--- a/lib/s390x/fault.c
+++ b/lib/s390x/fault.c
@@ -13,35 +13,56 @@
 #include <asm/page.h>
 #include <fault.h>
 
-/* Decodes the protection exceptions we'll most likely see */
-static void print_decode_pgm_prot(uint64_t teid)
-{
-	if (prot_is_lap(teid)) {
-		printf("Type: LAP\n");
-		return;
-	}
 
-	if (prot_is_iep(teid)) {
-		printf("Type: IEP\n");
-		return;
-	}
+static void print_decode_pgm_prot(union teid teid, bool dat)
+{
+	switch (get_supp_on_prot_facility()) {
+	case SOP_NONE:
+		printf("Type: ?\n");
+		break;
+	case SOP_BASIC:
+		if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
+			printf("Type: ACC\n");
+		else
+			printf("Type: ?\n");
+		break;
+	case SOP_ENHANCED_1:
+		if (teid.sop_teid_predictable) {/* implies access list or DAT */
+			if (teid.sop_acc_list)
+				printf("Type: ACC\n");
+			else
+				printf("Type: DAT\n");
+		} else {
+			printf("Type: KEY or LAP\n");
+		}
+		break;
+	case SOP_ENHANCED_2: {
+		static const char * const prot_str[] = {
+			"KEY or LAP",
+			"DAT",
+			"KEY",
+			"ACC",
+			"LAP",
+			"IEP",
+		};
+		int prot_code = teid_esop2_prot_code(teid);
 
-	if (prot_is_datp(teid)) {
-		printf("Type: DAT\n");
-		return;
+		assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));
+		printf("Type: %s\n", prot_str[prot_code]);
+		}
 	}
 }
 
-void print_decode_teid(uint64_t teid)
+void print_decode_teid(uint64_t raw_teid)
 {
-	int asce_id = teid & 3;
+	union teid teid = { .val = raw_teid };
 	bool dat = lowcore.pgm_old_psw.mask & PSW_MASK_DAT;
 
 	printf("Memory exception information:\n");
 	printf("DAT: %s\n", dat ? "on" : "off");
 
 	printf("AS: ");
-	switch (asce_id) {
+	switch (teid.asce_id) {
 	case AS_PRIM:
 		printf("Primary\n");
 		break;
@@ -57,7 +78,7 @@ void print_decode_teid(uint64_t teid)
 	}
 
 	if (lowcore.pgm_int_code == PGM_INT_CODE_PROTECTION)
-		print_decode_pgm_prot(teid);
+		print_decode_pgm_prot(teid, dat);
 
 	/*
 	 * If teid bit 61 is off for these two exception the reported
@@ -65,10 +86,10 @@ void print_decode_teid(uint64_t teid)
 	 */
 	if ((lowcore.pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
 	     lowcore.pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
-	    !test_bit_inv(61, &teid)) {
-		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
+	    !teid.sop_teid_predictable) {
+		printf("Address: %lx, unpredictable\n ", raw_teid & PAGE_MASK);
 		return;
 	}
-	printf("TEID: %lx\n", teid);
-	printf("Address: %lx\n\n", teid & PAGE_MASK);
+	printf("TEID: %lx\n", raw_teid);
+	printf("Address: %lx\n\n", raw_teid & PAGE_MASK);
 }
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 6da20c44..ac3d1ecd 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -77,7 +77,7 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 		break;
 	case PGM_INT_CODE_PROTECTION:
 		/* Handling for iep.c test case. */
-		if (prot_is_iep(lowcore.trans_exc_id))
+		if (prot_is_iep((union teid) { .val = lowcore.trans_exc_id }))
 			/*
 			 * We branched to the instruction that caused
 			 * the exception so we can use the return
diff --git a/s390x/edat.c b/s390x/edat.c
index c6c25042..89ff4f51 100644
--- a/s390x/edat.c
+++ b/s390x/edat.c
@@ -26,8 +26,8 @@ static void *root, *mem, *m;
 volatile unsigned int *p;
 
 /*
- * Check if a non-access-list protection exception happened for the given
- * address, in the primary address space.
+ * Check if the exception is consistent with DAT protection and has the correct
+ * address and primary address space.
  */
 static bool check_pgm_prot(void *ptr)
 {
@@ -37,14 +37,22 @@ static bool check_pgm_prot(void *ptr)
 		return false;
 
 	teid.val = lowcore.trans_exc_id;
-
-	/*
-	 * depending on the presence of the ESOP feature, the rest of the
-	 * field might or might not be meaningful when the m field is 0.
-	 */
-	if (!teid.m)
+	switch (get_supp_on_prot_facility()) {
+	case SOP_NONE:
 		return true;
-	return (!teid.acc_list_prot && !teid.asce_id &&
+	case SOP_BASIC:
+		if (!teid.sop_teid_predictable)
+			return true;
+		break;
+	case SOP_ENHANCED_1:
+		if (!teid.sop_teid_predictable) /* implies key or low addr */
+			return false;
+		break;
+	case SOP_ENHANCED_2:
+		if (teid_esop2_prot_code(teid) != PROT_DAT)
+			return false;
+	}
+	return (!teid.sop_acc_list && !teid.asce_id &&
 		(teid.addr == ((unsigned long)ptr >> PAGE_SHIFT)));
 }
 
-- 
2.33.1

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Claudio Imbrenda]

On Wed,  8 Jun 2022 15:33:03 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> The translation-exception identification (TEID) contains information to
> identify the cause of certain program exceptions, including translation
> exceptions occurring during dynamic address translation, as well as
> protection exceptions.
> The meaning of fields in the TEID is complex, depending on the exception
> occurring and various potentially installed facilities.
> 
> Rework the type describing the TEID, in order to ease decoding.
> Change the existing code interpreting the TEID and extend it to take the
> installed suppression-on-protection facility into account.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
>  lib/s390x/fault.h         | 30 +++++-------------
>  lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
>  lib/s390x/interrupt.c     |  2 +-
>  s390x/edat.c              | 26 ++++++++++------
>  5 files changed, 115 insertions(+), 69 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index d9ab0bd7..3ca6bf76 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -20,23 +20,56 @@
>  
>  union teid {
>  	unsigned long val;
> -	struct {
> -		unsigned long addr:52;
> -		unsigned long fetch:1;
> -		unsigned long store:1;
> -		unsigned long reserved:6;
> -		unsigned long acc_list_prot:1;
> -		/*
> -		 * depending on the exception and the installed facilities,
> -		 * the m field can indicate several different things,
> -		 * including whether the exception was triggered by a MVPG
> -		 * instruction, or whether the addr field is meaningful
> -		 */
> -		unsigned long m:1;
> -		unsigned long asce_id:2;
> +	union {
> +		/* common fields DAT exc & protection exc */
> +		struct {
> +			uint64_t addr			: 52 -  0;
> +			uint64_t acc_exc_f_s		: 54 - 52;
> +			uint64_t side_effect_acc	: 55 - 54;
> +			uint64_t /* reserved */		: 62 - 55;
> +			uint64_t asce_id		: 64 - 62;
> +		};
> +		/* DAT exc */
> +		struct {
> +			uint64_t /* pad */		: 61 -  0;
> +			uint64_t dat_move_page		: 62 - 61;
> +		};
> +		/* suppression on protection */
> +		struct {
> +			uint64_t /* pad */		: 60 -  0;
> +			uint64_t sop_acc_list		: 61 - 60;
> +			uint64_t sop_teid_predictable	: 62 - 61;
> +		};
> +		/* enhanced suppression on protection 2 */
> +		struct {
> +			uint64_t /* pad */		: 56 -  0;
> +			uint64_t esop2_prot_code_0	: 57 - 56;
> +			uint64_t /* pad */		: 60 - 57;
> +			uint64_t esop2_prot_code_1	: 61 - 60;
> +			uint64_t esop2_prot_code_2	: 62 - 61;
> +		};
>  	};
>  };
>  
> +enum prot_code {
> +	PROT_KEY_LAP,
> +	PROT_DAT,
> +	PROT_KEY,
> +	PROT_ACC_LIST,
> +	PROT_LAP,
> +	PROT_IEP,

add:
	PROT_CODE_SIZE,	/* Must always be the last one */

[...]

> +	case SOP_ENHANCED_2: {
> +		static const char * const prot_str[] = {

static const char * const prot_str[PROT_CODE_SIZE] = {

so you have the guarantee that this has the right size, and you will
get a compile error if a new value is added to the enum but not here

and at this point I think it might make more sense to move this right
after the enum itself

> +			"KEY or LAP",
> +			"DAT",
> +			"KEY",
> +			"ACC",
> +			"LAP",
> +			"IEP",
> +		};
> +		int prot_code = teid_esop2_prot_code(teid);

enum prot_code prot_code = teid_esop2_prot_code(teid);

>  
> -	if (prot_is_datp(teid)) {
> -		printf("Type: DAT\n");
> -		return;
> +		assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));

then you can remove this assert ^

> +		printf("Type: %s\n", prot_str[prot_code]);
> +		}
>  	}
>  }
>  
> -void print_decode_teid(uint64_t teid)
> +void print_decode_teid(uint64_t raw_teid)
>  {
> -	int asce_id = teid & 3;
> +	union teid teid = { .val = raw_teid };
>  	bool dat = lowcore.pgm_old_psw.mask & PSW_MASK_DAT;
>  
>  	printf("Memory exception information:\n");
>  	printf("DAT: %s\n", dat ? "on" : "off");
>  
>  	printf("AS: ");
> -	switch (asce_id) {
> +	switch (teid.asce_id) {
>  	case AS_PRIM:
>  		printf("Primary\n");
>  		break;
> @@ -57,7 +78,7 @@ void print_decode_teid(uint64_t teid)
>  	}
>  
>  	if (lowcore.pgm_int_code == PGM_INT_CODE_PROTECTION)
> -		print_decode_pgm_prot(teid);
> +		print_decode_pgm_prot(teid, dat);
>  
>  	/*
>  	 * If teid bit 61 is off for these two exception the reported
> @@ -65,10 +86,10 @@ void print_decode_teid(uint64_t teid)
>  	 */
>  	if ((lowcore.pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
>  	     lowcore.pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
> -	    !test_bit_inv(61, &teid)) {
> -		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
> +	    !teid.sop_teid_predictable) {
> +		printf("Address: %lx, unpredictable\n ", raw_teid & PAGE_MASK);
>  		return;
>  	}
> -	printf("TEID: %lx\n", teid);
> -	printf("Address: %lx\n\n", teid & PAGE_MASK);
> +	printf("TEID: %lx\n", raw_teid);
> +	printf("Address: %lx\n\n", raw_teid & PAGE_MASK);
>  }
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 6da20c44..ac3d1ecd 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -77,7 +77,7 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>  		break;
>  	case PGM_INT_CODE_PROTECTION:
>  		/* Handling for iep.c test case. */
> -		if (prot_is_iep(lowcore.trans_exc_id))
> +		if (prot_is_iep((union teid) { .val = lowcore.trans_exc_id }))
>  			/*
>  			 * We branched to the instruction that caused
>  			 * the exception so we can use the return
> diff --git a/s390x/edat.c b/s390x/edat.c
> index c6c25042..89ff4f51 100644
> --- a/s390x/edat.c
> +++ b/s390x/edat.c
> @@ -26,8 +26,8 @@ static void *root, *mem, *m;
>  volatile unsigned int *p;
>  
>  /*
> - * Check if a non-access-list protection exception happened for the given
> - * address, in the primary address space.
> + * Check if the exception is consistent with DAT protection and has the correct
> + * address and primary address space.
>   */
>  static bool check_pgm_prot(void *ptr)
>  {
> @@ -37,14 +37,22 @@ static bool check_pgm_prot(void *ptr)
>  		return false;
>  
>  	teid.val = lowcore.trans_exc_id;
> -
> -	/*
> -	 * depending on the presence of the ESOP feature, the rest of the
> -	 * field might or might not be meaningful when the m field is 0.
> -	 */
> -	if (!teid.m)
> +	switch (get_supp_on_prot_facility()) {
> +	case SOP_NONE:
>  		return true;
> -	return (!teid.acc_list_prot && !teid.asce_id &&
> +	case SOP_BASIC:
> +		if (!teid.sop_teid_predictable)
> +			return true;
> +		break;
> +	case SOP_ENHANCED_1:
> +		if (!teid.sop_teid_predictable) /* implies key or low addr */
> +			return false;
> +		break;
> +	case SOP_ENHANCED_2:
> +		if (teid_esop2_prot_code(teid) != PROT_DAT)
> +			return false;
> +	}
> +	return (!teid.sop_acc_list && !teid.asce_id &&
>  		(teid.addr == ((unsigned long)ptr >> PAGE_SHIFT)));
>  }
>  

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch]

On 6/8/22 16:03, Claudio Imbrenda wrote:
> On Wed,  8 Jun 2022 15:33:03 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> The translation-exception identification (TEID) contains information to
>> identify the cause of certain program exceptions, including translation
>> exceptions occurring during dynamic address translation, as well as
>> protection exceptions.
>> The meaning of fields in the TEID is complex, depending on the exception
>> occurring and various potentially installed facilities.
>>
>> Rework the type describing the TEID, in order to ease decoding.
>> Change the existing code interpreting the TEID and extend it to take the
>> installed suppression-on-protection facility into account.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
>>  lib/s390x/fault.h         | 30 +++++-------------
>>  lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
>>  lib/s390x/interrupt.c     |  2 +-
>>  s390x/edat.c              | 26 ++++++++++------
>>  5 files changed, 115 insertions(+), 69 deletions(-)
>>
>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>> index d9ab0bd7..3ca6bf76 100644
>> --- a/lib/s390x/asm/interrupt.h
>> +++ b/lib/s390x/asm/interrupt.h
>> @@ -20,23 +20,56 @@
>>  

[...]

>>  
>> +enum prot_code {
>> +	PROT_KEY_LAP,
>> +	PROT_DAT,
>> +	PROT_KEY,
>> +	PROT_ACC_LIST,
>> +	PROT_LAP,
>> +	PROT_IEP,
> 
> add:
> 	PROT_CODE_SIZE,	/* Must always be the last one */
> 
> [...]
> 
>> +	case SOP_ENHANCED_2: {
>> +		static const char * const prot_str[] = {
> 
> static const char * const prot_str[PROT_CODE_SIZE] = {
> 
> so you have the guarantee that this has the right size, and you will
> get a compile error if a new value is added to the enum but not here

Will I? It would just initialize missing elements with NULL, no?
> 
> and at this point I think it might make more sense to move this right
> after the enum itself
> 
>> +			"KEY or LAP",
>> +			"DAT",
>> +			"KEY",
>> +			"ACC",
>> +			"LAP",
>> +			"IEP",
>> +		};
>> +		int prot_code = teid_esop2_prot_code(teid);
> 
> enum prot_code prot_code = teid_esop2_prot_code(teid)> 
>>  
>> -	if (prot_is_datp(teid)) {
>> -		printf("Type: DAT\n");
>> -		return;
>> +		assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));
> 
> then you can remove this assert ^
> 
>> +		printf("Type: %s\n", prot_str[prot_code]);
>> +		}
>>  	}
>>  }
>>  
[...]

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Claudio Imbrenda]

On Wed, 8 Jun 2022 17:55:08 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On 6/8/22 16:03, Claudio Imbrenda wrote:
> > On Wed,  8 Jun 2022 15:33:03 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> >> The translation-exception identification (TEID) contains information to
> >> identify the cause of certain program exceptions, including translation
> >> exceptions occurring during dynamic address translation, as well as
> >> protection exceptions.
> >> The meaning of fields in the TEID is complex, depending on the exception
> >> occurring and various potentially installed facilities.
> >>
> >> Rework the type describing the TEID, in order to ease decoding.
> >> Change the existing code interpreting the TEID and extend it to take the
> >> installed suppression-on-protection facility into account.
> >>
> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> >> ---
> >>  lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
> >>  lib/s390x/fault.h         | 30 +++++-------------
> >>  lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
> >>  lib/s390x/interrupt.c     |  2 +-
> >>  s390x/edat.c              | 26 ++++++++++------
> >>  5 files changed, 115 insertions(+), 69 deletions(-)
> >>
> >> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> >> index d9ab0bd7..3ca6bf76 100644
> >> --- a/lib/s390x/asm/interrupt.h
> >> +++ b/lib/s390x/asm/interrupt.h
> >> @@ -20,23 +20,56 @@
> >>    
> 
> [...]
> 
> >>  
> >> +enum prot_code {
> >> +	PROT_KEY_LAP,
> >> +	PROT_DAT,
> >> +	PROT_KEY,
> >> +	PROT_ACC_LIST,
> >> +	PROT_LAP,
> >> +	PROT_IEP,  
> > 
> > add:
> > 	PROT_CODE_SIZE,	/* Must always be the last one */
> > 
> > [...]
> >   
> >> +	case SOP_ENHANCED_2: {
> >> +		static const char * const prot_str[] = {  
> > 
> > static const char * const prot_str[PROT_CODE_SIZE] = {
> > 
> > so you have the guarantee that this has the right size, and you will
> > get a compile error if a new value is added to the enum but not here  
> 
> Will I? It would just initialize missing elements with NULL, no?

hmm makes sense, somehow I was convinced you would at least get a
warning, probably a case of -ENOCOFFEE

in any case, if you add the "SIZE" element at the end (and especially
if you also move the array right after the enum) there should be no
issues to keep the two in sync.

even better, you can put a
_Static_assert(ARRAY_SIZE(prot_str) == PROT_CODE_SIZE);

> > 
> > and at this point I think it might make more sense to move this right
> > after the enum itself
> >   
> >> +			"KEY or LAP",
> >> +			"DAT",
> >> +			"KEY",
> >> +			"ACC",
> >> +			"LAP",
> >> +			"IEP",
> >> +		};
> >> +		int prot_code = teid_esop2_prot_code(teid);  
> > 
> > enum prot_code prot_code = teid_esop2_prot_code(teid)>   
> >>  
> >> -	if (prot_is_datp(teid)) {
> >> -		printf("Type: DAT\n");
> >> -		return;
> >> +		assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));  
> > 
> > then you can remove this assert ^
> >   
> >> +		printf("Type: %s\n", prot_str[prot_code]);
> >> +		}
> >>  	}
> >>  }
> >>    
> [...]

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janosch Frank]

On 6/8/22 15:33, Janis Schoetterl-Glausch wrote:
> The translation-exception identification (TEID) contains information to
> identify the cause of certain program exceptions, including translation
> exceptions occurring during dynamic address translation, as well as
> protection exceptions.
> The meaning of fields in the TEID is complex, depending on the exception
> occurring and various potentially installed facilities.
> 
> Rework the type describing the TEID, in order to ease decoding.
> Change the existing code interpreting the TEID and extend it to take the
> installed suppression-on-protection facility into account.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
>   lib/s390x/fault.h         | 30 +++++-------------
>   lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
>   lib/s390x/interrupt.c     |  2 +-
>   s390x/edat.c              | 26 ++++++++++------
>   5 files changed, 115 insertions(+), 69 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index d9ab0bd7..3ca6bf76 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -20,23 +20,56 @@
>   
>   union teid {
>   	unsigned long val;
> -	struct {
> -		unsigned long addr:52;
> -		unsigned long fetch:1;
> -		unsigned long store:1;
> -		unsigned long reserved:6;
> -		unsigned long acc_list_prot:1;
> -		/*
> -		 * depending on the exception and the installed facilities,
> -		 * the m field can indicate several different things,
> -		 * including whether the exception was triggered by a MVPG
> -		 * instruction, or whether the addr field is meaningful
> -		 */
> -		unsigned long m:1;
> -		unsigned long asce_id:2;
> +	union {
> +		/* common fields DAT exc & protection exc */
> +		struct {
> +			uint64_t addr			: 52 -  0;
> +			uint64_t acc_exc_f_s		: 54 - 52;
> +			uint64_t side_effect_acc	: 55 - 54;
> +			uint64_t /* reserved */		: 62 - 55;
> +			uint64_t asce_id		: 64 - 62;
> +		};
> +		/* DAT exc */
> +		struct {
> +			uint64_t /* pad */		: 61 -  0;
> +			uint64_t dat_move_page		: 62 - 61;
> +		};
> +		/* suppression on protection */
> +		struct {
> +			uint64_t /* pad */		: 60 -  0;
> +			uint64_t sop_acc_list		: 61 - 60;
> +			uint64_t sop_teid_predictable	: 62 - 61;
> +		};
> +		/* enhanced suppression on protection 2 */
> +		struct {
> +			uint64_t /* pad */		: 56 -  0;
> +			uint64_t esop2_prot_code_0	: 57 - 56;
> +			uint64_t /* pad */		: 60 - 57;
> +			uint64_t esop2_prot_code_1	: 61 - 60;
> +			uint64_t esop2_prot_code_2	: 62 - 61;
> +		};

Quite messy, would it be more readable to unionize the fields that overlap?

>   	};
>   };
>   
> +enum prot_code {
> +	PROT_KEY_LAP,

That's key OR LAP, right?

> +	PROT_DAT,
> +	PROT_KEY,
> +	PROT_ACC_LIST,
> +	PROT_LAP,
> +	PROT_IEP,
> +};
> +

Yes, I like that more than my quick fixes :-)

> +static void print_decode_pgm_prot(union teid teid, bool dat)
> +{
> +	switch (get_supp_on_prot_facility()) {
> +	case SOP_NONE:
> +		printf("Type: ?\n");
> +		break;
> +	case SOP_BASIC:
> +		if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
> +			printf("Type: ACC\n");
> +		else
> +			printf("Type: ?\n");
> +		break;

I'm wondering if we should cut off the two possibilities above to make 
it a bit more sane. The SOP facility is about my age now and ESOP1 has 
been introduced with z10 if I'm not mistaken so it's not young either.

Do we have tests that require SOP/no-SOP?

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch]

On 6/10/22 11:31, Janosch Frank wrote:
> On 6/8/22 15:33, Janis Schoetterl-Glausch wrote:
>> The translation-exception identification (TEID) contains information to
>> identify the cause of certain program exceptions, including translation
>> exceptions occurring during dynamic address translation, as well as
>> protection exceptions.
>> The meaning of fields in the TEID is complex, depending on the exception
>> occurring and various potentially installed facilities.
>>
>> Rework the type describing the TEID, in order to ease decoding.
>> Change the existing code interpreting the TEID and extend it to take the
>> installed suppression-on-protection facility into account.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
>>   lib/s390x/fault.h         | 30 +++++-------------
>>   lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
>>   lib/s390x/interrupt.c     |  2 +-
>>   s390x/edat.c              | 26 ++++++++++------
>>   5 files changed, 115 insertions(+), 69 deletions(-)
>>
>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>> index d9ab0bd7..3ca6bf76 100644
>> --- a/lib/s390x/asm/interrupt.h
>> +++ b/lib/s390x/asm/interrupt.h
>> @@ -20,23 +20,56 @@
>>     union teid {
>>       unsigned long val;
>> -    struct {
>> -        unsigned long addr:52;
>> -        unsigned long fetch:1;
>> -        unsigned long store:1;
>> -        unsigned long reserved:6;
>> -        unsigned long acc_list_prot:1;
>> -        /*
>> -         * depending on the exception and the installed facilities,
>> -         * the m field can indicate several different things,
>> -         * including whether the exception was triggered by a MVPG
>> -         * instruction, or whether the addr field is meaningful
>> -         */
>> -        unsigned long m:1;
>> -        unsigned long asce_id:2;
>> +    union {
>> +        /* common fields DAT exc & protection exc */
>> +        struct {
>> +            uint64_t addr            : 52 -  0;
>> +            uint64_t acc_exc_f_s        : 54 - 52;
>> +            uint64_t side_effect_acc    : 55 - 54;
>> +            uint64_t /* reserved */        : 62 - 55;
>> +            uint64_t asce_id        : 64 - 62;
>> +        };
>> +        /* DAT exc */
>> +        struct {
>> +            uint64_t /* pad */        : 61 -  0;
>> +            uint64_t dat_move_page        : 62 - 61;
>> +        };
>> +        /* suppression on protection */
>> +        struct {
>> +            uint64_t /* pad */        : 60 -  0;
>> +            uint64_t sop_acc_list        : 61 - 60;
>> +            uint64_t sop_teid_predictable    : 62 - 61;
>> +        };
>> +        /* enhanced suppression on protection 2 */
>> +        struct {
>> +            uint64_t /* pad */        : 56 -  0;
>> +            uint64_t esop2_prot_code_0    : 57 - 56;
>> +            uint64_t /* pad */        : 60 - 57;
>> +            uint64_t esop2_prot_code_1    : 61 - 60;
>> +            uint64_t esop2_prot_code_2    : 62 - 61;
>> +        };
> 
> Quite messy, would it be more readable to unionize the fields that overlap?

Not sure, I prefer this because it reflects the structure of the PoP,
where there is a section for DAT exceptions, SOP, ESOP1, ESOP2.
It's not exactly like this in the code because I factored out common fields,
and I removed the struct for ESOP1 because it was mostly redundant with SOP.
> 
>>       };
>>   };
>>   +enum prot_code {
>> +    PROT_KEY_LAP,
> 
> That's key OR LAP, right?

Yes, do you want me to make that explicit?
> 
>> +    PROT_DAT,
>> +    PROT_KEY,
>> +    PROT_ACC_LIST,
>> +    PROT_LAP,
>> +    PROT_IEP,
>> +};
>> +
> 
> Yes, I like that more than my quick fixes :-)
> 
>> +static void print_decode_pgm_prot(union teid teid, bool dat)
>> +{
>> +    switch (get_supp_on_prot_facility()) {
>> +    case SOP_NONE:
>> +        printf("Type: ?\n");
>> +        break;
>> +    case SOP_BASIC:
>> +        if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
>> +            printf("Type: ACC\n");
>> +        else
>> +            printf("Type: ?\n");
>> +        break;
> 
> I'm wondering if we should cut off the two possibilities above to make it a bit more sane. The SOP facility is about my age now and ESOP1 has been introduced with z10 if I'm not mistaken so it's not young either.

So

case SOP_NONE:
case SOP_BASIC:
	assert(false);

?
	
> 
> Do we have tests that require SOP/no-SOP?

No, just going for correctness.

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janosch Frank]

On 6/10/22 12:37, Janis Schoetterl-Glausch wrote:
> On 6/10/22 11:31, Janosch Frank wrote:
>> On 6/8/22 15:33, Janis Schoetterl-Glausch wrote:
>>> The translation-exception identification (TEID) contains information to
>>> identify the cause of certain program exceptions, including translation
>>> exceptions occurring during dynamic address translation, as well as
>>> protection exceptions.
>>> The meaning of fields in the TEID is complex, depending on the exception
>>> occurring and various potentially installed facilities.
>>>
>>> Rework the type describing the TEID, in order to ease decoding.
>>> Change the existing code interpreting the TEID and extend it to take the
>>> installed suppression-on-protection facility into account.
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>> ---
>>>    lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
>>>    lib/s390x/fault.h         | 30 +++++-------------
>>>    lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
>>>    lib/s390x/interrupt.c     |  2 +-
>>>    s390x/edat.c              | 26 ++++++++++------
>>>    5 files changed, 115 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>>> index d9ab0bd7..3ca6bf76 100644
>>> --- a/lib/s390x/asm/interrupt.h
>>> +++ b/lib/s390x/asm/interrupt.h
>>> @@ -20,23 +20,56 @@
>>>      union teid {
>>>        unsigned long val;
>>> -    struct {
>>> -        unsigned long addr:52;
>>> -        unsigned long fetch:1;
>>> -        unsigned long store:1;
>>> -        unsigned long reserved:6;
>>> -        unsigned long acc_list_prot:1;
>>> -        /*
>>> -         * depending on the exception and the installed facilities,
>>> -         * the m field can indicate several different things,
>>> -         * including whether the exception was triggered by a MVPG
>>> -         * instruction, or whether the addr field is meaningful
>>> -         */
>>> -        unsigned long m:1;
>>> -        unsigned long asce_id:2;
>>> +    union {
>>> +        /* common fields DAT exc & protection exc */
>>> +        struct {
>>> +            uint64_t addr            : 52 -  0;
>>> +            uint64_t acc_exc_f_s        : 54 - 52;

I'd either name it acc_exc_fs or spell it out properly.

>>> +            uint64_t side_effect_acc    : 55 - 54;
>>> +            uint64_t /* reserved */        : 62 - 55;
>>> +            uint64_t asce_id        : 64 - 62;
>>> +        };
>>> +        /* DAT exc */
>>> +        struct {
>>> +            uint64_t /* pad */        : 61 -  0;
>>> +            uint64_t dat_move_page        : 62 - 61;
>>> +        };
>>> +        /* suppression on protection */
>>> +        struct {
>>> +            uint64_t /* pad */        : 60 -  0;
>>> +            uint64_t sop_acc_list        : 61 - 60;
>>> +            uint64_t sop_teid_predictable    : 62 - 61;
>>> +        };
>>> +        /* enhanced suppression on protection 2 */
>>> +        struct {
>>> +            uint64_t /* pad */        : 56 -  0;
>>> +            uint64_t esop2_prot_code_0    : 57 - 56;
>>> +            uint64_t /* pad */        : 60 - 57;
>>> +            uint64_t esop2_prot_code_1    : 61 - 60;
>>> +            uint64_t esop2_prot_code_2    : 62 - 61;
>>> +        };
>>
>> Quite messy, would it be more readable to unionize the fields that overlap?
> 
> Not sure, I prefer this because it reflects the structure of the PoP,
> where there is a section for DAT exceptions, SOP, ESOP1, ESOP2.
> It's not exactly like this in the code because I factored out common fields,
> and I removed the struct for ESOP1 because it was mostly redundant with SOP.

Well, the rest of the code is readable and I can compare the struct to 
the POP so I'm okish with this.

>>
>>>        };
>>>    };
>>>    +enum prot_code {
>>> +    PROT_KEY_LAP,
>>
>> That's key OR LAP, right?
> 
> Yes, do you want me to make that explicit?

Yes

>>
>>> +    PROT_DAT,
>>> +    PROT_KEY,
>>> +    PROT_ACC_LIST,
>>> +    PROT_LAP,
>>> +    PROT_IEP,
>>> +};
>>> +
>>
>> Yes, I like that more than my quick fixes :-)
>>
>>> +static void print_decode_pgm_prot(union teid teid, bool dat)
>>> +{
>>> +    switch (get_supp_on_prot_facility()) {
>>> +    case SOP_NONE:
>>> +        printf("Type: ?\n");
>>> +        break;
>>> +    case SOP_BASIC:
>>> +        if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
>>> +            printf("Type: ACC\n");
>>> +        else
>>> +            printf("Type: ?\n");
>>> +        break;
>>
>> I'm wondering if we should cut off the two possibilities above to make it a bit more sane. The SOP facility is about my age now and ESOP1 has been introduced with z10 if I'm not mistaken so it's not young either.
> 
> So
> 
> case SOP_NONE:
> case SOP_BASIC:
> 	assert(false);
> 
> ?

I'd check (e)sop on initialization and abort early so we never need to 
worry about it in other files.

> 	
>>
>> Do we have tests that require SOP/no-SOP?
> 
> No, just going for correctness.
> 

Re: [kvm-unit-tests PATCH v2 3/3] s390x: Rework TEID decoding and usage

[Janis Schoetterl-Glausch]

On Fri, 2022-06-10 at 14:10 +0200, Janosch Frank wrote:
> On 6/10/22 12:37, Janis Schoetterl-Glausch wrote:
> > On 6/10/22 11:31, Janosch Frank wrote:
> > > On 6/8/22 15:33, Janis Schoetterl-Glausch wrote:
> > > > The translation-exception identification (TEID) contains information to
> > > > identify the cause of certain program exceptions, including translation
> > > > exceptions occurring during dynamic address translation, as well as
> > > > protection exceptions.
> > > > The meaning of fields in the TEID is complex, depending on the exception
> > > > occurring and various potentially installed facilities.
> > > > 
> > > > Rework the type describing the TEID, in order to ease decoding.
> > > > Change the existing code interpreting the TEID and extend it to take the
> > > > installed suppression-on-protection facility into account.
> > > > 
> > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > > > ---
> > > >    lib/s390x/asm/interrupt.h | 61 +++++++++++++++++++++++++++---------
> > > >    lib/s390x/fault.h         | 30 +++++-------------
> > > >    lib/s390x/fault.c         | 65 ++++++++++++++++++++++++++-------------
> > > >    lib/s390x/interrupt.c     |  2 +-
> > > >    s390x/edat.c              | 26 ++++++++++------
> > > >    5 files changed, 115 insertions(+), 69 deletions(-)
> > > 

[...]

> > > > +static void print_decode_pgm_prot(union teid teid, bool dat)
> > > > +{
> > > > +    switch (get_supp_on_prot_facility()) {
> > > > +    case SOP_NONE:
> > > > +        printf("Type: ?\n");
> > > > +        break;
> > > > +    case SOP_BASIC:
> > > > +        if (teid.sop_teid_predictable && dat && teid.sop_acc_list)
> > > > +            printf("Type: ACC\n");
> > > > +        else
> > > > +            printf("Type: ?\n");
> > > > +        break;
> > > 
> > > I'm wondering if we should cut off the two possibilities above to make it a bit more sane. The SOP facility is about my age now and ESOP1 has been introduced with z10 if I'm not mistaken so it's not young either.
> > 
> > So
> > 
> > case SOP_NONE:
> > case SOP_BASIC:
> > 	assert(false);
> > 
> > ?
> 
> I'd check (e)sop on initialization and abort early so we never need to 
> worry about it in other files.

We could just ignore those cases since we don't depend on them for the
tests to function. Breaking all tests seems disproportional to me.
> 
> > 	
> > > 
> > > Do we have tests that require SOP/no-SOP?
> > 
> > No, just going for correctness.
> > 
>