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

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

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)));
>  }
>