Re: [kvm-unit-tests PATCH v2 4/4] s390x: Add storage key removal facility

Janosch Frank
2019-08-28T14:16:06Z

[-- Attachment #1.1: Type: text/plain, Size: 2761 bytes --]

On 8/28/19 2:02 PM, Thomas Huth wrote:
> On 28/08/2019 13.36, Janosch Frank wrote:
>> The storage key removal facility (stfle bit 169) makes all key related
>> instructions result in a special operation exception if they handle a
>> key.
>>
>> Let's make sure that the skey and pfmf tests only run non key code
>> (pfmf) or not at all (skey).
>>
>> Also let's test this new facility. As lots of instructions are
>> affected by this, only some of them are tested for now.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> s390x/Makefile | 1 +
>> s390x/pfmf.c | 10 ++++
>> s390x/skey.c | 5 ++
>> s390x/skrf.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 144 insertions(+)
>> create mode 100644 s390x/skrf.c
> [...]
>> +static void test_mvcos(void)
>> +{
>> + uint64_t r3 = 64;
>> + uint8_t *src = pagebuf;
>> + uint8_t *dst = pagebuf + PAGE_SIZE;
>> + /* K bit set, as well as keys */
>> + register unsigned long oac asm("0") = 0xf002f002;
>> +
>> + report_prefix_push("mvcos");
>> + expect_pgm_int();
>> + asm volatile("mvcos %[dst],%[src],%[len]"
>> + : [dst] "+Q" (*(dst))
>> + : [src] "Q" (*(src)), [len] "d" (r3), "d" (oac)
>
> Just a nit: I think you could write "*dst" instead of "*(dst)" and
> "*src" instead of "*(src)".
>
>> + : "cc", "memory");
>> + check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> + report_prefix_pop();
>> +}
>> +
>> +static void test_spka(void)
>> +{
>> + report_prefix_push("spka");
>> + expect_pgm_int();
>> + asm volatile("spka 0xf0(0)\n");
>> + check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> + report_prefix_pop();
>> +}
>> +
>> +static void test_tprot(void)
>> +{
>> + report_prefix_push("tprot");
>> + expect_pgm_int();
>> + asm volatile("tprot %[addr],0xf0(0)\n"
>> + : : [addr] "a" (pagebuf) : );
>> + check_pgm_int_code(PGM_INT_CODE_SPECIAL_OPERATION);
>> + report_prefix_pop();
>> +}
>> +
>> +int main(void)
>> +{
>> + report_prefix_push("skrf");
>> + if (!test_facility(169)) {
>> + report_skip("storage key removal facility not available\n");
>> + goto done;
>> + }
>> +
>> + test_facilities();
>> + test_skey();
>> + test_pfmf();
>> + test_psw_key();
>> + test_mvcos();
>> + test_spka();
>> + test_tprot();
>> +
>> +done:
>> + report_prefix_pop();
>> + return report_summary();
>> +}
>
> I can't say much about the technical details here (since I don't have
> the doc for that "removal facility"), but apart from that, the patch
> looks fine to me now.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
>
> (and I'll wait one or two more days for additional reviews before
> queuing the patches)
>
Great, thank you!

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]