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

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 20 May 2022 21:08:50 +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 | 66 ++++++++++++++++++++++++++--------
>  lib/s390x/fault.h         | 30 ++++------------
>  lib/s390x/fault.c         | 74 +++++++++++++++++++++++++++------------
>  lib/s390x/interrupt.c     |  2 +-
>  s390x/edat.c              | 20 +++++++----
>  5 files changed, 124 insertions(+), 68 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index d9ab0bd7..8d5bfbf9 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -20,23 +20,61 @@
>  
>  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 */		: 55 - 54;

shouldn't this ^ be 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 1 */
> +		struct {
> +			uint64_t /* pad */		: 61 -  0;

60 - 0

> +			uint64_t esop1_acc_list_or_dat	: 62 - 61;

61 - 60

and then:

uint64_t esop1_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,

I would still also define two PROT_INVALID or PROT_RESERVED

just to avoid surprises

> +};
> +
> +static inline enum prot_code teid_esop2_prot_code(union teid teid)
> +{
> +	int code = 0;
> +
> +	code = code << 1 | teid.esop2_prot_code_0;
> +	code = code << 1 | teid.esop2_prot_code_1;
> +	code = code << 1 | teid.esop2_prot_code_2;
> +	return (enum prot_code)code;

return (enum prot_code)(teid.esop2_prot_code_0 << 2 |
			teid.esop2_prot_code_1 << 1 |
			teid.esop2_prot_code_2);

> +}
> +
>  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..02b3c098 100644
> --- a/lib/s390x/fault.c
> +++ b/lib/s390x/fault.c
> @@ -13,35 +13,63 @@
>  #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;
> -	}
>  
> -	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()) {
> +	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.esop1_acc_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:
> +		switch (teid_esop2_prot_code(teid)) {

I wonder if it weren't easier to do

static const char * const prot_strings[6] = {"KEY or LAP", "DAT", ...};
printf("Type: %s\n", prot_strings[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;
> +		}
>  	}
>  }
>  
> -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 +85,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 +93,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);

teid.addr << PAGE_SHIFT ?

>  }
> 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..af442039 100644
> --- a/s390x/edat.c
> +++ b/s390x/edat.c
> @@ -37,14 +37,20 @@ 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;

add:

if (teid.sop_acc_list)
	return false;

> +	case SOP_ENHANCED_1:

you need to handle the unpredictable case here too

> +		if (!teid.esop1_acc_list_or_dat)
> +			return false;

so you return false the it is DAT... but if it is not DAT, it's
access-control-list... 

you might want to replace this whole case with:

return !teid.esop1_teid_predictable;

(although I don't understand why you want to exclude DAT here)

> +	case SOP_ENHANCED_2:
> +		if (teid_esop2_prot_code(teid) != 1)

why not using the PROT_DAT enum?
also, handle the PROT_ACC_LIST too

also, add:

if (PROT_KEY_LAP)
	return true;

because in that case you don't have the address part.



but at this point I wonder if you can't just rewrite this function with
an additional enum prot_code parameter, to specify the exact type of
exception you're expecting

> +			return false;
> +	}
> +	return (!teid.sop_acc_list && !teid.asce_id &&
>  		(teid.addr == ((unsigned long)ptr >> PAGE_SHIFT)));
>  }
>