On 5/6/22 17:37, Claudio Imbrenda wrote:
> On Thu, 5 May 2022 14:46:55 +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 | 2 ++
>> lib/s390x/sclp.c | 2 ++
>> s390x/skey.c | 60 ++++++++++++++++++++++++++++++++++++----
>> 4 files changed, 79 insertions(+), 6 deletions(-)
>>
[...]
>>
>> +enum access {
>> + ACC_FETCH = 2,
>> + ACC_STORE = 1,
>> + ACC_UPDATE = 3,
>> +};
>
> why not in numerical order?
The numbers are chosen such that the bit masking in the function below
is nicer, but the ordering is basically arbitrary.
Somehow fetch, store, update seems natural to me, but I can sort it.
I had ACC_NONE for a bit, but as I don't need it, I removed it.
>
>> +
>> +enum protection {
>> + PROT_STORE = 1,
>> + PROT_FETCH_STORE = 3,
>> +};
>
> what happened to 2?
There is no such thing as fetch only protection, so that's a result
of the choice of values for masking.
>
>> +
>> +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)
>
> why not teid.m?
I considered using .m, but the name does not contain any more information
on the meaning than just the number.
The PoP talks of the bits by their numbers in the suppression on detection
chapter, so this is the most straight forward translation into code.
>
>> + report_pass("key-controlled protection");
>> + break;
>> + case SOP_ENHANCED_2:
>> + if ((teid.val & (BIT(63 - 56) | BIT(63 - 61))) == 0) {
>
> maybe here you need to expand struct teid a little to accomodate for
> bit 56.
I could not think of a good name.
>
>> + report_pass("key-controlled protection");
>> + if (teid.val & BIT(63 - 60)) {
>> + int access_code = teid.fetch << 1 | teid.store;
>> +
>> + report_info("access code: %d", access_code);
>
> I don't like an unconditional report_info (it's ok to aid debugging if
> something fails)
In the case of update references the value you get is unspecified, so I found
it interesting to see what happens in LPAR.
I could only print it for update references, but I'm also fine with just
dropping it. What do you think?
>
>> + 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");
>
> what about cases 0 and 3?
Case 0 is specified to not contain any information and 3 is reserved,
so also no information.
> if they should never happen, handle it properly
> and if they can happen... handle it properly
>
>> + }
>> + }
>> + 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();
>>
>> @@ -314,7 +362,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();
>>
>> @@ -327,7 +375,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();
>>
>> @@ -351,7 +399,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();
>>
>
