diff mbox series

[kvm-unit-tests,3/3] s390x: Test effect of storage keys on some more instructions

Message ID 20220505124656.1954092-4-scgl@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: More storage key instruction | expand

Commit Message

Janis Schoetterl-Glausch May 5, 2022, 12:46 p.m. UTC
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@linux.ibm.com>
---
 s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg |   1 +
 2 files changed, 278 insertions(+)

Comments

Claudio Imbrenda May 6, 2022, 4:52 p.m. UTC | #1
On Thu,  5 May 2022 14:46:56 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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@linux.ibm.com>
> ---
>  s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg |   1 +
>  2 files changed, 278 insertions(+)
> 
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 56bf5f45..d50470a8 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,114 @@ 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 channel_subsystem_call_key_1(void *communication_block)

this function name is very long (maybe chsc_with_key_1 instead?)

> +{
> +	uint32_t program_mask;
> +
> +	asm volatile (
> +		"spka	0x10\n\t"
> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
> +		"spka	0\n\t"
> +		"ipm	%[program_mask]\n"
> +		: [program_mask] "=d" (program_mask)
> +		: [communication_block] "d" (communication_block)
> +		: "memory"
> +	);
> +	return program_mask >> 28;
> +}
> +
> +static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)

same here (init_comm_block?)

> +{
> +	memset(communication_block, 0, PAGE_SIZE);
> +	communication_block[0] = 0x10;
> +	communication_block[1] = 0x10;
> +	communication_block[9] = 0;
> +}
> +
> +static void test_channel_subsystem_call(void)
> +{
> +	static const char request_name[] = "Store channel-subsystem-characteristics";

so this "request_name" is for when CHSC succeeds? why not just
"Success" then?

> +	uint16_t *communication_block = (uint16_t *)&pagebuf;

long name (consider comm_block, or even cb)

> +	unsigned int cc;
> +
> +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
> +
> +	report_prefix_push("zero key");
> +	init_store_channel_subsystem_characteristics(communication_block);

see what I mean when I say that the names are too long? ^

> +	set_storage_key(communication_block, 0x10, 0);
> +	asm volatile (
> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
> +		"ipm	%[cc]\n"
> +		: [cc] "=d" (cc)
> +		: [communication_block] "d" (communication_block)
> +		: "memory"
> +	);
> +	cc = cc >> 28;
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x10, 0);

you just set the storage key in the previous test, and you did not set
it back to 0, why do you need to set it again?

> +	cc = channel_subsystem_call_key_1(communication_block);
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x20, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_push("fetch protection");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x28, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);
> +	report_prefix_pop();
> +
> +	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override, invalid key");
> +	set_storage_key(communication_block, 0x20, 0);
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_push("storage-protection override, override key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x90, 0);
> +	cc = channel_subsystem_call_key_1(communication_block);
> +	report(cc == 0 && communication_block[9], request_name);
> +	report_prefix_pop();
> +
> +	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("storage-protection override disabled, override key");
> +	init_store_channel_subsystem_characteristics(communication_block);
> +	set_storage_key(communication_block, 0x90, 0);
> +	expect_pgm_int();
> +	channel_subsystem_call_key_1(communication_block);
> +	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
> +	report_prefix_pop();
> +
> +	report_prefix_pop();
> +
> +	set_storage_key(communication_block, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  /*
>   * Perform SET PREFIX (SPX) instruction while temporarily executing
>   * with access key 1.
> @@ -410,6 +519,172 @@ static void test_set_prefix(void)
>  	report_prefix_pop();
>  }
>  
> +/*
> + * 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;
> +
> +	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"
> +	);
> +	return program_mask >> 28;
> +}
> +
> +static void test_msch(void)
> +{
> +	struct schib *schib = (struct schib *)pagebuf;
> +	struct schib *no_override_schib;
> +	int test_device_sid;
> +	pgd_t *root;
> +	int cc;
> +
> +	report_prefix_push("MSCH");
> +	root = (pgd_t *)(stctg(1) & PAGE_MASK);
> +	test_device_sid = css_enumerate();
> +
> +	if (!(test_device_sid & SCHID_ONE)) {
> +		report_fail("no I/O device found");
> +		return;
> +	}
> +
> +	cc = stsch(test_device_sid, schib);
> +	if (cc) {
> +		report_fail("could not store SCHIB");
> +		return;
> +	}
> +
> +	report_prefix_push("zero key");
> +	schib->pmcw.intparm = 100;
> +	set_storage_key(schib, 0x28, 0);
> +	cc = msch(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);

why are you using WRITE_ONCE here?

> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	schib->pmcw.intparm = 200;
> +	set_storage_key(schib, 0x18, 0);
> +	cc = modify_subchannel_key_1(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 200, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	schib->pmcw.intparm = 300;
> +	set_storage_key(schib, 0x20, 0);
> +	cc = modify_subchannel_key_1(test_device_sid, schib);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 300, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("fetch protection");
> +		schib->pmcw.intparm = 400;
> +		set_storage_key(schib, 0x28, 0);
> +		expect_pgm_int();
> +		modify_subchannel_key_1(test_device_sid, schib);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	register_pgm_cleanup_func(dat_fixup_pgm_int);
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("remapped page, fetch protection");
> +		schib->pmcw.intparm = 500;
> +		set_storage_key(pagebuf, 0x28, 0);
> +		expect_pgm_int();
> +		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +		modify_subchannel_key_1(test_device_sid, (struct schib *)0);
> +		install_page(root, 0, 0);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("fetch-protection override applies");
> +	schib->pmcw.intparm = 600;
> +	set_storage_key(pagebuf, 0x28, 0);
> +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +	cc = modify_subchannel_key_1(test_device_sid, (struct schib *)0);
> +	install_page(root, 0, 0);
> +	if (!cc) {
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 600, "fetched from SCHIB");
> +	} else {
> +		report_fail("MSCH cc != 0");
> +	}
> +	report_prefix_pop();
> +
> +	schib->pmcw.intparm = 0;
> +	if (!msch(test_device_sid, schib)) {
> +		report_prefix_push("fetch-protection override does not apply");
> +		schib->pmcw.intparm = 700;
> +		no_override_schib = (struct schib *)(pagebuf + 2048);
> +		memcpy(no_override_schib, schib, sizeof(struct schib));
> +		set_storage_key(pagebuf, 0x28, 0);
> +		expect_pgm_int();
> +		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +		modify_subchannel_key_1(test_device_sid, (struct schib *)2048);
> +		install_page(root, 0, 0);
> +		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> +		cc = stsch(test_device_sid, schib);
> +		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
> +		report_prefix_pop();
> +	} else {
> +		report_fail("could not reset SCHIB");
> +	}
> +
> +	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +	register_pgm_cleanup_func(NULL);
> +	report_prefix_pop();
> +	set_storage_key(schib, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("skey");
> @@ -424,9 +699,11 @@ int main(void)
>  	test_chg();
>  	test_test_protection();
>  	test_store_cpu_address();
> +	test_channel_subsystem_call();
>  
>  	setup_vm();
>  	test_set_prefix();
> +	test_msch();
>  done:
>  	report_prefix_pop();
>  	return report_summary();
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 743013b2..069c41a7 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -41,6 +41,7 @@ file = sthyi.elf
>  
>  [skey]
>  file = skey.elf
> +extra_params = -device virtio-net-ccw
>  
>  [diag10]
>  file = diag10.elf
Janis Schoetterl-Glausch May 9, 2022, 2:07 p.m. UTC | #2
On 5/6/22 18:52, Claudio Imbrenda wrote:
> On Thu,  5 May 2022 14:46:56 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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@linux.ibm.com>
>> ---
>>  s390x/skey.c        | 277 ++++++++++++++++++++++++++++++++++++++++++++
>>  s390x/unittests.cfg |   1 +
>>  2 files changed, 278 insertions(+)
>>
>> diff --git a/s390x/skey.c b/s390x/skey.c
>> index 56bf5f45..d50470a8 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,114 @@ 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 channel_subsystem_call_key_1(void *communication_block)
> 
> this function name is very long (maybe chsc_with_key_1 instead?)

It's because of consistency with set_prefix_key_1 where I spelled out the instruction
name too. Granted the name of chsc is longer.
When in doubt I go for what seems more readable/contains more information.
> 
>> +{
>> +	uint32_t program_mask;
>> +
>> +	asm volatile (
>> +		"spka	0x10\n\t"
>> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
>> +		"spka	0\n\t"
>> +		"ipm	%[program_mask]\n"
>> +		: [program_mask] "=d" (program_mask)
>> +		: [communication_block] "d" (communication_block)
>> +		: "memory"
>> +	);
>> +	return program_mask >> 28;
>> +}
>> +
>> +static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
> 
> same here (init_comm_block?)

Since we're only performing one kind of operation in this test, that is fine,
but I'll add a comment saying how we initialize the communication block then.
> 
>> +{
>> +	memset(communication_block, 0, PAGE_SIZE);
>> +	communication_block[0] = 0x10;
>> +	communication_block[1] = 0x10;
>> +	communication_block[9] = 0;
>> +}
>> +
>> +static void test_channel_subsystem_call(void)
>> +{
>> +	static const char request_name[] = "Store channel-subsystem-characteristics";
> 
> so this "request_name" is for when CHSC succeeds? why not just
> "Success" then?

That's the operation being performed. So maybe I should change it to
msg[] = "Performed store channel-subsystem-characteristics" ?
> 
>> +	uint16_t *communication_block = (uint16_t *)&pagebuf;
> 
> long name (consider comm_block, or even cb)
> 
>> +	unsigned int cc;
>> +
>> +	report_prefix_push("CHANNEL SUBSYSTEM CALL");
>> +
>> +	report_prefix_push("zero key");
>> +	init_store_channel_subsystem_characteristics(communication_block);
> 
> see what I mean when I say that the names are too long? ^

Fits in 80 columns ;-)
> 
>> +	set_storage_key(communication_block, 0x10, 0);
>> +	asm volatile (
>> +		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
>> +		"ipm	%[cc]\n"
>> +		: [cc] "=d" (cc)
>> +		: [communication_block] "d" (communication_block)
>> +		: "memory"
>> +	);
>> +	cc = cc >> 28;
>> +	report(cc == 0 && communication_block[9], request_name);
>> +	report_prefix_pop();
>> +
>> +	report_prefix_push("matching key");
>> +	init_store_channel_subsystem_characteristics(communication_block);
>> +	set_storage_key(communication_block, 0x10, 0);
> 
> you just set the storage key in the previous test, and you did not set
> it back to 0, why do you need to set it again?

It's not necessary, but I want the tests to be independent from each other,
so you can remove/reorder/add ones without having to think.
> 
>> +	cc = channel_subsystem_call_key_1(communication_block);
>> +	report(cc == 0 && communication_block[9], request_name);
>> +	report_prefix_pop();
>> +

[...]

>> +
>> +	cc = stsch(test_device_sid, schib);
>> +	if (cc) {
>> +		report_fail("could not store SCHIB");
>> +		return;
>> +	}
>> +
>> +	report_prefix_push("zero key");
>> +	schib->pmcw.intparm = 100;
>> +	set_storage_key(schib, 0x28, 0);
>> +	cc = msch(test_device_sid, schib);
>> +	if (!cc) {
>> +		WRITE_ONCE(schib->pmcw.intparm, 0);
> 
> why are you using WRITE_ONCE here?

It's a dead store because of the stsch below.
That line is just for good measure so we know stsch really overwrote the value.
> 
>> +		cc = stsch(test_device_sid, schib);
>> +		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");

[...]
diff mbox series

Patch

diff --git a/s390x/skey.c b/s390x/skey.c
index 56bf5f45..d50470a8 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,114 @@  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 channel_subsystem_call_key_1(void *communication_block)
+{
+	uint32_t program_mask;
+
+	asm volatile (
+		"spka	0x10\n\t"
+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
+		"spka	0\n\t"
+		"ipm	%[program_mask]\n"
+		: [program_mask] "=d" (program_mask)
+		: [communication_block] "d" (communication_block)
+		: "memory"
+	);
+	return program_mask >> 28;
+}
+
+static void init_store_channel_subsystem_characteristics(uint16_t *communication_block)
+{
+	memset(communication_block, 0, PAGE_SIZE);
+	communication_block[0] = 0x10;
+	communication_block[1] = 0x10;
+	communication_block[9] = 0;
+}
+
+static void test_channel_subsystem_call(void)
+{
+	static const char request_name[] = "Store channel-subsystem-characteristics";
+	uint16_t *communication_block = (uint16_t *)&pagebuf;
+	unsigned int cc;
+
+	report_prefix_push("CHANNEL SUBSYSTEM CALL");
+
+	report_prefix_push("zero key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x10, 0);
+	asm volatile (
+		".insn	rre,0xb25f0000,%[communication_block],0\n\t"
+		"ipm	%[cc]\n"
+		: [cc] "=d" (cc)
+		: [communication_block] "d" (communication_block)
+		: "memory"
+	);
+	cc = cc >> 28;
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x10, 0);
+	cc = channel_subsystem_call_key_1(communication_block);
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x20, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("fetch protection");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x28, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_FETCH_STORE);
+	report_prefix_pop();
+
+	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	report_prefix_push("storage-protection override, invalid key");
+	set_storage_key(communication_block, 0x20, 0);
+	init_store_channel_subsystem_characteristics(communication_block);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_push("storage-protection override, override key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x90, 0);
+	cc = channel_subsystem_call_key_1(communication_block);
+	report(cc == 0 && communication_block[9], request_name);
+	report_prefix_pop();
+
+	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	report_prefix_push("storage-protection override disabled, override key");
+	init_store_channel_subsystem_characteristics(communication_block);
+	set_storage_key(communication_block, 0x90, 0);
+	expect_pgm_int();
+	channel_subsystem_call_key_1(communication_block);
+	check_key_prot_exc(ACC_UPDATE, PROT_STORE);
+	report_prefix_pop();
+
+	report_prefix_pop();
+
+	set_storage_key(communication_block, 0x00, 0);
+	report_prefix_pop();
+}
+
 /*
  * Perform SET PREFIX (SPX) instruction while temporarily executing
  * with access key 1.
@@ -410,6 +519,172 @@  static void test_set_prefix(void)
 	report_prefix_pop();
 }
 
+/*
+ * 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;
+
+	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"
+	);
+	return program_mask >> 28;
+}
+
+static void test_msch(void)
+{
+	struct schib *schib = (struct schib *)pagebuf;
+	struct schib *no_override_schib;
+	int test_device_sid;
+	pgd_t *root;
+	int cc;
+
+	report_prefix_push("MSCH");
+	root = (pgd_t *)(stctg(1) & PAGE_MASK);
+	test_device_sid = css_enumerate();
+
+	if (!(test_device_sid & SCHID_ONE)) {
+		report_fail("no I/O device found");
+		return;
+	}
+
+	cc = stsch(test_device_sid, schib);
+	if (cc) {
+		report_fail("could not store SCHIB");
+		return;
+	}
+
+	report_prefix_push("zero key");
+	schib->pmcw.intparm = 100;
+	set_storage_key(schib, 0x28, 0);
+	cc = msch(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 100, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	schib->pmcw.intparm = 200;
+	set_storage_key(schib, 0x18, 0);
+	cc = modify_subchannel_key_1(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 200, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	schib->pmcw.intparm = 300;
+	set_storage_key(schib, 0x20, 0);
+	cc = modify_subchannel_key_1(test_device_sid, schib);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 300, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("fetch protection");
+		schib->pmcw.intparm = 400;
+		set_storage_key(schib, 0x28, 0);
+		expect_pgm_int();
+		modify_subchannel_key_1(test_device_sid, schib);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	register_pgm_cleanup_func(dat_fixup_pgm_int);
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("remapped page, fetch protection");
+		schib->pmcw.intparm = 500;
+		set_storage_key(pagebuf, 0x28, 0);
+		expect_pgm_int();
+		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+		modify_subchannel_key_1(test_device_sid, (struct schib *)0);
+		install_page(root, 0, 0);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+
+	report_prefix_push("fetch-protection override applies");
+	schib->pmcw.intparm = 600;
+	set_storage_key(pagebuf, 0x28, 0);
+	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+	cc = modify_subchannel_key_1(test_device_sid, (struct schib *)0);
+	install_page(root, 0, 0);
+	if (!cc) {
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 600, "fetched from SCHIB");
+	} else {
+		report_fail("MSCH cc != 0");
+	}
+	report_prefix_pop();
+
+	schib->pmcw.intparm = 0;
+	if (!msch(test_device_sid, schib)) {
+		report_prefix_push("fetch-protection override does not apply");
+		schib->pmcw.intparm = 700;
+		no_override_schib = (struct schib *)(pagebuf + 2048);
+		memcpy(no_override_schib, schib, sizeof(struct schib));
+		set_storage_key(pagebuf, 0x28, 0);
+		expect_pgm_int();
+		install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+		modify_subchannel_key_1(test_device_sid, (struct schib *)2048);
+		install_page(root, 0, 0);
+		check_key_prot_exc(ACC_FETCH, PROT_FETCH_STORE);
+		WRITE_ONCE(schib->pmcw.intparm, 0);
+		cc = stsch(test_device_sid, schib);
+		report(!cc && schib->pmcw.intparm == 0, "did not modify subchannel");
+		report_prefix_pop();
+	} else {
+		report_fail("could not reset SCHIB");
+	}
+
+	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+	register_pgm_cleanup_func(NULL);
+	report_prefix_pop();
+	set_storage_key(schib, 0x00, 0);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skey");
@@ -424,9 +699,11 @@  int main(void)
 	test_chg();
 	test_test_protection();
 	test_store_cpu_address();
+	test_channel_subsystem_call();
 
 	setup_vm();
 	test_set_prefix();
+	test_msch();
 done:
 	report_prefix_pop();
 	return report_summary();
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 743013b2..069c41a7 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -41,6 +41,7 @@  file = sthyi.elf
 
 [skey]
 file = skey.elf
+extra_params = -device virtio-net-ccw
 
 [diag10]
 file = diag10.elf