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

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

On Tue, 21 Jun 2022 16:30:12 +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.

thanks, queued

> 
> 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.
> 
> v2 -> v3
>  * rename some identifiers
>  * implement Claudio's feedback to assert that the array for printing
>    has the correct number of elements. I kept the array inside the
>    printing function, tho, because I consider its content an implementation
>    detail
>  * ignore ancient machines without at least ESOP-1
> 
> 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 | 62 ++++++++++++++++++++++++++++++---------
>  lib/s390x/fault.h         | 30 +++++--------------
>  lib/s390x/sclp.h          | 18 +++++++-----
>  lib/s390x/fault.c         | 58 +++++++++++++++++++++++-------------
>  lib/s390x/interrupt.c     |  2 +-
>  lib/s390x/sclp.c          |  2 ++
>  s390x/edat.c              | 25 +++++++++-------
>  8 files changed, 142 insertions(+), 76 deletions(-)
> 
> Range-diff against v2:
> 1:  6427944a = 1:  189e03ca s390x: Fix sclp facility bit numbers
> 2:  a08fce3b = 2:  0783c2a4 s390x: lib: SOP facility query function
> 3:  eb268af1 ! 3:  615ec8df s390x: Rework TEID decoding and usage
>     @@ lib/s390x/asm/interrupt.h
>      +		/* common fields DAT exc & protection exc */
>      +		struct {
>      +			uint64_t addr			: 52 -  0;
>     -+			uint64_t acc_exc_f_s		: 54 - 52;
>     ++			uint64_t acc_exc_fetch_store	: 54 - 52;
>      +			uint64_t side_effect_acc	: 55 - 54;
>      +			uint64_t /* reserved */		: 62 - 55;
>      +			uint64_t asce_id		: 64 - 62;
>     @@ lib/s390x/asm/interrupt.h
>       };
>       
>      +enum prot_code {
>     -+	PROT_KEY_LAP,
>     ++	PROT_KEY_OR_LAP,
>      +	PROT_DAT,
>      +	PROT_KEY,
>      +	PROT_ACC_LIST,
>      +	PROT_LAP,
>      +	PROT_IEP,
>     ++	PROT_NUM_CODES /* Must always be last */
>      +};
>      +
>      +static inline enum prot_code teid_esop2_prot_code(union teid teid)
>     @@ lib/s390x/asm/interrupt.h
>      +		    teid.esop2_prot_code_1 << 1 |
>      +		    teid.esop2_prot_code_2);
>      +
>     -+	assert(code < 6);
>     ++	assert(code < PROT_NUM_CODES);
>      +	return (enum prot_code)code;
>      +}
>      +
>     @@ lib/s390x/fault.c
>      -		printf("Type: IEP\n");
>      -		return;
>      -	}
>     -+static void print_decode_pgm_prot(union teid teid, bool dat)
>     ++static void print_decode_pgm_prot(union teid teid)
>      +{
>      +	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");
>     ++		printf("Type: ?\n"); /* modern/relevant machines have ESOP */
>      +		break;
>      +	case SOP_ENHANCED_1:
>      +		if (teid.sop_teid_predictable) {/* implies access list or DAT */
>     @@ lib/s390x/fault.c
>      +			"LAP",
>      +			"IEP",
>      +		};
>     ++		_Static_assert(ARRAY_SIZE(prot_str) == PROT_NUM_CODES);
>      +		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/fault.c
>       	case AS_PRIM:
>       		printf("Primary\n");
>       		break;
>     -@@ lib/s390x/fault.c: 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
>      @@ lib/s390x/fault.c: void print_decode_teid(uint64_t teid)
>       	 */
>       	if ((lowcore.pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
>     @@ s390x/edat.c: static bool check_pgm_prot(void *ptr)
>      -	 * field might or might not be meaningful when the m field is 0.
>      -	 */
>      -	if (!teid.m)
>     +-		return true;
>     +-	return (!teid.acc_list_prot && !teid.asce_id &&
>      +	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;
>     ++		assert(false); /* let's ignore ancient/irrelevant machines */
>      +	case SOP_ENHANCED_1:
>      +		if (!teid.sop_teid_predictable) /* implies key or low addr */
>      +			return false;
> 
> base-commit: 610c15284a537484682adfb4b6d6313991ab954f