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(-)
>
> diff --git a/lib/s390x/asm/facility.h b/lib/s390x/asm/facility.h
> index ef0fd037..f21bb9d7 100644
> --- a/lib/s390x/asm/facility.h
> +++ b/lib/s390x/asm/facility.h
> @@ -12,6 +12,7 @@
> #include <asm/facility.h>
> #include <asm/arch_def.h>
> #include <bitops.h>
> +#include <sclp.h>
>
> #define NB_STFL_DOUBLEWORDS 32
> extern uint64_t stfl_doublewords[];
> @@ -44,4 +45,24 @@ static inline void setup_facilities(void)
> stfle(stfl_doublewords, NB_STFL_DOUBLEWORDS);
> }
>
> +enum supp_on_prot_facility {
> + SOP_NONE,
> + SOP_BASIC,
> + SOP_ENHANCED_1,
> + SOP_ENHANCED_2,
> +};
> +
> +static inline enum supp_on_prot_facility get_supp_on_prot_facility(void)
> +{
> + if (sclp_facilities.has_esop) {
> + if (test_facility(131)) /* side-effect-access facility */
> + return SOP_ENHANCED_2;
> + else
> + return SOP_ENHANCED_1;
> + }
> + if (sclp_facilities.has_sop)
> + return SOP_BASIC;
> + return SOP_NONE;
> +}
> +
> #endif
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index 3488f4d2..853529bf 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -123,7 +123,9 @@ struct sclp_facilities {
> uint64_t has_cei : 1;
>
> uint64_t has_diag318 : 1;
> + uint64_t has_sop : 1;
> uint64_t has_gsls : 1;
> + uint64_t has_esop : 1;
> uint64_t has_cmma : 1;
> uint64_t has_64bscao : 1;
> uint64_t has_esca : 1;
> @@ -134,7 +136,9 @@ struct sclp_facilities {
> };
>
> /* bit number within a certain byte */
> +#define SCLP_FEAT_80_BIT_SOP 2
> #define SCLP_FEAT_85_BIT_GSLS 0
> +#define SCLP_FEAT_85_BIT_ESOP 6
> #define SCLP_FEAT_98_BIT_KSS 7
> #define SCLP_FEAT_116_BIT_64BSCAO 0
> #define SCLP_FEAT_116_BIT_CMMA 1
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index b8204c5f..e6017f64 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -152,7 +152,9 @@ void sclp_facilities_setup(void)
> cpu = sclp_get_cpu_entries();
> if (read_info->offset_cpu > 134)
> sclp_facilities.has_diag318 = read_info->byte_134_diag318;
> + sclp_facilities.has_sop = sclp_feat_check(80, SCLP_FEAT_80_BIT_SOP);
> sclp_facilities.has_gsls = sclp_feat_check(85, SCLP_FEAT_85_BIT_GSLS);
> + sclp_facilities.has_esop = sclp_feat_check(85, SCLP_FEAT_85_BIT_ESOP);
> sclp_facilities.has_kss = sclp_feat_check(98, SCLP_FEAT_98_BIT_KSS);
> sclp_facilities.has_cmma = sclp_feat_check(116, SCLP_FEAT_116_BIT_CMMA);
> sclp_facilities.has_64bscao = sclp_feat_check(116, SCLP_FEAT_116_BIT_64BSCAO);
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 7aa91d19..19fa5721 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -8,6 +8,7 @@
> * Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> */
> #include <libcflat.h>
> +#include <bitops.h>
> #include <asm/asm-offsets.h>
> #include <asm/interrupt.h>
> #include <vmalloc.h>
> @@ -158,6 +159,53 @@ static void test_test_protection(void)
> report_prefix_pop();
> }
>
> +enum access {
> + ACC_STORE = 1,
> + ACC_FETCH = 2,
> + ACC_UPDATE = 3,
> +};
> +
> +enum protection {
> + PROT_STORE = 1,
> + PROT_FETCH_STORE = 3,
> +};
> +
> +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?
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), ...);
> + 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) {
> + 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
and at this point you should check for 0 explicitly (always pass) and 3
(always fail)
> + }
> + }
} else {
/* not key protection */
report_fail(...);
}
> + break;
> + }
> + report_prefix_pop();
> +}
> +
> /*
> * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
> * with access key 1.
> @@ -199,7 +247,7 @@ static void test_store_cpu_address(void)
> expect_pgm_int();
> *out = 0xbeef;
> store_cpu_address_key_1(out);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_STORE, PROT_STORE);
> report(*out == 0xbeef, "no store occurred");
> report_prefix_pop();
>
> @@ -210,7 +258,7 @@ static void test_store_cpu_address(void)
> expect_pgm_int();
> *out = 0xbeef;
> store_cpu_address_key_1(out);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_STORE, PROT_STORE);
> report(*out == 0xbeef, "no store occurred");
> report_prefix_pop();
>
> @@ -228,7 +276,7 @@ static void test_store_cpu_address(void)
> expect_pgm_int();
> *out = 0xbeef;
> store_cpu_address_key_1(out);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_STORE, PROT_STORE);
> report(*out == 0xbeef, "no store occurred");
> report_prefix_pop();
>
> @@ -321,7 +369,7 @@ static void test_set_prefix(void)
> set_storage_key(pagebuf, 0x28, 0);
> expect_pgm_int();
> set_prefix_key_1(prefix_ptr);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> report(get_prefix() == old_prefix, "did not set prefix");
> report_prefix_pop();
>
> @@ -334,7 +382,7 @@ static void test_set_prefix(void)
> install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> set_prefix_key_1((uint32_t *)0);
> install_page(root, 0, 0);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> report(get_prefix() == old_prefix, "did not set prefix");
> report_prefix_pop();
>
> @@ -358,7 +406,7 @@ static void test_set_prefix(void)
> install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> set_prefix_key_1((uint32_t *)2048);
> install_page(root, 0, 0);
> - check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> + check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> report(get_prefix() == old_prefix, "did not set prefix");
> report_prefix_pop();
>
