Re: [kvm-unit-tests PATCH 3/8] lib: s390x: Print addressing related exception information

[Janosch Frank <frankja@linux.ibm.com>]

On 8/18/21 11:12 AM, Thomas Huth wrote:
> On 13/08/2021 09.36, Janosch Frank wrote:
>> Right now we only get told the kind of program exception as well as
>> the PSW at the point where it happened.
>>
>> For addressing exceptions the PSW is not always enough so let's print
>> the TEID which contains the failing address and flags that tell us
>> more about the kind of address exception.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h |  4 +++
>>   lib/s390x/interrupt.c    | 72 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 4ca02c1d..39c5ba99 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -41,6 +41,10 @@ struct psw {
>>   	uint64_t	addr;
>>   };
>>   
>> +/* Let's ignore spaces we don't expect to use for now. */
>> +#define AS_PRIM				0
>> +#define AS_HOME				3
>> +
>>   #define PSW_MASK_EXT			0x0100000000000000UL
>>   #define PSW_MASK_IO			0x0200000000000000UL
>>   #define PSW_MASK_DAT			0x0400000000000000UL
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 01ded49d..1248bceb 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -12,6 +12,7 @@
>>   #include <sclp.h>
>>   #include <interrupt.h>
>>   #include <sie.h>
>> +#include <asm/page.h>
>>   
>>   static bool pgm_int_expected;
>>   static bool ext_int_expected;
>> @@ -126,6 +127,73 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>>   	/* suppressed/terminated/completed point already at the next address */
>>   }
>>   
>> +static void decode_pgm_prot(uint64_t teid)
>> +{
>> +	/* Low-address protection exception, 100 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid)) {
> 
> Likely just a matter of taste, but I'd prefer something like:
> 
> 	if ((teid & 0x8c) == 0x80) {

The POP states these as bits when you have a look at the ESOP section
and I'd like to keep it the same here for easier comparison.

The test_bits() are as explicit as it gets and I value that.

> 
>> +		printf("Type: LAP\n");
>> +		return;
>> +	}
>> +
>> +	/* Instruction execution prevention, i.e. no-execute, 101 */
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
>> +		printf("Type: IEP\n");
>> +		return;
>> +	}
>> +
>> +	/* Standard DAT exception, 001 */
>> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid)) {
>> +		printf("Type: DAT\n");
>> +		return;
>> +	}
> 
> What about 010 (key controlled protection) and 011 (access-list controlled 
> protection)? Even if we do not trigger those yet, it might make sense to add 
> them right from the start, too?

If I do that then I can start a whole new file "fault.c" and move these
changes there (which I'll do now anyway). My intentions were a small
change that covers 90% of our current exceptions (especially PV
exceptions) to make my life easier in LPAR.

If people add skey/ar code they can also add the decoding here, no? :-)

> 
>> +}
>> +
>> +static void decode_teid(uint64_t teid)
>> +{
>> +	int asce_id = lc->trans_exc_id & 3;
> 
> Why are you referencing the lc->trans_exc_id here again? It's already passed 
> as "teid" parameter.

Forgot to remove that

> 
>> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
>> +
>> +	printf("Memory exception information:\n");
>> +	printf("TEID: %lx\n", teid);
>> +	printf("DAT: %s\n", dat ? "on" : "off");
>> +	printf("AS: %s\n", asce_id == AS_PRIM ? "Primary" : "Home");
> 
> Could "secondary" or "AR" mode really never happen here? I'd rather like to 
> see a switch-case statement here that is able to print all four modes, just 
> to avoid confusion.

Right now we ONLY use primary space.

> 
>> +	if (lc->pgm_int_code == PGM_INT_CODE_PROTECTION)
>> +		decode_pgm_prot(teid);
>> +
>> +	/*
>> +	 * If teid bit 61 is off for these two exception the reported
>> +	 * address is unpredictable.
>> +	 */
>> +	if ((lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_ACCESS ||
>> +	     lc->pgm_int_code == PGM_INT_CODE_SECURE_STOR_VIOLATION) &&
>> +	    !test_bit_inv(61, &teid)) {
>> +		printf("Address: %lx, unpredictable\n ", teid & PAGE_MASK);
>> +		return;
>> +	}
>> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
>> +}
>> +
>> +static void print_storage_exception_information(void)
>> +{
>> +	switch (lc->pgm_int_code) {
>> +	case PGM_INT_CODE_PROTECTION:
>> +	case PGM_INT_CODE_PAGE_TRANSLATION:
>> +	case PGM_INT_CODE_SEGMENT_TRANSLATION:
>> +	case PGM_INT_CODE_ASCE_TYPE:
>> +	case PGM_INT_CODE_REGION_FIRST_TRANS:
>> +	case PGM_INT_CODE_REGION_SECOND_TRANS:
>> +	case PGM_INT_CODE_REGION_THIRD_TRANS:
>> +	case PGM_INT_CODE_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_NON_SECURE_STOR_ACCESS:
>> +	case PGM_INT_CODE_SECURE_STOR_VIOLATION:
>> +		decode_teid(lc->trans_exc_id);
>> +		break;
>> +	default:
>> +		return;
> 
> I think you could drop that default case.

Yes

> 
>> +	}
>> +}
>> +
>>   static void print_int_regs(struct stack_frame_int *stack)
>>   {
>>   	printf("\n");
>> @@ -155,6 +223,10 @@ static void print_pgm_info(struct stack_frame_int *stack)
>>   	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr, lc->pgm_int_id);
>>   	print_int_regs(stack);
>>   	dump_stack();
>> +
>> +	/* Dump stack doesn't end with a \n so we add it here instead */
>> +	printf("\n");
>> +	print_storage_exception_information();
>>   	report_summary();
>>   	abort();
>>   }
>>
>