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

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

On 8/20/21 1:48 PM, Claudio Imbrenda wrote:
> On Fri, 20 Aug 2021 11:39:58 +0000
> Janosch Frank <frankja@linux.ibm.com> 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>
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks

> 
> (but see a small nit below)
> 
>> ---
>>  lib/s390x/asm/arch_def.h |  5 +++
>>  lib/s390x/fault.c        | 76 ++++++++++++++++++++++++++++++++++++++++
>>  lib/s390x/fault.h        | 44 +++++++++++++++++++++++
>>  lib/s390x/interrupt.c    | 29 +++++++++++++--
>>  s390x/Makefile           |  1 +
>>  5 files changed, 153 insertions(+), 2 deletions(-)
>>  create mode 100644 lib/s390x/fault.c
>>  create mode 100644 lib/s390x/fault.h
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 302ef1ff..ab5a9043 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -41,6 +41,11 @@ struct psw {
>>  	uint64_t	addr;
>>  };
>>  
>> +#define AS_PRIM				0
>> +#define AS_ACCR				1
>> +#define AS_SECN				2
>> +#define AS_HOME				3
>> +
>>  #define PSW_MASK_EXT			0x0100000000000000UL
>>  #define PSW_MASK_IO			0x0200000000000000UL
>>  #define PSW_MASK_DAT			0x0400000000000000UL
>> diff --git a/lib/s390x/fault.c b/lib/s390x/fault.c
>> new file mode 100644
>> index 00000000..fd1490e3
>> --- /dev/null
>> +++ b/lib/s390x/fault.c
>> @@ -0,0 +1,76 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Library to decode addressing related exceptions
>> + *
>> + * Copyright 2021 IBM Corp.
>> + *
>> + * Authors:
>> + *    Janosch Frank <frankja@linux.ibm.com>
>> + */
>> +#include <libcflat.h>
>> +#include <bitops.h>
>> +#include <asm/arch_def.h>
>> +#include <asm/page.h>
>> +#include <fault.h>
>> +
>> +static struct lowcore *lc = (struct lowcore *)0x0;
>> +
>> +/* Decodes the protection exceptions we'll most likely see */
>> +static void decode_pgm_prot(uint64_t teid)
> 
> I think this is also a "print_decode_pgm_prot", but it's static
> so I guess we don't really care too much

Can do, sounds reasonable

> 
>> +{
>> +	if (prot_is_lap(teid)) {
>> +		printf("Type: LAP\n");
>> +		return;
>> +	}
>> +
>> +	if (prot_is_iep(teid)) {
>> +		printf("Type: IEP\n");
>> +		return;
>> +	}
>> +
>> +	if (prot_is_datp(teid)) {
>> +		printf("Type: DAT\n");
>> +		return;
>> +	}
>> +}
>> +
>> +void print_decode_teid(uint64_t teid)
>> +{
>> +	int asce_id = teid & 3;
>> +	bool dat = lc->pgm_old_psw.mask & PSW_MASK_DAT;
>> +
>> +	printf("Memory exception information:\n");
>> +	printf("DAT: %s\n", dat ? "on" : "off");
>> +
>> +	printf("AS: ");
>> +	switch (asce_id) {
>> +	case AS_PRIM:
>> +		printf("Primary\n");
>> +		break;
>> +	case AS_ACCR:
>> +		printf("Access Register\n");
>> +		break;
>> +	case AS_SECN:
>> +		printf("Secondary\n");
>> +		break;
>> +	case AS_HOME:
>> +		printf("Home\n");
>> +		break;
>> +	}
>> +
>> +	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("TEID: %lx\n", teid);
>> +	printf("Address: %lx\n\n", teid & PAGE_MASK);
>> +}
>> diff --git a/lib/s390x/fault.h b/lib/s390x/fault.h
>> new file mode 100644
>> index 00000000..726da2f0
>> --- /dev/null
>> +++ b/lib/s390x/fault.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Headers for fault.c
>> + *
>> + * Copyright 2021 IBM Corp.
>> + *
>> + * Authors:
>> + *    Janosch Frank <frankja@linux.ibm.com>
>> + */
>> +#ifndef _S390X_FAULT_H_
>> +#define _S390X_FAULT_H_
>> +
>> +#include <bitops.h>
>> +
>> +/* Instruction execution prevention, i.e. no-execute, 101 */
>> +static inline bool prot_is_iep(uint64_t teid)
>> +{
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/* Standard DAT exception, 001 */
>> +static inline bool prot_is_datp(uint64_t teid)
>> +{
>> +	if (!test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && test_bit_inv(61, &teid))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/* Low-address protection exception, 100 */
>> +static inline bool prot_is_lap(uint64_t teid)
>> +{
>> +	if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && !test_bit_inv(61, &teid))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +void print_decode_teid(uint64_t teid);
>> +
>> +#endif /* _S390X_FAULT_H_ */
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index 01ded49d..721e6758 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -12,6 +12,8 @@
>>  #include <sclp.h>
>>  #include <interrupt.h>
>>  #include <sie.h>
>> +#include <fault.h>
>> +#include <asm/page.h>
>>  
>>  static bool pgm_int_expected;
>>  static bool ext_int_expected;
>> @@ -76,8 +78,7 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>>  		break;
>>  	case PGM_INT_CODE_PROTECTION:
>>  		/* Handling for iep.c test case. */
>> -		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
>> -		    !(lc->trans_exc_id & 0x08UL))
>> +		if (prot_is_iep(lc->trans_exc_id))
>>  			/*
>>  			 * We branched to the instruction that caused
>>  			 * the exception so we can use the return
>> @@ -126,6 +127,26 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>>  	/* suppressed/terminated/completed point already at the next address */
>>  }
>>  
>> +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:
>> +		print_decode_teid(lc->trans_exc_id);
>> +		break;
>> +	default:
>> +		return;
>> +	}
>> +}
>> +
>>  static void print_int_regs(struct stack_frame_int *stack)
>>  {
>>  	printf("\n");
>> @@ -155,6 +176,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();
>>  }
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index ef8041a6..5d1a33a0 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -72,6 +72,7 @@ cflatobjs += lib/s390x/css_lib.o
>>  cflatobjs += lib/s390x/malloc_io.o
>>  cflatobjs += lib/s390x/uv.o
>>  cflatobjs += lib/s390x/sie.o
>> +cflatobjs += lib/s390x/fault.o
>>  
>>  OBJDIRS += lib/s390x
>>  
>