| Message ID | 20220520190850.3445768-4-scgl@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Rework TEID decoding and usage | expand |
On Fri, 20 May 2022 21:08:50 +0200 Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > The translation-exception identification (TEID) contains information to > identify the cause of certain program exceptions, including translation > exceptions occurring during dynamic address translation, as well as > protection exceptions. > The meaning of fields in the TEID is complex, depending on the exception > occurring and various potentially installed facilities. > > Rework the type describing the TEID, in order to ease decoding. > Change the existing code interpreting the TEID and extend it to take the > installed suppression-on-protection facility into account. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > lib/s390x/asm/interrupt.h | 66 ++++++++++++++++++++++++++-------- > lib/s390x/fault.h | 30 ++++------------ > lib/s390x/fault.c | 74 +++++++++++++++++++++++++++------------ > lib/s390x/interrupt.c | 2 +- > s390x/edat.c | 20 +++++++---- > 5 files changed, 124 insertions(+), 68 deletions(-) > > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h > index d9ab0bd7..8d5bfbf9 100644 > --- a/lib/s390x/asm/interrupt.h > +++ b/lib/s390x/asm/interrupt.h > @@ -20,23 +20,61 @@ > > union teid { > unsigned long val; > - struct { > - unsigned long addr:52; > - unsigned long fetch:1; > - unsigned long store:1; > - unsigned long reserved:6; > - unsigned long acc_list_prot:1; > - /* > - * depending on the exception and the installed facilities, > - * the m field can indicate several different things, > - * including whether the exception was triggered by a MVPG > - * instruction, or whether the addr field is meaningful > - */ > - unsigned long m:1; > - unsigned long asce_id:2; > + union { > + /* common fields DAT exc & protection exc */ > + struct { > + uint64_t addr : 52 - 0; > + uint64_t acc_exc_f_s : 54 - 52; > + uint64_t side_effect_acc : 55 - 54; > + uint64_t /* reserved */ : 55 - 54; shouldn't this ^ be 62 - 55 ? > + 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; > + }; > + /* 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 > +}; > + > +static inline enum prot_code teid_esop2_prot_code(union teid teid) > +{ > + int code = 0; > + > + code = code << 1 | teid.esop2_prot_code_0; > + code = code << 1 | teid.esop2_prot_code_1; > + code = code << 1 | teid.esop2_prot_code_2; > + return (enum prot_code)code; return (enum prot_code)(teid.esop2_prot_code_0 << 2 | teid.esop2_prot_code_1 << 1 | teid.esop2_prot_code_2); > +} > + > void register_pgm_cleanup_func(void (*f)(void)); > void handle_pgm_int(struct stack_frame_int *stack); > void handle_ext_int(struct stack_frame_int *stack); > diff --git a/lib/s390x/fault.h b/lib/s390x/fault.h > index 726da2f0..867997f2 100644 > --- a/lib/s390x/fault.h > +++ b/lib/s390x/fault.h > @@ -11,32 +11,16 @@ > #define _S390X_FAULT_H_ > > #include <bitops.h> > +#include <asm/facility.h> > +#include <asm/interrupt.h> > > /* Instruction execution prevention, i.e. no-execute, 101 */ > -static inline bool prot_is_iep(uint64_t teid) > +static inline bool prot_is_iep(union teid 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; > + if (!test_facility(130)) > + return false; > + /* IEP installed -> ESOP2 installed */ > + return teid_esop2_prot_code(teid) == PROT_IEP; > } > > void print_decode_teid(uint64_t teid); > diff --git a/lib/s390x/fault.c b/lib/s390x/fault.c > index efa62fcb..02b3c098 100644 > --- a/lib/s390x/fault.c > +++ b/lib/s390x/fault.c > @@ -13,35 +13,63 @@ > #include <asm/page.h> > #include <fault.h> > > -/* Decodes the protection exceptions we'll most likely see */ > -static void print_decode_pgm_prot(uint64_t teid) > -{ > - 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; > +static void print_decode_pgm_prot(union teid teid, bool dat) > +{ > + switch (get_supp_on_prot_facility()) { > + case SOP_NONE: > + printf("Type: ?\n"); > + break; > + case SOP_BASIC: > + if (teid.sop_teid_predictable && dat && teid.sop_acc_list) > + printf("Type: ACC\n"); > + else > + printf("Type: ?\n"); > + break; > + case SOP_ENHANCED_1: > + if (teid.esop1_acc_list_or_dat) { > + if (teid.sop_acc_list) > + printf("Type: ACC\n"); > + else > + printf("Type: DAT\n"); > + } else { > + printf("Type: KEY or LAP\n"); > + } > + break; > + case SOP_ENHANCED_2: > + switch (teid_esop2_prot_code(teid)) { I wonder if it weren't easier to do static const char * const prot_strings[6] = {"KEY or LAP", "DAT", ...}; printf("Type: %s\n", prot_strings[teid_esop2_prot_code(teid)]); > + case PROT_KEY_LAP: > + printf("Type: KEY or LAP\n"); > + break; > + case PROT_DAT: > + printf("Type: DAT\n"); > + break; > + case PROT_KEY: > + printf("Type: KEY\n"); > + break; > + case PROT_ACC_LIST: > + printf("Type: ACC\n"); > + break; > + case PROT_LAP: > + printf("Type: LAP\n"); > + break; > + case PROT_IEP: > + printf("Type: IEP\n"); > + break; > + } > } > } > > -void print_decode_teid(uint64_t teid) > +void print_decode_teid(uint64_t raw_teid) > { > - int asce_id = teid & 3; > + union teid teid = { .val = raw_teid }; > bool dat = lowcore.pgm_old_psw.mask & PSW_MASK_DAT; > > printf("Memory exception information:\n"); > printf("DAT: %s\n", dat ? "on" : "off"); > > printf("AS: "); > - switch (asce_id) { > + switch (teid.asce_id) { > case AS_PRIM: > printf("Primary\n"); > break; > @@ -57,7 +85,7 @@ void print_decode_teid(uint64_t teid) > } > > if (lowcore.pgm_int_code == PGM_INT_CODE_PROTECTION) > - print_decode_pgm_prot(teid); > + print_decode_pgm_prot(teid, dat); > > /* > * If teid bit 61 is off for these two exception the reported > @@ -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 ? > } > 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; add: if (teid.sop_acc_list) return false; > + 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... 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? also, handle the PROT_ACC_LIST too also, add: if (PROT_KEY_LAP) return true; 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 > + return false; > + } > + return (!teid.sop_acc_list && !teid.asce_id && > (teid.addr == ((unsigned long)ptr >> PAGE_SHIFT))); > } >
On 5/24/22 16:40, Claudio Imbrenda wrote: > On Fri, 20 May 2022 21:08:50 +0200 > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > >> The translation-exception identification (TEID) contains information to >> identify the cause of certain program exceptions, including translation >> exceptions occurring during dynamic address translation, as well as >> protection exceptions. >> The meaning of fields in the TEID is complex, depending on the exception >> occurring and various potentially installed facilities. >> >> Rework the type describing the TEID, in order to ease decoding. >> Change the existing code interpreting the TEID and extend it to take the >> installed suppression-on-protection facility into account. >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >> --- >> lib/s390x/asm/interrupt.h | 66 ++++++++++++++++++++++++++-------- >> lib/s390x/fault.h | 30 ++++------------ >> lib/s390x/fault.c | 74 +++++++++++++++++++++++++++------------ >> lib/s390x/interrupt.c | 2 +- >> s390x/edat.c | 20 +++++++---- >> 5 files changed, 124 insertions(+), 68 deletions(-) >> >> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h >> index d9ab0bd7..8d5bfbf9 100644 >> --- a/lib/s390x/asm/interrupt.h >> +++ b/lib/s390x/asm/interrupt.h >> @@ -20,23 +20,61 @@ >> >> union teid { >> unsigned long val; >> - struct { >> - unsigned long addr:52; >> - unsigned long fetch:1; >> - unsigned long store:1; >> - unsigned long reserved:6; >> - unsigned long acc_list_prot:1; >> - /* >> - * depending on the exception and the installed facilities, >> - * the m field can indicate several different things, >> - * including whether the exception was triggered by a MVPG >> - * instruction, or whether the addr field is meaningful >> - */ >> - unsigned long m:1; >> - unsigned long asce_id:2; >> + union { >> + /* common fields DAT exc & protection exc */ >> + struct { >> + uint64_t addr : 52 - 0; >> + uint64_t acc_exc_f_s : 54 - 52; >> + uint64_t side_effect_acc : 55 - 54; >> + uint64_t /* reserved */ : 55 - 54; > > shouldn't this ^ be 62 - 55 ? Oops, yes. > >> + 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. 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. >> + }; >> + /* 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? Then we'd be notified to fix the test. [...] >> +static void print_decode_pgm_prot(union teid teid, bool dat) >> +{ >> + switch (get_supp_on_prot_facility()) { >> + case SOP_NONE: >> + printf("Type: ?\n"); >> + break; >> + case SOP_BASIC: >> + if (teid.sop_teid_predictable && dat && teid.sop_acc_list) >> + printf("Type: ACC\n"); >> + else >> + printf("Type: ?\n"); >> + break; >> + case SOP_ENHANCED_1: >> + if (teid.esop1_acc_list_or_dat) { >> + if (teid.sop_acc_list) >> + printf("Type: ACC\n"); >> + else >> + printf("Type: DAT\n"); >> + } else { >> + printf("Type: KEY or LAP\n"); >> + } >> + break; >> + case SOP_ENHANCED_2: >> + switch (teid_esop2_prot_code(teid)) { > > I wonder if it weren't easier to do > > static const char * const prot_strings[6] = {"KEY or LAP", "DAT", ...}; > printf("Type: %s\n", prot_strings[teid_esop2_prot_code(teid)]); > Yeah, good idea. >> + case PROT_KEY_LAP: >> + printf("Type: KEY or LAP\n"); >> + break; >> + case PROT_DAT: >> + printf("Type: DAT\n"); >> + break; >> + case PROT_KEY: >> + printf("Type: KEY\n"); >> + break; >> + case PROT_ACC_LIST: >> + printf("Type: ACC\n"); >> + break; >> + case PROT_LAP: >> + printf("Type: LAP\n"); >> + break; >> + case PROT_IEP: >> + printf("Type: IEP\n"); >> + break; >> + } >> } >> } >> [...] >> @@ -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. > >> } >> 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. > 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. > > 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 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))); >> } >> >
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))); > >> } > >> > > >
diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h index d9ab0bd7..8d5bfbf9 100644 --- a/lib/s390x/asm/interrupt.h +++ b/lib/s390x/asm/interrupt.h @@ -20,23 +20,61 @@ union teid { unsigned long val; - struct { - unsigned long addr:52; - unsigned long fetch:1; - unsigned long store:1; - unsigned long reserved:6; - unsigned long acc_list_prot:1; - /* - * depending on the exception and the installed facilities, - * the m field can indicate several different things, - * including whether the exception was triggered by a MVPG - * instruction, or whether the addr field is meaningful - */ - unsigned long m:1; - unsigned long asce_id:2; + union { + /* common fields DAT exc & protection exc */ + struct { + uint64_t addr : 52 - 0; + uint64_t acc_exc_f_s : 54 - 52; + uint64_t side_effect_acc : 55 - 54; + uint64_t /* reserved */ : 55 - 54; + 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; + uint64_t esop1_acc_list_or_dat : 62 - 61; + }; + /* 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, +}; + +static inline enum prot_code teid_esop2_prot_code(union teid teid) +{ + int code = 0; + + code = code << 1 | teid.esop2_prot_code_0; + code = code << 1 | teid.esop2_prot_code_1; + code = code << 1 | teid.esop2_prot_code_2; + return (enum prot_code)code; +} + void register_pgm_cleanup_func(void (*f)(void)); void handle_pgm_int(struct stack_frame_int *stack); void handle_ext_int(struct stack_frame_int *stack); diff --git a/lib/s390x/fault.h b/lib/s390x/fault.h index 726da2f0..867997f2 100644 --- a/lib/s390x/fault.h +++ b/lib/s390x/fault.h @@ -11,32 +11,16 @@ #define _S390X_FAULT_H_ #include <bitops.h> +#include <asm/facility.h> +#include <asm/interrupt.h> /* Instruction execution prevention, i.e. no-execute, 101 */ -static inline bool prot_is_iep(uint64_t teid) +static inline bool prot_is_iep(union teid 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; + if (!test_facility(130)) + return false; + /* IEP installed -> ESOP2 installed */ + return teid_esop2_prot_code(teid) == PROT_IEP; } void print_decode_teid(uint64_t teid); diff --git a/lib/s390x/fault.c b/lib/s390x/fault.c index efa62fcb..02b3c098 100644 --- a/lib/s390x/fault.c +++ b/lib/s390x/fault.c @@ -13,35 +13,63 @@ #include <asm/page.h> #include <fault.h> -/* Decodes the protection exceptions we'll most likely see */ -static void print_decode_pgm_prot(uint64_t teid) -{ - 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; +static void print_decode_pgm_prot(union teid teid, bool dat) +{ + switch (get_supp_on_prot_facility()) { + case SOP_NONE: + printf("Type: ?\n"); + break; + case SOP_BASIC: + if (teid.sop_teid_predictable && dat && teid.sop_acc_list) + printf("Type: ACC\n"); + else + printf("Type: ?\n"); + break; + case SOP_ENHANCED_1: + if (teid.esop1_acc_list_or_dat) { + if (teid.sop_acc_list) + printf("Type: ACC\n"); + else + printf("Type: DAT\n"); + } else { + printf("Type: KEY or LAP\n"); + } + break; + case SOP_ENHANCED_2: + switch (teid_esop2_prot_code(teid)) { + case PROT_KEY_LAP: + printf("Type: KEY or LAP\n"); + break; + case PROT_DAT: + printf("Type: DAT\n"); + break; + case PROT_KEY: + printf("Type: KEY\n"); + break; + case PROT_ACC_LIST: + printf("Type: ACC\n"); + break; + case PROT_LAP: + printf("Type: LAP\n"); + break; + case PROT_IEP: + printf("Type: IEP\n"); + break; + } } } -void print_decode_teid(uint64_t teid) +void print_decode_teid(uint64_t raw_teid) { - int asce_id = teid & 3; + union teid teid = { .val = raw_teid }; bool dat = lowcore.pgm_old_psw.mask & PSW_MASK_DAT; printf("Memory exception information:\n"); printf("DAT: %s\n", dat ? "on" : "off"); printf("AS: "); - switch (asce_id) { + switch (teid.asce_id) { case AS_PRIM: printf("Primary\n"); break; @@ -57,7 +85,7 @@ void print_decode_teid(uint64_t teid) } if (lowcore.pgm_int_code == PGM_INT_CODE_PROTECTION) - print_decode_pgm_prot(teid); + print_decode_pgm_prot(teid, dat); /* * If teid bit 61 is off for these two exception the reported @@ -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); } 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; + case SOP_ENHANCED_1: + if (!teid.esop1_acc_list_or_dat) + return false; + case SOP_ENHANCED_2: + if (teid_esop2_prot_code(teid) != 1) + return false; + } + return (!teid.sop_acc_list && !teid.asce_id && (teid.addr == ((unsigned long)ptr >> PAGE_SHIFT))); }
The translation-exception identification (TEID) contains information to identify the cause of certain program exceptions, including translation exceptions occurring during dynamic address translation, as well as protection exceptions. The meaning of fields in the TEID is complex, depending on the exception occurring and various potentially installed facilities. Rework the type describing the TEID, in order to ease decoding. Change the existing code interpreting the TEID and extend it to take the installed suppression-on-protection facility into account. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- lib/s390x/asm/interrupt.h | 66 ++++++++++++++++++++++++++-------- lib/s390x/fault.h | 30 ++++------------ lib/s390x/fault.c | 74 +++++++++++++++++++++++++++------------ lib/s390x/interrupt.c | 2 +- s390x/edat.c | 20 +++++++---- 5 files changed, 124 insertions(+), 68 deletions(-)