On Tue, 2022-05-17 at 15:46 +0200, Claudio Imbrenda wrote: > On Tue, 17 May 2022 13:56:05 +0200 > Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote: > > > On a protection exception, test that the Translation-Exception > > Identification (TEID) values are correct given the circumstances of the > > particular test. > > The meaning of the TEID values is dependent on the installed > > suppression-on-protection facility. > > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> > > --- > > lib/s390x/asm/facility.h | 21 ++++++++++++++ > > lib/s390x/sclp.h | 4 +++ > > lib/s390x/sclp.c | 2 ++ > > s390x/skey.c | 60 ++++++++++++++++++++++++++++++++++++---- > > 4 files changed, 81 insertions(+), 6 deletions(-) > > [...] > > +static void check_key_prot_exc(enum access access, enum protection prot) > > +{ > > + struct lowcore *lc = 0; > > + union teid teid; > > + > > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > > + report_prefix_push("TEID"); > > + teid.val = lc->trans_exc_id; > > + switch (get_supp_on_prot_facility()) { > > + case SOP_NONE: > > + case SOP_BASIC: > > + break; > > + case SOP_ENHANCED_1: > > + if ((teid.val & (BIT(63 - 61))) == 0) > > can you at least replace the hardcoded values with a macro or a const > variable? I'll see if maybe I can come up with a nice way to extend the teid, but I'll use a const if not. > > like: > > const unsigned long esop_bit = BIT(63 - 61); > > ... > > if (!(teid.val & esop_bit)) > > > + report_pass("key-controlled protection"); > > actually, now that I think of it, aren't we expecting the bit to be > zero? should that not be like this? > > report (!(teid.val & esop_bit), ...); Indeed. > > > + break; > > + case SOP_ENHANCED_2: > > + if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) { > > const unsigned long esop2_bits = 0x8C; /* bits 56, 60, and 61 */ > const unsigned long esop2_key_prot = BIT(63 - 60); > > if ((teid.val & esop2_bits) == 0) { > report_pass(...); > > > + report_pass("key-controlled protection"); > > + if (teid.val & BIT(63 - 60)) { > > } else if ((teid.val & esop2_bits) == esop_key_prot) { 010 binary also means key protection, so we should pass that test here, too. The access code checking is an additional test, IMO. > > > + int access_code = teid.fetch << 1 | teid.store; > > + > > + if (access_code == 2) > > + report((access & 2) && (prot & 2), > > + "exception due to fetch"); > > + if (access_code == 1) > > + report((access & 1) && (prot & 1), > > + "exception due to store"); > > + /* no relevant information if code is 0 or 3 */ > > here you should check for the access-exception-fetch/store-indi- > cation facility, then you can check the access code Oh, yes. By the way, can we get rid of magic numbers for facility checking? Just defining an enum in lib/asm/facility.h and doing test_facility(FCLTY_ACCESS_EXC_FETCH_STORE_INDICATION) would be an improvement. Well, I guess you'd end up with quite horribly long names, but at least you have to review the values only once and not for every patch that tests a facility. > > and at this point you should check for 0 explicitly (always pass) and 3 > (always fail) I'm fine with passing 0, but I'm not so sure about 3. The value is reserved, so the correct thing to do is to not attribute *any* meaning to it. But kvm currently really should not set it either. > > + } > > + } > > } else { > /* not key protection */ > report_fail(...); > } > > + break; > > + } > > + report_prefix_pop(); > > +} > > + [...]