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

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

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]);
>> +		}
>>  	}
>>  }
>>  
[...]