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