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

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

On Fri, 3 Jun 2022 15:49:33 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

[...]

> >   
> >> +			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 1 */
> >> +		struct {
> >> +			uint64_t /* pad */		: 61 -  0;  
> > 
> > 60 - 0
> >   
> >> +			uint64_t esop1_acc_list_or_dat	: 62 - 61;  
> > 
> > 61 - 60
> > 
> > and then:
> > 
> > uint64_t esop1_teid_predictable : 62 - 61;
> >   
> Ah, no, but I see how it is confusing.
> If bit 61 is one then the exception is due to access list or DAT.

ok

> That's why its called acc_list_or_dat.
> If it is zero it's due to low address or key and the rest of the TEID
> is unpredictable. So this is an alias of sop_teid_predictable.

ok, but then you need a definition for bit 60, which tells whether it
is DAT or ACL. (but see below)

> 
> >> +		};
> >> +		/* 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,  
> > 
> > I would still also define two PROT_INVALID or PROT_RESERVED
> > 
> > just to avoid surprises
> >   
> I guess the values are reserved, but maybe an assert would be better?

ok

> Then we'd be notified to fix the test.
> 

[...]

> >> @@ -65,10 +93,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);  
> > 
> > teid.addr << PAGE_SHIFT ?  
> 
> I got compiler warnings because teid.addr is 52 bit.

oufff... ok forget it then, keep it as it is

> >   
> >>  }
> >> 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..af442039 100644
> >> --- a/s390x/edat.c
> >> +++ b/s390x/edat.c
> >> @@ -37,14 +37,20 @@ 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;  
> >   
> This function is mostly correct, except it's missing
> break; statements (so not correct at all :)).
> 
> > add:
> > 
> > if (teid.sop_acc_list)
> > 	return false;
> >   
> Will be taken care of by the return statement at the very bottom.
> 
> >> +	case SOP_ENHANCED_1:  
> > 
> > you need to handle the unpredictable case here too
> >   
> >> +		if (!teid.esop1_acc_list_or_dat)
> >> +			return false;  
> >
> > so you return false the it is DAT... but if it is not DAT, it's
> > access-control-list... 
> >   
> So this makes sense if instead you think about bit 61.
> It also shows the rational for the variable name if you read it as
> "if the exception was not due to either access list or DAT", so we
> return false in case we know it was not DAT.

ahh I see.

at this point I think it would be better to simply remove that bit, and
only use sop_teid_predictable and sop_acc_list

> 
> > you might want to replace this whole case with:
> > 
> > return !teid.esop1_teid_predictable;
> > 
> > (although I don't understand why you want to exclude DAT here)
> >   
> >> +	case SOP_ENHANCED_2:
> >> +		if (teid_esop2_prot_code(teid) != 1)  
> > 
> > why not using the PROT_DAT enum?  
> 
> Just forgot.
> 
> > also, handle the PROT_ACC_LIST too
> > 
> > also, add:
> > 
> > if (PROT_KEY_LAP)
> > 	return true;  
> 
> Am I misunderstanding the edat test? We're expecting nothing but
> DAT protection exceptions, no? So everything else is a test failure.

then return false

currently you would not handle that correctly, I think

> > 
> > because in that case you don't have the address part.
> > 
> > 
> > 
> > but at this point I wonder if you can't just rewrite this function with
> > an additional enum prot_code parameter, to specify the exact type of
> > exception you're expecting  
> 
> Maybe, but I don't think it's worth it. The logic is complicated and I'd

fair enough

> prefer to keep it as simple as possible and keeping it specific to the test
> helps with that, instead of generalizing it to all possibilities.
> >   
> >> +			return false;
> >> +	}
> >> +	return (!teid.sop_acc_list && !teid.asce_id &&
> >>  		(teid.addr == ((unsigned long)ptr >> PAGE_SHIFT)));
> >>  }
> >>    
> >   
>