On Fri, 20 May 2022 21:08:50 +0200
Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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)));
> }
>
