| Message ID | 20220301095059.3026178-1-scgl@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2] s390x: Test effect of storage keys on some instructions | expand |
On 01/03/2022 10.50, Janis Schoetterl-Glausch wrote: > Some instructions are emulated by KVM. Test that KVM correctly emulates > storage key checking for two of those instructions (STORE CPU ADDRESS, > SET PREFIX). > Test success and error conditions, including coverage of storage and > fetch protection override. > Also add test for TEST PROTECTION, even if that instruction will not be > emulated by KVM under normal conditions. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- [...] > lib/s390x/asm/arch_def.h | 20 ++--- > s390x/skey.c | 171 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 182 insertions(+), 9 deletions(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index 40626d72..e443a9cd 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -55,15 +55,17 @@ struct psw { > #define PSW_MASK_BA 0x0000000080000000UL > #define PSW_MASK_64 (PSW_MASK_BA | PSW_MASK_EA) > > -#define CTL0_LOW_ADDR_PROT (63 - 35) > -#define CTL0_EDAT (63 - 40) > -#define CTL0_IEP (63 - 43) > -#define CTL0_AFP (63 - 45) > -#define CTL0_VECTOR (63 - 46) > -#define CTL0_EMERGENCY_SIGNAL (63 - 49) > -#define CTL0_EXTERNAL_CALL (63 - 50) > -#define CTL0_CLOCK_COMPARATOR (63 - 52) > -#define CTL0_SERVICE_SIGNAL (63 - 54) > +#define CTL0_LOW_ADDR_PROT (63 - 35) > +#define CTL0_EDAT (63 - 40) > +#define CTL0_FETCH_PROTECTION_OVERRIDE (63 - 38) > +#define CTL0_STORAGE_PROTECTION_OVERRIDE (63 - 39) Just a matter of taste, but IMHO the names are getting a little bit long here ... maybe use "PROT" instead of "PROTECTION" to shorten them a little bit? > +#define CTL0_IEP (63 - 43) > +#define CTL0_AFP (63 - 45) > +#define CTL0_VECTOR (63 - 46) > +#define CTL0_EMERGENCY_SIGNAL (63 - 49) > +#define CTL0_EXTERNAL_CALL (63 - 50) > +#define CTL0_CLOCK_COMPARATOR (63 - 52) > +#define CTL0_SERVICE_SIGNAL (63 - 54) > #define CR0_EXTM_MASK 0x0000000000006200UL /* Combined external masks */ > > #define CTL2_GUARDED_STORAGE (63 - 59) > diff --git a/s390x/skey.c b/s390x/skey.c > index 58a55436..0ab3172e 100644 > --- a/s390x/skey.c > +++ b/s390x/skey.c > @@ -10,6 +10,7 @@ > #include <libcflat.h> > #include <asm/asm-offsets.h> > #include <asm/interrupt.h> > +#include <vmalloc.h> > #include <asm/page.h> > #include <asm/facility.h> > #include <asm/mem.h> > @@ -147,6 +148,171 @@ static void test_invalid_address(void) > report_prefix_pop(); > } > > +static void test_test_protection(void) > +{ > + unsigned long addr = (unsigned long)pagebuf; > + > + report_prefix_push("TPROT"); > + set_storage_key(pagebuf, 0x10, 0); > + report(tprot(addr, 0) == 0, "access key 0 -> no protection"); > + report(tprot(addr, 1) == 0, "access key matches -> no protection"); > + report(tprot(addr, 2) == 1, "access key mismatches, no fetch protection -> store protection"); > + set_storage_key(pagebuf, 0x18, 0); > + report(tprot(addr, 2) == 2, "access key mismatches, fetch protection -> fetch & store protection"); > + report_prefix_pop(); Maybe also check storage protection override here? > +} > + Could you please add a short comment in front of the store_cpu_address_key_1() function, saying what it does? ... not everybody (at least not me) knows CPU instructions like SPKA by hand, so I had to look that up first to understand what this is doing. > +static void store_cpu_address_key_1(uint16_t *out) > +{ > + asm volatile ( > + "spka 0x10(0)\n\t" > + "stap %0\n\t" > + "spka 0(0)\n" > + : "+Q" (*out) /* exception: old value remains in out -> + constraint*/ > + ); > +} > + > +static void test_store_cpu_address(void) > +{ > + uint16_t *out = (uint16_t *)pagebuf; > + uint16_t cpu_addr; > + > + asm ("stap %0" : "=Q" (cpu_addr)); > + > + report_prefix_push("STORE CPU ADDRESS, zero key"); You could also use one report_prefix_push("STORE CPU ADDRESS") prefix for the whole function, so you don't have to repeat that string everywhere again. > + set_storage_key(pagebuf, 0x20, 0); > + *out = 0xbeef; > + asm ("stap %0" : "=Q" (*out)); I think it might be better to use +Q here ... otherwise the compiler might optimize the "*out = 0xbeef" away, since it sees that the variable is only written twice, but never used in between. > + report(*out == cpu_addr, "store occurred"); > + report_prefix_pop(); > + > + report_prefix_push("STORE CPU ADDRESS, matching key"); > + set_storage_key(pagebuf, 0x10, 0); > + *out = 0xbeef; > + store_cpu_address_key_1(out); > + report(*out == cpu_addr, "store occurred"); > + report_prefix_pop(); > + > + report_prefix_push("STORE CPU ADDRESS, mismatching key"); > + set_storage_key(pagebuf, 0x20, 0); > + expect_pgm_int(); > + *out = 0xbeef; > + store_cpu_address_key_1(out); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + report(*out == 0xbeef, "no store occurred"); > + report_prefix_pop(); > + > + ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); > + > + report_prefix_push("STORE CPU ADDRESS, storage-protection override, invalid key"); > + set_storage_key(pagebuf, 0x20, 0); > + expect_pgm_int(); > + *out = 0xbeef; > + store_cpu_address_key_1(out); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + report(*out == 0xbeef, "no store occurred"); > + report_prefix_pop(); > + > + report_prefix_push("STORE CPU ADDRESS, storage-protection override, override key"); > + set_storage_key(pagebuf, 0x90, 0); > + *out = 0xbeef; > + store_cpu_address_key_1(out); > + report(*out == cpu_addr, "override occurred"); > + report_prefix_pop(); > + > + ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); Wow, that protection override stuff was new to me, crazy stuff! Should we maybe check with set_storage_key(pagebuf, 0x90, 0) but storage protection override disabled, too, to see whether this correctly blocks the access again? > + set_storage_key(pagebuf, 0x00, 0); The other tests don't clear the storage key at the end, so why here now? > +} > + > +static void set_prefix_key_1(uint32_t *out) > +{ > + asm volatile ( > + "spka 0x10(0)\n\t" > + "spx %0\n\t" > + "spka 0(0)\n" > + :: "Q" (*out) > + ); > +} > + > +/* > + * We remapped page 0, making the lowcore inaccessible, which breaks the normal > + * hanlder and breaks skipping the faulting instruction. s/hanlder/handler/ > + * Just disable dynamic address translation to make things work. > + */ > +static void dat_fixup_pgm_int(void) > +{ > + uint64_t psw_mask = extract_psw_mask(); > + > + psw_mask &= ~PSW_MASK_DAT; > + load_psw_mask(psw_mask); > +} > + > +static void test_set_prefix(void) > +{ > + uint32_t *out = (uint32_t *)pagebuf; > + pgd_t *root; > + > + root = (pgd_t *)(stctg(1) & PAGE_MASK); > + > + asm volatile("stpx %0" : "=Q"(*out)); > + > + report_prefix_push("SET PREFIX, zero key"); You could do one report_prefix_push("SET PREFIX") for the whole function again. > + set_storage_key(pagebuf, 0x20, 0); > + asm volatile("spx %0" : "=Q" (*out)); > + report_pass("no exception"); > + report_prefix_pop(); > + > + report_prefix_push("SET PREFIX, matching key"); > + set_storage_key(pagebuf, 0x10, 0); > + set_prefix_key_1(out); > + report_pass("no exception"); > + report_prefix_pop(); > + > + report_prefix_push("SET PREFIX, mismatching key, no fetch protection"); > + set_storage_key(pagebuf, 0x20, 0); > + set_prefix_key_1(out); > + report_pass("no exception"); > + report_prefix_pop(); > + > + report_prefix_push("SET PREFIX, mismatching key, fetch protection"); > + set_storage_key(pagebuf, 0x28, 0); > + expect_pgm_int(); > + *out = 0xdeadbeef; > + set_prefix_key_1(out); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + asm volatile("stpx %0" : "=Q"(*out)); > + report(*out != 0xdeadbeef, "no fetch occurred"); > + report_prefix_pop(); > + > + register_pgm_cleanup_func(dat_fixup_pgm_int); > + ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE); > + > + report_prefix_push("SET PREFIX, mismatching key, fetch protection override applies"); > + set_storage_key(pagebuf, 0x28, 0); > + install_page(root, virt_to_pte_phys(root, pagebuf), 0); > + set_prefix_key_1(0); > + install_page(root, 0, 0); > + report_pass("no exception"); > + report_prefix_pop(); Could we do the same test (with the page remapped via DAT), just without fetch protection override, to make sure that it generates an exception there? > + report_prefix_push("SET PREFIX, mismatching key, fetch protection override does not apply"); > + out = (uint32_t *)(pagebuf + 2048); > + set_storage_key(pagebuf, 0x28, 0); > + expect_pgm_int(); > + install_page(root, virt_to_pte_phys(root, pagebuf), 0); > + WRITE_ONCE(*out, 0xdeadbeef); Would it make sense to swap the above two lines, i.e. first the WRITE_ONCE, then the install_page? ... access to *out between the two intall_page() calls requires me to think twice whether that's ok or not ;-) > + set_prefix_key_1((uint32_t *)2048); > + install_page(root, 0, 0); > + check_pgm_int_code(PGM_INT_CODE_PROTECTION); > + asm volatile("stpx %0" : "=Q"(*out)); > + report(*out != 0xdeadbeef, "no fetch occurred"); > + report_prefix_pop(); > + > + ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE); > + set_storage_key(pagebuf, 0x00, 0); Dito, why clearing the key here, but not in the other functions? > + register_pgm_cleanup_func(NULL); > +} > + > int main(void) > { > report_prefix_push("skey"); > @@ -159,6 +325,11 @@ int main(void) > test_set(); > test_set_mb(); > test_chg(); > + test_test_protection(); > + test_store_cpu_address(); > + > + setup_vm(); > + test_set_prefix(); > done: > report_prefix_pop(); > return report_summary(); Thomas
On 4/12/22 11:32, Thomas Huth wrote: > On 01/03/2022 10.50, Janis Schoetterl-Glausch wrote: >> Some instructions are emulated by KVM. Test that KVM correctly emulates >> storage key checking for two of those instructions (STORE CPU ADDRESS, >> SET PREFIX). >> Test success and error conditions, including coverage of storage and >> fetch protection override. >> Also add test for TEST PROTECTION, even if that instruction will not be >> emulated by KVM under normal conditions. >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >> --- > [...] >> lib/s390x/asm/arch_def.h | 20 ++--- >> s390x/skey.c | 171 +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 182 insertions(+), 9 deletions(-) >> >> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >> index 40626d72..e443a9cd 100644 >> --- a/lib/s390x/asm/arch_def.h >> +++ b/lib/s390x/asm/arch_def.h >> @@ -55,15 +55,17 @@ struct psw { >> #define PSW_MASK_BA 0x0000000080000000UL >> #define PSW_MASK_64 (PSW_MASK_BA | PSW_MASK_EA) >> -#define CTL0_LOW_ADDR_PROT (63 - 35) >> -#define CTL0_EDAT (63 - 40) >> -#define CTL0_IEP (63 - 43) >> -#define CTL0_AFP (63 - 45) >> -#define CTL0_VECTOR (63 - 46) >> -#define CTL0_EMERGENCY_SIGNAL (63 - 49) >> -#define CTL0_EXTERNAL_CALL (63 - 50) >> -#define CTL0_CLOCK_COMPARATOR (63 - 52) >> -#define CTL0_SERVICE_SIGNAL (63 - 54) >> +#define CTL0_LOW_ADDR_PROT (63 - 35) >> +#define CTL0_EDAT (63 - 40) >> +#define CTL0_FETCH_PROTECTION_OVERRIDE (63 - 38) >> +#define CTL0_STORAGE_PROTECTION_OVERRIDE (63 - 39) > > Just a matter of taste, but IMHO the names are getting a little bit long here ... maybe use "PROT" instead of "PROTECTION" to shorten them a little bit? It's called CR0_STORAGE_PROTECTION_OVERRIDE in the kernel and I want it to keep it similar to that. [...] >> +static void test_store_cpu_address(void) >> +{ >> + uint16_t *out = (uint16_t *)pagebuf; >> + uint16_t cpu_addr; >> + >> + asm ("stap %0" : "=Q" (cpu_addr)); >> + >> + report_prefix_push("STORE CPU ADDRESS, zero key"); > > You could also use one report_prefix_push("STORE CPU ADDRESS") prefix for the whole function, so you don't have to repeat that string everywhere again. > >> + set_storage_key(pagebuf, 0x20, 0); >> + *out = 0xbeef; >> + asm ("stap %0" : "=Q" (*out)); > > I think it might be better to use +Q here ... otherwise the compiler might optimize the "*out = 0xbeef" away, since it sees that the variable is only written twice, but never used in between. Good catch, I'll use WRITE_ONCE tho, since no exceptions should occur in the asm and it might be a bit misleading. [...] >> + set_storage_key(pagebuf, 0x00, 0); > > The other tests don't clear the storage key at the end, so why here now? It's not necessary, but Claudio suggested it for the last version and I like that it indicates that there is not supposed to be a shared state between the tests. [...] >> + report_prefix_push("SET PREFIX, mismatching key, fetch protection override does not apply"); >> + out = (uint32_t *)(pagebuf + 2048); >> + set_storage_key(pagebuf, 0x28, 0); >> + expect_pgm_int(); >> + install_page(root, virt_to_pte_phys(root, pagebuf), 0); >> + WRITE_ONCE(*out, 0xdeadbeef); > > Would it make sense to swap the above two lines, i.e. first the WRITE_ONCE, then the install_page? ... access to *out between the two intall_page() calls requires me to think twice whether that's ok or not ;-) Yes, that is possible. Alternatively we could do *(uint32_t *)2048 = 0xdeadbeef; Which might make it clearer to the reader what's happening. I'm not sure if it is a good idea of if the compiler would take it as an invitation to do funky things. [...] I'll implement your other suggestions, too. Thanks for the input! > > Thomas >
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index 40626d72..e443a9cd 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -55,15 +55,17 @@ struct psw { #define PSW_MASK_BA 0x0000000080000000UL #define PSW_MASK_64 (PSW_MASK_BA | PSW_MASK_EA) -#define CTL0_LOW_ADDR_PROT (63 - 35) -#define CTL0_EDAT (63 - 40) -#define CTL0_IEP (63 - 43) -#define CTL0_AFP (63 - 45) -#define CTL0_VECTOR (63 - 46) -#define CTL0_EMERGENCY_SIGNAL (63 - 49) -#define CTL0_EXTERNAL_CALL (63 - 50) -#define CTL0_CLOCK_COMPARATOR (63 - 52) -#define CTL0_SERVICE_SIGNAL (63 - 54) +#define CTL0_LOW_ADDR_PROT (63 - 35) +#define CTL0_EDAT (63 - 40) +#define CTL0_FETCH_PROTECTION_OVERRIDE (63 - 38) +#define CTL0_STORAGE_PROTECTION_OVERRIDE (63 - 39) +#define CTL0_IEP (63 - 43) +#define CTL0_AFP (63 - 45) +#define CTL0_VECTOR (63 - 46) +#define CTL0_EMERGENCY_SIGNAL (63 - 49) +#define CTL0_EXTERNAL_CALL (63 - 50) +#define CTL0_CLOCK_COMPARATOR (63 - 52) +#define CTL0_SERVICE_SIGNAL (63 - 54) #define CR0_EXTM_MASK 0x0000000000006200UL /* Combined external masks */ #define CTL2_GUARDED_STORAGE (63 - 59) diff --git a/s390x/skey.c b/s390x/skey.c index 58a55436..0ab3172e 100644 --- a/s390x/skey.c +++ b/s390x/skey.c @@ -10,6 +10,7 @@ #include <libcflat.h> #include <asm/asm-offsets.h> #include <asm/interrupt.h> +#include <vmalloc.h> #include <asm/page.h> #include <asm/facility.h> #include <asm/mem.h> @@ -147,6 +148,171 @@ static void test_invalid_address(void) report_prefix_pop(); } +static void test_test_protection(void) +{ + unsigned long addr = (unsigned long)pagebuf; + + report_prefix_push("TPROT"); + set_storage_key(pagebuf, 0x10, 0); + report(tprot(addr, 0) == 0, "access key 0 -> no protection"); + report(tprot(addr, 1) == 0, "access key matches -> no protection"); + report(tprot(addr, 2) == 1, "access key mismatches, no fetch protection -> store protection"); + set_storage_key(pagebuf, 0x18, 0); + report(tprot(addr, 2) == 2, "access key mismatches, fetch protection -> fetch & store protection"); + report_prefix_pop(); +} + +static void store_cpu_address_key_1(uint16_t *out) +{ + asm volatile ( + "spka 0x10(0)\n\t" + "stap %0\n\t" + "spka 0(0)\n" + : "+Q" (*out) /* exception: old value remains in out -> + constraint*/ + ); +} + +static void test_store_cpu_address(void) +{ + uint16_t *out = (uint16_t *)pagebuf; + uint16_t cpu_addr; + + asm ("stap %0" : "=Q" (cpu_addr)); + + report_prefix_push("STORE CPU ADDRESS, zero key"); + set_storage_key(pagebuf, 0x20, 0); + *out = 0xbeef; + asm ("stap %0" : "=Q" (*out)); + report(*out == cpu_addr, "store occurred"); + report_prefix_pop(); + + report_prefix_push("STORE CPU ADDRESS, matching key"); + set_storage_key(pagebuf, 0x10, 0); + *out = 0xbeef; + store_cpu_address_key_1(out); + report(*out == cpu_addr, "store occurred"); + report_prefix_pop(); + + report_prefix_push("STORE CPU ADDRESS, mismatching key"); + set_storage_key(pagebuf, 0x20, 0); + expect_pgm_int(); + *out = 0xbeef; + store_cpu_address_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + report(*out == 0xbeef, "no store occurred"); + report_prefix_pop(); + + ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); + + report_prefix_push("STORE CPU ADDRESS, storage-protection override, invalid key"); + set_storage_key(pagebuf, 0x20, 0); + expect_pgm_int(); + *out = 0xbeef; + store_cpu_address_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + report(*out == 0xbeef, "no store occurred"); + report_prefix_pop(); + + report_prefix_push("STORE CPU ADDRESS, storage-protection override, override key"); + set_storage_key(pagebuf, 0x90, 0); + *out = 0xbeef; + store_cpu_address_key_1(out); + report(*out == cpu_addr, "override occurred"); + report_prefix_pop(); + + ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); + set_storage_key(pagebuf, 0x00, 0); +} + +static void set_prefix_key_1(uint32_t *out) +{ + asm volatile ( + "spka 0x10(0)\n\t" + "spx %0\n\t" + "spka 0(0)\n" + :: "Q" (*out) + ); +} + +/* + * We remapped page 0, making the lowcore inaccessible, which breaks the normal + * hanlder and breaks skipping the faulting instruction. + * Just disable dynamic address translation to make things work. + */ +static void dat_fixup_pgm_int(void) +{ + uint64_t psw_mask = extract_psw_mask(); + + psw_mask &= ~PSW_MASK_DAT; + load_psw_mask(psw_mask); +} + +static void test_set_prefix(void) +{ + uint32_t *out = (uint32_t *)pagebuf; + pgd_t *root; + + root = (pgd_t *)(stctg(1) & PAGE_MASK); + + asm volatile("stpx %0" : "=Q"(*out)); + + report_prefix_push("SET PREFIX, zero key"); + set_storage_key(pagebuf, 0x20, 0); + asm volatile("spx %0" : "=Q" (*out)); + report_pass("no exception"); + report_prefix_pop(); + + report_prefix_push("SET PREFIX, matching key"); + set_storage_key(pagebuf, 0x10, 0); + set_prefix_key_1(out); + report_pass("no exception"); + report_prefix_pop(); + + report_prefix_push("SET PREFIX, mismatching key, no fetch protection"); + set_storage_key(pagebuf, 0x20, 0); + set_prefix_key_1(out); + report_pass("no exception"); + report_prefix_pop(); + + report_prefix_push("SET PREFIX, mismatching key, fetch protection"); + set_storage_key(pagebuf, 0x28, 0); + expect_pgm_int(); + *out = 0xdeadbeef; + set_prefix_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + asm volatile("stpx %0" : "=Q"(*out)); + report(*out != 0xdeadbeef, "no fetch occurred"); + report_prefix_pop(); + + register_pgm_cleanup_func(dat_fixup_pgm_int); + ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE); + + report_prefix_push("SET PREFIX, mismatching key, fetch protection override applies"); + set_storage_key(pagebuf, 0x28, 0); + install_page(root, virt_to_pte_phys(root, pagebuf), 0); + set_prefix_key_1(0); + install_page(root, 0, 0); + report_pass("no exception"); + report_prefix_pop(); + + report_prefix_push("SET PREFIX, mismatching key, fetch protection override does not apply"); + out = (uint32_t *)(pagebuf + 2048); + set_storage_key(pagebuf, 0x28, 0); + expect_pgm_int(); + install_page(root, virt_to_pte_phys(root, pagebuf), 0); + WRITE_ONCE(*out, 0xdeadbeef); + set_prefix_key_1((uint32_t *)2048); + install_page(root, 0, 0); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); + asm volatile("stpx %0" : "=Q"(*out)); + report(*out != 0xdeadbeef, "no fetch occurred"); + report_prefix_pop(); + + ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE); + set_storage_key(pagebuf, 0x00, 0); + register_pgm_cleanup_func(NULL); +} + int main(void) { report_prefix_push("skey"); @@ -159,6 +325,11 @@ int main(void) test_set(); test_set_mb(); test_chg(); + test_test_protection(); + test_store_cpu_address(); + + setup_vm(); + test_set_prefix(); done: report_prefix_pop(); return report_summary();
Some instructions are emulated by KVM. Test that KVM correctly emulates storage key checking for two of those instructions (STORE CPU ADDRESS, SET PREFIX). Test success and error conditions, including coverage of storage and fetch protection override. Also add test for TEST PROTECTION, even if that instruction will not be emulated by KVM under normal conditions. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- v1 -> v2: * use install_page instead of manual page table entry manipulation * check that no store occurred if none is expected * try to check that no fetch occured if not expected, although in practice a fetch would probably cause the test to crash * reset storage key to 0 after test Range-diff against v1: 1: a1069f68 ! 1: a2e076d3 s390x: Test effect of storage keys on some instructions @@ s390x/skey.c #include <asm/asm-offsets.h> #include <asm/interrupt.h> +#include <vmalloc.h> -+#include <mmu.h> #include <asm/page.h> -+#include <asm/pgtable.h> #include <asm/facility.h> #include <asm/mem.h> - @@ s390x/skey.c: static void test_invalid_address(void) report_prefix_pop(); } @@ s390x/skey.c: static void test_invalid_address(void) + "spka 0x10(0)\n\t" + "stap %0\n\t" + "spka 0(0)\n" -+ : "=Q" (*out) ++ : "+Q" (*out) /* exception: old value remains in out -> + constraint*/ + ); +} + @@ s390x/skey.c: static void test_invalid_address(void) + report_prefix_push("STORE CPU ADDRESS, mismatching key"); + set_storage_key(pagebuf, 0x20, 0); + expect_pgm_int(); ++ *out = 0xbeef; + store_cpu_address_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); ++ report(*out == 0xbeef, "no store occurred"); + report_prefix_pop(); + + ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); @@ s390x/skey.c: static void test_invalid_address(void) + report_prefix_push("STORE CPU ADDRESS, storage-protection override, invalid key"); + set_storage_key(pagebuf, 0x20, 0); + expect_pgm_int(); ++ *out = 0xbeef; + store_cpu_address_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); ++ report(*out == 0xbeef, "no store occurred"); + report_prefix_pop(); + + report_prefix_push("STORE CPU ADDRESS, storage-protection override, override key"); @@ s390x/skey.c: static void test_invalid_address(void) + report_prefix_pop(); + + ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE); ++ set_storage_key(pagebuf, 0x00, 0); +} + +static void set_prefix_key_1(uint32_t *out) @@ s390x/skey.c: static void test_invalid_address(void) + "spka 0x10(0)\n\t" + "spx %0\n\t" + "spka 0(0)\n" -+ : "=Q" (*out) ++ :: "Q" (*out) + ); +} + @@ s390x/skey.c: static void test_invalid_address(void) +{ + uint32_t *out = (uint32_t *)pagebuf; + pgd_t *root; -+ pte_t *entry_0_p; -+ pte_t entry_lowcore, entry_pagebuf; + + root = (pgd_t *)(stctg(1) & PAGE_MASK); -+ entry_0_p = get_dat_entry(root, 0, pgtable_level_pte); -+ entry_lowcore = *entry_0_p; -+ entry_pagebuf = __pte((virt_to_pte_phys(root, out) & PAGE_MASK)); + + asm volatile("stpx %0" : "=Q"(*out)); + @@ s390x/skey.c: static void test_invalid_address(void) + report_prefix_push("SET PREFIX, mismatching key, fetch protection"); + set_storage_key(pagebuf, 0x28, 0); + expect_pgm_int(); ++ *out = 0xdeadbeef; + set_prefix_key_1(out); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); ++ asm volatile("stpx %0" : "=Q"(*out)); ++ report(*out != 0xdeadbeef, "no fetch occurred"); + report_prefix_pop(); + + register_pgm_cleanup_func(dat_fixup_pgm_int); @@ s390x/skey.c: static void test_invalid_address(void) + + report_prefix_push("SET PREFIX, mismatching key, fetch protection override applies"); + set_storage_key(pagebuf, 0x28, 0); -+ ipte(0, &pte_val(*entry_0_p)); -+ *entry_0_p = entry_pagebuf; ++ install_page(root, virt_to_pte_phys(root, pagebuf), 0); + set_prefix_key_1(0); -+ ipte(0, &pte_val(*entry_0_p)); -+ *entry_0_p = entry_lowcore; ++ install_page(root, 0, 0); + report_pass("no exception"); + report_prefix_pop(); + + report_prefix_push("SET PREFIX, mismatching key, fetch protection override does not apply"); ++ out = (uint32_t *)(pagebuf + 2048); + set_storage_key(pagebuf, 0x28, 0); + expect_pgm_int(); -+ ipte(0, &pte_val(*entry_0_p)); -+ *entry_0_p = entry_pagebuf; ++ install_page(root, virt_to_pte_phys(root, pagebuf), 0); ++ WRITE_ONCE(*out, 0xdeadbeef); + set_prefix_key_1((uint32_t *)2048); -+ ipte(0, &pte_val(*entry_0_p)); -+ *entry_0_p = entry_lowcore; ++ install_page(root, 0, 0); + check_pgm_int_code(PGM_INT_CODE_PROTECTION); ++ asm volatile("stpx %0" : "=Q"(*out)); ++ report(*out != 0xdeadbeef, "no fetch occurred"); + report_prefix_pop(); + + ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE); ++ set_storage_key(pagebuf, 0x00, 0); + register_pgm_cleanup_func(NULL); +} + lib/s390x/asm/arch_def.h | 20 ++--- s390x/skey.c | 171 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 9 deletions(-) base-commit: 257c962f3d1b2d0534af59de4ad18764d734903a