On Wed, 18 Aug 2021 11:12:57 +0200
Thomas Huth <thuth@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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();
> > }
> >
>
