Re: [kvm-unit-tests PATCH v2 3/4] s390x: Test effect of storage keys on some more instructions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux