Re: [kvm-unit-tests PATCH v2 2/4] s390x: Test TEID values in storage key test

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

On Tue, 17 May 2022 17:11:37 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On Tue, 2022-05-17 at 15:46 +0200, Claudio Imbrenda wrote:
> > On Tue, 17 May 2022 13:56:05 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> > > On a protection exception, test that the Translation-Exception
> > > Identification (TEID) values are correct given the circumstances of the
> > > particular test.
> > > The meaning of the TEID values is dependent on the installed
> > > suppression-on-protection facility.
> > > 
> > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > > ---
> > >  lib/s390x/asm/facility.h | 21 ++++++++++++++
> > >  lib/s390x/sclp.h         |  4 +++
> > >  lib/s390x/sclp.c         |  2 ++
> > >  s390x/skey.c             | 60 ++++++++++++++++++++++++++++++++++++----
> > >  4 files changed, 81 insertions(+), 6 deletions(-)
> > >   
> [...]
> 
> > > +static void check_key_prot_exc(enum access access, enum protection prot)
> > > +{
> > > +	struct lowcore *lc = 0;
> > > +	union teid teid;
> > > +
> > > +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> > > +	report_prefix_push("TEID");
> > > +	teid.val = lc->trans_exc_id;
> > > +	switch (get_supp_on_prot_facility()) {
> > > +	case SOP_NONE:
> > > +	case SOP_BASIC:
> > > +		break;
> > > +	case SOP_ENHANCED_1:
> > > +		if ((teid.val & (BIT(63 - 61))) == 0)  
> > 
> > can you at least replace the hardcoded values with a macro or a const
> > variable?  
> 
> I'll see if maybe I can come up with a nice way to extend the teid, but
> I'll use a const if not.
> > 
> > like:
> > 
> > 	const unsigned long esop_bit = BIT(63 - 61);
> > 
> > 	...
> > 
> > 		if (!(teid.val & esop_bit))
> >   
> > > +			report_pass("key-controlled protection");  
> > 
> > actually, now that I think of it, aren't we expecting the bit to be
> > zero? should that not be like this?
> > 
> > report (!(teid.val & esop_bit), ...);  
> 
> Indeed.
> >   
> > > +		break;
> > > +	case SOP_ENHANCED_2:
> > > +		if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {  
> > 
> > const unsigned long esop2_bits = 0x8C;	/* bits 56, 60, and 61 */
> > const unsigned long esop2_key_prot = BIT(63 - 60);
> > 
> > if ((teid.val & esop2_bits) == 0) {
> > 	report_pass(...);
> >   
> > > +			report_pass("key-controlled protection");
> > > +			if (teid.val & BIT(63 - 60)) {  
> > 
> > } else if ((teid.val & esop2_bits) == esop_key_prot) {  
> 
> 010 binary also means key protection, so we should pass that test here,
> too. The access code checking is an additional test, IMO.

yes, my idea was to pass it only once when checking the fetch/store
indication (if any)

> >   
> > > +				int access_code = teid.fetch << 1 | teid.store;
> > > +
> > > +				if (access_code == 2)
> > > +					report((access & 2) && (prot & 2),
> > > +					       "exception due to fetch");
> > > +				if (access_code == 1)
> > > +					report((access & 1) && (prot & 1),
> > > +					       "exception due to store");
> > > +				/* no relevant information if code is 0 or 3 */  
> > 
> > here you should check for the access-exception-fetch/store-indi-
> > cation facility, then you can check the access code  
> 
> Oh, yes. By the way, can we get rid of magic numbers for facility
> checking? Just defining an enum in lib/asm/facility.h and doing
> test_facility(FCLTY_ACCESS_EXC_FETCH_STORE_INDICATION) would be an
> improvement.

not sure, that name looks very ugly

although that would definitely make future review easier, as you say

but that would be something for another patchseries :)

> Well, I guess you'd end up with quite horribly long names, but at least
> you have to review the values only once and not for every patch that
> tests a facility. 
> > 
> > and at this point you should check for 0 explicitly (always pass) and 3
> > (always fail)  
> 
> I'm fine with passing 0, but I'm not so sure about 3.
> The value is reserved, so the correct thing to do is to not attribute
> *any* meaning to it. But kvm currently really should not set it either.

fine

> > > +			}
> > > +		}  
> > 
> > } else {
> > 	/* not key protection */
> > 	report_fail(...);
> > }  
> > > +		break;
> > > +	}
> > > +	report_prefix_pop();
> > > +}
> > > +  
> [...]
>