On Tue, 2022-05-17 at 15:54 +0200, Claudio Imbrenda wrote: > On Tue, 17 May 2022 13:56:06 +0200 > Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote: > > > Test correctness of some instructions handled by user space instead of > > KVM with regards to storage keys. > > Test success and error conditions, including coverage of storage and > > fetch protection override. > > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> > > --- > > s390x/skey.c | 285 ++++++++++++++++++++++++++++++++++++++++++++ > > s390x/unittests.cfg | 1 + > > 2 files changed, 286 insertions(+) > > > > diff --git a/s390x/skey.c b/s390x/skey.c > > index 19fa5721..60ae8158 100644 > > --- a/s390x/skey.c > > +++ b/s390x/skey.c > > @@ -12,6 +12,7 @@ > > #include <asm/asm-offsets.h> > > #include <asm/interrupt.h> > > #include <vmalloc.h> > > +#include <css.h> > > #include <asm/page.h> > > #include <asm/facility.h> > > #include <asm/mem.h> > > @@ -284,6 +285,115 @@ static void test_store_cpu_address(void) > > report_prefix_pop(); > > } > > > > +/* > > + * Perform CHANNEL SUBSYSTEM CALL (CHSC) instruction while temporarily executing > > + * with access key 1. > > + */ > > +static unsigned int chsc_key_1(void *comm_block) > > +{ > > + uint32_t program_mask; > > + > > + asm volatile ( > > + "spka 0x10\n\t" > > + ".insn rre,0xb25f0000,%[comm_block],0\n\t" > > + "spka 0\n\t" > > + "ipm %[program_mask]\n" > > + : [program_mask] "=d" (program_mask) > > + : [comm_block] "d" (comm_block) > > + : "memory" > > + ); > > + return program_mask >> 28; > > +} > > + > > +static const char chsc_msg[] = "Performed store-channel-subsystem-characteristics"; > > +static void init_comm_block(uint16_t *comm_block) > > +{ > > + memset(comm_block, 0, PAGE_SIZE); > > + /* store-channel-subsystem-characteristics command */ > > + comm_block[0] = 0x10; > > + comm_block[1] = 0x10; > > + comm_block[9] = 0; > > +} > > + > > +static void test_channel_subsystem_call(void) > > +{ > > + uint16_t *comm_block = (uint16_t *)&pagebuf; > > + unsigned int cc; > > + > > + report_prefix_push("CHANNEL SUBSYSTEM CALL"); > > + > > + report_prefix_push("zero key"); > > + init_comm_block(comm_block); > > + set_storage_key(comm_block, 0x10, 0); > > + asm volatile ( > > + ".insn rre,0xb25f0000,%[comm_block],0\n\t" > > + "ipm %[cc]\n" > > + : [cc] "=d" (cc) > > + : [comm_block] "d" (comm_block) > > + : "memory" > > + ); > > + cc = cc >> 28; > > + report(cc == 0 && comm_block[9], chsc_msg); > > + report_prefix_pop(); > > + > > + report_prefix_push("matching key"); > > + init_comm_block(comm_block); > > + set_storage_key(comm_block, 0x10, 0); > > + cc = chsc_key_1(comm_block); > > + report(cc == 0 && comm_block[9], chsc_msg); > > + report_prefix_pop(); > > + > > + report_prefix_push("mismatching key"); > > + > > + report_prefix_push("no fetch protection"); > > + init_comm_block(comm_block); > > + set_storage_key(comm_block, 0x20, 0); > > + expect_pgm_int(); > > + chsc_key_1(comm_block); > > + check_key_prot_exc(ACC_UPDATE, PROT_STORE); > > I wonder if ACC_UPDATE is really needed here? you should clearly never > get a read error, right? Maybe the naming isn't great, the first argument specifies the access if it weren't for protection, not the access actually taking place. If a read is indicated, that will cause a test failure. You could use ACC_STORE, but that would be misleading, because it does do a fetch. > > > + report_prefix_pop(); > > + > > + report_prefix_push("fetch protection"); > > + init_comm_block(comm_block); > > + set_storage_key(comm_block, 0x28, 0); > > + expect_pgm_int(); > > + chsc_key_1(comm_block); > > + check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE); > > and here, I guess you would wait for a read error? or is it actually > defined as unpredictable? Unpredictable, kvm and LPAR do different things IIRC, that's why I had the report_info. > > (same for all ACC_UPDATE below) [...] > > > > +/* > > + * Perform MODIFY SUBCHANNEL (MSCH) instruction while temporarily executing > > + * with access key 1. > > + */ > > +static uint32_t modify_subchannel_key_1(uint32_t sid, struct schib *schib) > > +{ > > + uint32_t program_mask; > > + > > +/* > > + * gcc 12.0.1 warns if schib is < 4k. > > + * We need such addresses to test fetch protection override. > > + */ > > +#pragma GCC diagnostic push > > +#pragma GCC diagnostic ignored "-Warray-bounds" > > I really dislike these pragmas > > can we find a nicer way? I'll do what ever we decide on in the other patch series. > > > + asm volatile ( > > + "lr %%r1,%[sid]\n\t" > > + "spka 0x10\n\t" > > + "msch %[schib]\n\t" > > + "spka 0\n\t" > > + "ipm %[program_mask]\n" > > + : [program_mask] "=d" (program_mask) > > + : [sid] "d" (sid), > > + [schib] "Q" (*schib) > > + : "%r1" > > + ); > > +#pragma GCC diagnostic pop > > + return program_mask >> 28; > > +} > > + [...] Thanks for the review, also for the other patch.