On Fri, 3 Jun 2022 15:49:33 +0200
Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> 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)));
> >> }
> >>
> >
>
