| Message ID | 20210813073615.32837-4-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Cleanup and maintenance | expand |
On Fri, 13 Aug 2021 07:36:10 +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> > --- > 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) it is actually more complicated than this. if you don't want to add all the possibilities because they are unlikely and/or not relevant, maybe add a comment > +{ > + /* Low-address protection exception, 100 */ > + if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && > !test_bit_inv(61, &teid)) { > + 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; > + } > +} > + > +static void decode_teid(uint64_t teid) > +{ > + int asce_id = lc->trans_exc_id & 3; > + 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"); > + > + 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; > + } > +} > + > 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(); > }
On 8/13/21 10:40 AM, Claudio Imbrenda wrote: > On Fri, 13 Aug 2021 07:36:10 +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> >> --- >> 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) > > it is actually more complicated than this. I know it hurts to look at the spec :) > > if you don't want to add all the possibilities because they are > unlikely and/or not relevant, maybe add a comment Will do! > >> +{ >> + /* Low-address protection exception, 100 */ >> + if (test_bit_inv(56, &teid) && !test_bit_inv(60, &teid) && >> !test_bit_inv(61, &teid)) { >> + 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; >> + } >> +} >> + >> +static void decode_teid(uint64_t teid) >> +{ >> + int asce_id = lc->trans_exc_id & 3; >> + 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"); >> + >> + 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; >> + } >> +} >> + >> 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(); >> } >
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) { > + 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? > +} > + > +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. > + 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. > + 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. > + } > +} > + > 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(); > } >
On Wed, 18 Aug 2021 11:12:57 +0200 Thomas Huth <thuth@redhat.com> 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) { or even better: switch (teid & TEID_MASK) { > > > + 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? > > > +} > > + > > +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. > > > + 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. > > > + 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. > > > + } > > +} > > + > > 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(); > > } > > >
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(); >> } >> >
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)) { + 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; + } +} + +static void decode_teid(uint64_t teid) +{ + int asce_id = lc->trans_exc_id & 3; + 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"); + + 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; + } +} + 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(); }
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(+)