On Wed, 8 Jun 2022 15:33:03 +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 | 61 +++++++++++++++++++++++++++---------
> lib/s390x/fault.h | 30 +++++-------------
> lib/s390x/fault.c | 65 ++++++++++++++++++++++++++-------------
> lib/s390x/interrupt.c | 2 +-
> s390x/edat.c | 26 ++++++++++------
> 5 files changed, 115 insertions(+), 69 deletions(-)
>
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index d9ab0bd7..3ca6bf76 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -20,23 +20,56 @@
>
> 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 */ : 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 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,
add:
PROT_CODE_SIZE, /* Must always be the last one */
[...]
> + case SOP_ENHANCED_2: {
> + static const char * const prot_str[] = {
static const char * const prot_str[PROT_CODE_SIZE] = {
so you have the guarantee that this has the right size, and you will
get a compile error if a new value is added to the enum but not here
and at this point I think it might make more sense to move this right
after the enum itself
> + "KEY or LAP",
> + "DAT",
> + "KEY",
> + "ACC",
> + "LAP",
> + "IEP",
> + };
> + int prot_code = teid_esop2_prot_code(teid);
enum prot_code prot_code = teid_esop2_prot_code(teid);
>
> - if (prot_is_datp(teid)) {
> - printf("Type: DAT\n");
> - return;
> + assert(0 <= prot_code && prot_code < ARRAY_SIZE(prot_str));
then you can remove this assert ^
> + printf("Type: %s\n", prot_str[prot_code]);
> + }
> }
> }
>
> -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 +78,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 +86,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..89ff4f51 100644
> --- a/s390x/edat.c
> +++ b/s390x/edat.c
> @@ -26,8 +26,8 @@ static void *root, *mem, *m;
> volatile unsigned int *p;
>
> /*
> - * Check if a non-access-list protection exception happened for the given
> - * address, in the primary address space.
> + * Check if the exception is consistent with DAT protection and has the correct
> + * address and primary address space.
> */
> static bool check_pgm_prot(void *ptr)
> {
> @@ -37,14 +37,22 @@ 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;
> + break;
> + case SOP_ENHANCED_1:
> + if (!teid.sop_teid_predictable) /* implies key or low addr */
> + return false;
> + break;
> + case SOP_ENHANCED_2:
> + if (teid_esop2_prot_code(teid) != PROT_DAT)
> + return false;
> + }
> + return (!teid.sop_acc_list && !teid.asce_id &&
> (teid.addr == ((unsigned long)ptr >> PAGE_SHIFT)));
> }
>
