[kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[Janis Schoetterl-Glausch]

Improve readability by making the return value of tprot() an enum.

No functional change intended.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 11 +++++++++--
 lib/s390x/sclp.c         |  6 +++---
 s390x/tprot.c            | 24 ++++++++++++------------
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index bab3c374..46c370e6 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -228,7 +228,14 @@ static inline uint64_t stidp(void)
 	return cpuid;
 }
 
-static inline int tprot(unsigned long addr, char access_key)
+enum tprot_permission {
+	TPROT_READ_WRITE = 0,
+	TPROT_READ = 1,
+	TPROT_RW_PROTECTED = 2,
+	TPROT_TRANSL_UNAVAIL = 3,
+};
+
+static inline enum tprot_permission tprot(unsigned long addr, char access_key)
 {
 	int cc;
 
@@ -237,7 +244,7 @@ static inline int tprot(unsigned long addr, char access_key)
 		"	ipm	%0\n"
 		"	srl	%0,28\n"
 		: "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
-	return cc;
+	return (enum tprot_permission)cc;
 }
 
 static inline void lctlg(int cr, uint64_t value)
diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
index 33985eb4..b8204c5f 100644
--- a/lib/s390x/sclp.c
+++ b/lib/s390x/sclp.c
@@ -198,7 +198,7 @@ int sclp_service_call(unsigned int command, void *sccb)
 void sclp_memory_setup(void)
 {
 	uint64_t rnmax, rnsize;
-	int cc;
+	enum tprot_permission permission;
 
 	assert(read_info);
 
@@ -222,9 +222,9 @@ void sclp_memory_setup(void)
 	/* probe for r/w memory up to max memory size */
 	while (ram_size < max_ram_size) {
 		expect_pgm_int();
-		cc = tprot(ram_size + storage_increment_size - 1, 0);
+		permission = tprot(ram_size + storage_increment_size - 1, 0);
 		/* stop once we receive an exception or have protected memory */
-		if (clear_pgm_int() || cc != 0)
+		if (clear_pgm_int() || permission != TPROT_READ_WRITE)
 			break;
 		ram_size += storage_increment_size;
 	}
diff --git a/s390x/tprot.c b/s390x/tprot.c
index 460a0db7..8eb91c18 100644
--- a/s390x/tprot.c
+++ b/s390x/tprot.c
@@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
 
 static void test_tprot_rw(void)
 {
-	int cc;
+	enum tprot_permission permission;
 
 	report_prefix_push("Page read/writeable");
 
-	cc = tprot((unsigned long)pagebuf, 0);
-	report(cc == 0, "CC = 0");
+	permission = tprot((unsigned long)pagebuf, 0);
+	report(permission == TPROT_READ_WRITE, "CC = 0");
 
 	report_prefix_pop();
 }
 
 static void test_tprot_ro(void)
 {
-	int cc;
+	enum tprot_permission permission;
 
 	report_prefix_push("Page readonly");
 
 	protect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
 
-	cc = tprot((unsigned long)pagebuf, 0);
-	report(cc == 1, "CC = 1");
+	permission = tprot((unsigned long)pagebuf, 0);
+	report(permission == TPROT_READ, "CC = 1");
 
 	unprotect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
 
@@ -48,28 +48,28 @@ static void test_tprot_ro(void)
 
 static void test_tprot_low_addr_prot(void)
 {
-	int cc;
+	enum tprot_permission permission;
 
 	report_prefix_push("low-address protection");
 
 	low_prot_enable();
-	cc = tprot(0, 0);
+	permission = tprot(0, 0);
 	low_prot_disable();
-	report(cc == 1, "CC = 1");
+	report(permission == TPROT_READ, "CC = 1");
 
 	report_prefix_pop();
 }
 
 static void test_tprot_transl_unavail(void)
 {
-	int cc;
+	enum tprot_permission permission;
 
 	report_prefix_push("Page translation unavailable");
 
 	protect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
 
-	cc = tprot((unsigned long)pagebuf, 0);
-	report(cc == 3, "CC = 3");
+	permission = tprot((unsigned long)pagebuf, 0);
+	report(permission == TPROT_TRANSL_UNAVAIL, "CC = 3");
 
 	unprotect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
 
-- 
2.33.1

Re: [kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[Claudio Imbrenda]

On Wed, 27 Apr 2022 12:06:09 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> Improve readability by making the return value of tprot() an enum.
> 
> No functional change intended.

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

but see nit below

> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 11 +++++++++--
>  lib/s390x/sclp.c         |  6 +++---
>  s390x/tprot.c            | 24 ++++++++++++------------
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bab3c374..46c370e6 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -228,7 +228,14 @@ static inline uint64_t stidp(void)
>  	return cpuid;
>  }
>  
> -static inline int tprot(unsigned long addr, char access_key)
> +enum tprot_permission {
> +	TPROT_READ_WRITE = 0,
> +	TPROT_READ = 1,
> +	TPROT_RW_PROTECTED = 2,
> +	TPROT_TRANSL_UNAVAIL = 3,
> +};
> +
> +static inline enum tprot_permission tprot(unsigned long addr, char access_key)
>  {
>  	int cc;
>  
> @@ -237,7 +244,7 @@ static inline int tprot(unsigned long addr, char access_key)
>  		"	ipm	%0\n"
>  		"	srl	%0,28\n"
>  		: "=d" (cc) : "a" (addr), "a" (access_key << 4) : "cc");
> -	return cc;
> +	return (enum tprot_permission)cc;
>  }
>  
>  static inline void lctlg(int cr, uint64_t value)
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index 33985eb4..b8204c5f 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -198,7 +198,7 @@ int sclp_service_call(unsigned int command, void *sccb)
>  void sclp_memory_setup(void)
>  {
>  	uint64_t rnmax, rnsize;
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	assert(read_info);
>  
> @@ -222,9 +222,9 @@ void sclp_memory_setup(void)
>  	/* probe for r/w memory up to max memory size */
>  	while (ram_size < max_ram_size) {
>  		expect_pgm_int();
> -		cc = tprot(ram_size + storage_increment_size - 1, 0);
> +		permission = tprot(ram_size + storage_increment_size - 1, 0);
>  		/* stop once we receive an exception or have protected memory */
> -		if (clear_pgm_int() || cc != 0)
> +		if (clear_pgm_int() || permission != TPROT_READ_WRITE)
>  			break;
>  		ram_size += storage_increment_size;
>  	}
> diff --git a/s390x/tprot.c b/s390x/tprot.c
> index 460a0db7..8eb91c18 100644
> --- a/s390x/tprot.c
> +++ b/s390x/tprot.c
> @@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>  
>  static void test_tprot_rw(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page read/writeable");
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 0, "CC = 0");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_READ_WRITE, "CC = 0");

here and in all similar cases below: does it still make sense to have
"CC = 0" as message at this point? Maybe a more descriptive one would
be better

>  
>  	report_prefix_pop();
>  }
>  
>  static void test_tprot_ro(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page readonly");
>  
>  	protect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 1, "CC = 1");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_READ, "CC = 1");
>  
>  	unprotect_dat_entry(pagebuf, PAGE_ENTRY_P, 5);
>  
> @@ -48,28 +48,28 @@ static void test_tprot_ro(void)
>  
>  static void test_tprot_low_addr_prot(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("low-address protection");
>  
>  	low_prot_enable();
> -	cc = tprot(0, 0);
> +	permission = tprot(0, 0);
>  	low_prot_disable();
> -	report(cc == 1, "CC = 1");
> +	report(permission == TPROT_READ, "CC = 1");
>  
>  	report_prefix_pop();
>  }
>  
>  static void test_tprot_transl_unavail(void)
>  {
> -	int cc;
> +	enum tprot_permission permission;
>  
>  	report_prefix_push("Page translation unavailable");
>  
>  	protect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
>  
> -	cc = tprot((unsigned long)pagebuf, 0);
> -	report(cc == 3, "CC = 3");
> +	permission = tprot((unsigned long)pagebuf, 0);
> +	report(permission == TPROT_TRANSL_UNAVAIL, "CC = 3");
>  
>  	unprotect_dat_entry(pagebuf, PAGE_ENTRY_I, 5);
>  

Re: [kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[Janis Schoetterl-Glausch]

On 4/27/22 13:14, Claudio Imbrenda wrote:
> On Wed, 27 Apr 2022 12:06:09 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> Improve readability by making the return value of tprot() an enum.
>>
>> No functional change intended.
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> but see nit below
> 
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>  lib/s390x/asm/arch_def.h | 11 +++++++++--
>>  lib/s390x/sclp.c         |  6 +++---
>>  s390x/tprot.c            | 24 ++++++++++++------------
>>  3 files changed, 24 insertions(+), 17 deletions(-)

[...]

>> diff --git a/s390x/tprot.c b/s390x/tprot.c
>> index 460a0db7..8eb91c18 100644
>> --- a/s390x/tprot.c
>> +++ b/s390x/tprot.c
>> @@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>>  
>>  static void test_tprot_rw(void)
>>  {
>> -	int cc;
>> +	enum tprot_permission permission;
>>  
>>  	report_prefix_push("Page read/writeable");
>>  
>> -	cc = tprot((unsigned long)pagebuf, 0);
>> -	report(cc == 0, "CC = 0");
>> +	permission = tprot((unsigned long)pagebuf, 0);
>> +	report(permission == TPROT_READ_WRITE, "CC = 0");
> 
> here and in all similar cases below: does it still make sense to have
> "CC = 0" as message at this point? Maybe a more descriptive one would
> be better

I thought about it, but decided against it. Firstly, because I preferred
not to do any functional changes and secondly, I could not think of anything
better. The prefix already tells you the meaning of the cc, so I don't know
what to print that would not be redundant.

[...]

Re: [kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[Claudio Imbrenda]

On Wed, 27 Apr 2022 14:04:52 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On 4/27/22 13:14, Claudio Imbrenda wrote:
> > On Wed, 27 Apr 2022 12:06:09 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> >> Improve readability by making the return value of tprot() an enum.
> >>
> >> No functional change intended.  
> > 
> > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > 
> > but see nit below
> >   
> >>
> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> >> ---
> >>  lib/s390x/asm/arch_def.h | 11 +++++++++--
> >>  lib/s390x/sclp.c         |  6 +++---
> >>  s390x/tprot.c            | 24 ++++++++++++------------
> >>  3 files changed, 24 insertions(+), 17 deletions(-)  
> 
> [...]
> 
> >> diff --git a/s390x/tprot.c b/s390x/tprot.c
> >> index 460a0db7..8eb91c18 100644
> >> --- a/s390x/tprot.c
> >> +++ b/s390x/tprot.c
> >> @@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> >>  
> >>  static void test_tprot_rw(void)
> >>  {
> >> -	int cc;
> >> +	enum tprot_permission permission;
> >>  
> >>  	report_prefix_push("Page read/writeable");
> >>  
> >> -	cc = tprot((unsigned long)pagebuf, 0);
> >> -	report(cc == 0, "CC = 0");
> >> +	permission = tprot((unsigned long)pagebuf, 0);
> >> +	report(permission == TPROT_READ_WRITE, "CC = 0");  
> > 
> > here and in all similar cases below: does it still make sense to have
> > "CC = 0" as message at this point? Maybe a more descriptive one would
> > be better  
> 
> I thought about it, but decided against it. Firstly, because I preferred
> not to do any functional changes and secondly, I could not think of anything
> better. The prefix already tells you the meaning of the cc, so I don't know
> what to print that would not be redundant.
> 
> [...]

fair enough

Re: [kvm-unit-tests PATCH v6 1/3] s390x: Give name to return value of tprot()

[Janosch Frank]

On 4/27/22 14:04, Janis Schoetterl-Glausch wrote:
> On 4/27/22 13:14, Claudio Imbrenda wrote:
>> On Wed, 27 Apr 2022 12:06:09 +0200
>> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>
>>> Improve readability by making the return value of tprot() an enum.
>>>
>>> No functional change intended.
>>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>
>> but see nit below
>>
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>> ---
>>>   lib/s390x/asm/arch_def.h | 11 +++++++++--
>>>   lib/s390x/sclp.c         |  6 +++---
>>>   s390x/tprot.c            | 24 ++++++++++++------------
>>>   3 files changed, 24 insertions(+), 17 deletions(-)
> 
> [...]
> 
>>> diff --git a/s390x/tprot.c b/s390x/tprot.c
>>> index 460a0db7..8eb91c18 100644
>>> --- a/s390x/tprot.c
>>> +++ b/s390x/tprot.c
>>> @@ -20,26 +20,26 @@ static uint8_t pagebuf[PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
>>>   
>>>   static void test_tprot_rw(void)
>>>   {
>>> -	int cc;
>>> +	enum tprot_permission permission;
>>>   
>>>   	report_prefix_push("Page read/writeable");
>>>   
>>> -	cc = tprot((unsigned long)pagebuf, 0);
>>> -	report(cc == 0, "CC = 0");
>>> +	permission = tprot((unsigned long)pagebuf, 0);
>>> +	report(permission == TPROT_READ_WRITE, "CC = 0");
>>
>> here and in all similar cases below: does it still make sense to have
>> "CC = 0" as message at this point? Maybe a more descriptive one would
>> be better
> 
> I thought about it, but decided against it. Firstly, because I preferred
> not to do any functional changes and secondly, I could not think of anything
> better. The prefix already tells you the meaning of the cc, so I don't know
> what to print that would not be redundant.
> 
> [...]

I'm ok with that for now especially considering we're at v6 already and 
functionally this looks good.

Let's add the series to the devel branch so the CI can have a look at it 
before we pick it.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com >

[kvm-unit-tests PATCH v6 2/3] s390x: Test effect of storage keys on some instructions

[Janis Schoetterl-Glausch]

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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h |  20 ++--
 s390x/skey.c             | 236 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 247 insertions(+), 9 deletions(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 46c370e6..72553819 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 edad53e9..46efe3f5 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>
@@ -118,6 +119,236 @@ 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) == TPROT_READ_WRITE, "access key 0 -> no protection");
+	report(tprot(addr, 1) == TPROT_READ_WRITE, "access key matches -> no protection");
+	report(tprot(addr, 2) == TPROT_READ,
+	       "access key mismatches, no fetch protection -> store protection");
+
+	set_storage_key(pagebuf, 0x18, 0);
+	report(tprot(addr, 2) == TPROT_RW_PROTECTED,
+	       "access key mismatches, fetch protection -> fetch & store protection");
+
+	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+	set_storage_key(pagebuf, 0x90, 0);
+	report(tprot(addr, 2) == TPROT_READ_WRITE,
+	       "access key mismatches, storage protection override -> no protection");
+	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
+
+	set_storage_key(pagebuf, 0x00, 0);
+	report_prefix_pop();
+}
+
+/*
+ * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
+ * with access key 1.
+ */
+static void store_cpu_address_key_1(uint16_t *out)
+{
+	asm volatile (
+		"spka	0x10\n\t"
+		"stap	%0\n\t"
+		"spka	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;
+
+	report_prefix_push("STORE CPU ADDRESS");
+	asm ("stap %0" : "=Q" (cpu_addr));
+
+	report_prefix_push("zero key");
+	set_storage_key(pagebuf, 0x20, 0);
+	WRITE_ONCE(*out, 0xbeef);
+	asm ("stap %0" : "=Q" (*out));
+	report(*out == cpu_addr, "store occurred");
+	report_prefix_pop();
+
+	report_prefix_push("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("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("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("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);
+
+	report_prefix_push("storage-protection override disabled, override key");
+	set_storage_key(pagebuf, 0x90, 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();
+
+	set_storage_key(pagebuf, 0x00, 0);
+	report_prefix_pop();
+}
+
+/*
+ * Perform SET PREFIX (SPX) instruction while temporarily executing
+ * with access key 1.
+ */
+static void set_prefix_key_1(uint32_t *prefix_ptr)
+{
+	asm volatile (
+		"spka	0x10\n\t"
+		"spx	%0\n\t"
+		"spka	0\n"
+	     :: "Q" (*prefix_ptr)
+	);
+}
+
+/*
+ * We remapped page 0, making the lowcore inaccessible, which breaks the normal
+ * handler 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);
+}
+
+#define PREFIX_AREA_SIZE (PAGE_SIZE * 2)
+static char lowcore_tmp[PREFIX_AREA_SIZE] __attribute__((aligned(PREFIX_AREA_SIZE)));
+
+/*
+ * Test accessibility of the operand to SET PREFIX given different configurations
+ * with regards to storage keys. That is, check the accessibility of the location
+ * holding the new prefix, not that of the new prefix area. The new prefix area
+ * is a valid lowcore, so that the test does not crash on failure.
+ */
+static void test_set_prefix(void)
+{
+	uint32_t *prefix_ptr = (uint32_t *)pagebuf;
+	uint32_t *no_override_prefix_ptr;
+	uint32_t old_prefix;
+	pgd_t *root;
+
+	report_prefix_push("SET PREFIX");
+	root = (pgd_t *)(stctg(1) & PAGE_MASK);
+	old_prefix = get_prefix();
+	memcpy(lowcore_tmp, 0, sizeof(lowcore_tmp));
+	assert(((uint64_t)&lowcore_tmp >> 31) == 0);
+	*prefix_ptr = (uint32_t)(uint64_t)&lowcore_tmp;
+
+	report_prefix_push("zero key");
+	set_prefix(old_prefix);
+	set_storage_key(prefix_ptr, 0x20, 0);
+	set_prefix(*prefix_ptr);
+	report(get_prefix() == *prefix_ptr, "set prefix");
+	report_prefix_pop();
+
+	report_prefix_push("matching key");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x10, 0);
+	set_prefix_key_1(prefix_ptr);
+	report(get_prefix() == *prefix_ptr, "set prefix");
+	report_prefix_pop();
+
+	report_prefix_push("mismatching key");
+
+	report_prefix_push("no fetch protection");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x20, 0);
+	set_prefix_key_1(prefix_ptr);
+	report(get_prefix() == *prefix_ptr, "set prefix");
+	report_prefix_pop();
+
+	report_prefix_push("fetch protection");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x28, 0);
+	expect_pgm_int();
+	set_prefix_key_1(prefix_ptr);
+	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	report(get_prefix() == old_prefix, "did not set prefix");
+	report_prefix_pop();
+
+	register_pgm_cleanup_func(dat_fixup_pgm_int);
+
+	report_prefix_push("remapped page, fetch protection");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x28, 0);
+	expect_pgm_int();
+	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+	set_prefix_key_1((uint32_t *)0);
+	install_page(root, 0, 0);
+	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	report(get_prefix() == old_prefix, "did not set prefix");
+	report_prefix_pop();
+
+	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+
+	report_prefix_push("fetch protection override applies");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x28, 0);
+	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+	set_prefix_key_1((uint32_t *)0);
+	install_page(root, 0, 0);
+	report(get_prefix() == *prefix_ptr, "set prefix");
+	report_prefix_pop();
+
+	no_override_prefix_ptr = (uint32_t *)(pagebuf + 2048);
+	WRITE_ONCE(*no_override_prefix_ptr, (uint32_t)(uint64_t)&lowcore_tmp);
+	report_prefix_push("fetch protection override does not apply");
+	set_prefix(old_prefix);
+	set_storage_key(pagebuf, 0x28, 0);
+	expect_pgm_int();
+	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
+	set_prefix_key_1((uint32_t *)2048);
+	install_page(root, 0, 0);
+	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
+	report(get_prefix() == old_prefix, "did not set prefix");
+	report_prefix_pop();
+
+	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
+	register_pgm_cleanup_func(NULL);
+	report_prefix_pop();
+	set_storage_key(pagebuf, 0x00, 0);
+	report_prefix_pop();
+}
+
 int main(void)
 {
 	report_prefix_push("skey");
@@ -130,6 +361,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();
-- 
2.33.1

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

[Claudio Imbrenda]

On Wed, 27 Apr 2022 12:06:10 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  lib/s390x/asm/arch_def.h |  20 ++--
>  s390x/skey.c             | 236 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 247 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 46c370e6..72553819 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 edad53e9..46efe3f5 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>
> @@ -118,6 +119,236 @@ 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) == TPROT_READ_WRITE, "access key 0 -> no protection");
> +	report(tprot(addr, 1) == TPROT_READ_WRITE, "access key matches -> no protection");
> +	report(tprot(addr, 2) == TPROT_READ,
> +	       "access key mismatches, no fetch protection -> store protection");
> +
> +	set_storage_key(pagebuf, 0x18, 0);
> +	report(tprot(addr, 2) == TPROT_RW_PROTECTED,
> +	       "access key mismatches, fetch protection -> fetch & store protection");
> +
> +	ctl_set_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +	set_storage_key(pagebuf, 0x90, 0);
> +	report(tprot(addr, 2) == TPROT_READ_WRITE,
> +	       "access key mismatches, storage protection override -> no protection");
> +	ctl_clear_bit(0, CTL0_STORAGE_PROTECTION_OVERRIDE);
> +
> +	set_storage_key(pagebuf, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
> +/*
> + * Perform STORE CPU ADDRESS (STAP) instruction while temporarily executing
> + * with access key 1.
> + */
> +static void store_cpu_address_key_1(uint16_t *out)
> +{
> +	asm volatile (
> +		"spka	0x10\n\t"
> +		"stap	%0\n\t"
> +		"spka	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;
> +
> +	report_prefix_push("STORE CPU ADDRESS");
> +	asm ("stap %0" : "=Q" (cpu_addr));
> +
> +	report_prefix_push("zero key");
> +	set_storage_key(pagebuf, 0x20, 0);
> +	WRITE_ONCE(*out, 0xbeef);
> +	asm ("stap %0" : "=Q" (*out));
> +	report(*out == cpu_addr, "store occurred");
> +	report_prefix_pop();
> +
> +	report_prefix_push("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("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("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("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);
> +
> +	report_prefix_push("storage-protection override disabled, override key");
> +	set_storage_key(pagebuf, 0x90, 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();
> +
> +	set_storage_key(pagebuf, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
> +/*
> + * Perform SET PREFIX (SPX) instruction while temporarily executing
> + * with access key 1.
> + */
> +static void set_prefix_key_1(uint32_t *prefix_ptr)
> +{
> +	asm volatile (
> +		"spka	0x10\n\t"
> +		"spx	%0\n\t"
> +		"spka	0\n"
> +	     :: "Q" (*prefix_ptr)
> +	);
> +}
> +
> +/*
> + * We remapped page 0, making the lowcore inaccessible, which breaks the normal
> + * handler 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);
> +}
> +
> +#define PREFIX_AREA_SIZE (PAGE_SIZE * 2)
> +static char lowcore_tmp[PREFIX_AREA_SIZE] __attribute__((aligned(PREFIX_AREA_SIZE)));
> +
> +/*
> + * Test accessibility of the operand to SET PREFIX given different configurations
> + * with regards to storage keys. That is, check the accessibility of the location
> + * holding the new prefix, not that of the new prefix area. The new prefix area
> + * is a valid lowcore, so that the test does not crash on failure.
> + */
> +static void test_set_prefix(void)
> +{
> +	uint32_t *prefix_ptr = (uint32_t *)pagebuf;
> +	uint32_t *no_override_prefix_ptr;
> +	uint32_t old_prefix;
> +	pgd_t *root;
> +
> +	report_prefix_push("SET PREFIX");
> +	root = (pgd_t *)(stctg(1) & PAGE_MASK);
> +	old_prefix = get_prefix();
> +	memcpy(lowcore_tmp, 0, sizeof(lowcore_tmp));
> +	assert(((uint64_t)&lowcore_tmp >> 31) == 0);
> +	*prefix_ptr = (uint32_t)(uint64_t)&lowcore_tmp;
> +
> +	report_prefix_push("zero key");
> +	set_prefix(old_prefix);
> +	set_storage_key(prefix_ptr, 0x20, 0);
> +	set_prefix(*prefix_ptr);
> +	report(get_prefix() == *prefix_ptr, "set prefix");
> +	report_prefix_pop();
> +
> +	report_prefix_push("matching key");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x10, 0);
> +	set_prefix_key_1(prefix_ptr);
> +	report(get_prefix() == *prefix_ptr, "set prefix");
> +	report_prefix_pop();
> +
> +	report_prefix_push("mismatching key");
> +
> +	report_prefix_push("no fetch protection");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x20, 0);
> +	set_prefix_key_1(prefix_ptr);
> +	report(get_prefix() == *prefix_ptr, "set prefix");
> +	report_prefix_pop();
> +
> +	report_prefix_push("fetch protection");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x28, 0);
> +	expect_pgm_int();
> +	set_prefix_key_1(prefix_ptr);
> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	report(get_prefix() == old_prefix, "did not set prefix");
> +	report_prefix_pop();
> +
> +	register_pgm_cleanup_func(dat_fixup_pgm_int);
> +
> +	report_prefix_push("remapped page, fetch protection");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x28, 0);
> +	expect_pgm_int();
> +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +	set_prefix_key_1((uint32_t *)0);
> +	install_page(root, 0, 0);
> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	report(get_prefix() == old_prefix, "did not set prefix");
> +	report_prefix_pop();
> +
> +	ctl_set_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +
> +	report_prefix_push("fetch protection override applies");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x28, 0);
> +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +	set_prefix_key_1((uint32_t *)0);
> +	install_page(root, 0, 0);
> +	report(get_prefix() == *prefix_ptr, "set prefix");
> +	report_prefix_pop();
> +
> +	no_override_prefix_ptr = (uint32_t *)(pagebuf + 2048);
> +	WRITE_ONCE(*no_override_prefix_ptr, (uint32_t)(uint64_t)&lowcore_tmp);
> +	report_prefix_push("fetch protection override does not apply");
> +	set_prefix(old_prefix);
> +	set_storage_key(pagebuf, 0x28, 0);
> +	expect_pgm_int();
> +	install_page(root, virt_to_pte_phys(root, pagebuf), 0);
> +	set_prefix_key_1((uint32_t *)2048);
> +	install_page(root, 0, 0);
> +	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
> +	report(get_prefix() == old_prefix, "did not set prefix");
> +	report_prefix_pop();
> +
> +	ctl_clear_bit(0, CTL0_FETCH_PROTECTION_OVERRIDE);
> +	register_pgm_cleanup_func(NULL);
> +	report_prefix_pop();
> +	set_storage_key(pagebuf, 0x00, 0);
> +	report_prefix_pop();
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("skey");
> @@ -130,6 +361,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();

[kvm-unit-tests PATCH v6 3/3] Disable s390x skey test in GitLab CI

[Janis Schoetterl-Glausch]

The test cases newly added to skey.c require kernel 5.18.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
 .gitlab-ci.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4f3049d8..e5768f1d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -166,7 +166,7 @@ s390x-kvm:
   - ./configure --arch=s390x
   - make -j$(nproc)
   - ACCEL=kvm ./run_tests.sh
-      selftest-setup intercept emulator sieve sthyi skey diag10 diag308 pfmf
+      selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf
       cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie
       | tee results.txt
   - grep -q PASS results.txt && ! grep -q FAIL results.txt
-- 
2.33.1

Re: [kvm-unit-tests PATCH v6 3/3] Disable s390x skey test in GitLab CI

[Claudio Imbrenda]

On Wed, 27 Apr 2022 12:06:11 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> The test cases newly added to skey.c require kernel 5.18.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  .gitlab-ci.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 4f3049d8..e5768f1d 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -166,7 +166,7 @@ s390x-kvm:
>    - ./configure --arch=s390x
>    - make -j$(nproc)
>    - ACCEL=kvm ./run_tests.sh
> -      selftest-setup intercept emulator sieve sthyi skey diag10 diag308 pfmf
> +      selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf
>        cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie
>        | tee results.txt
>    - grep -q PASS results.txt && ! grep -q FAIL results.txt

Re: [kvm-unit-tests PATCH v6 3/3] Disable s390x skey test in GitLab CI

[Thomas Huth]

On 27/04/2022 12.06, Janis Schoetterl-Glausch wrote:
> The test cases newly added to skey.c require kernel 5.18.
> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
>   .gitlab-ci.yml | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 4f3049d8..e5768f1d 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -166,7 +166,7 @@ s390x-kvm:
>     - ./configure --arch=s390x
>     - make -j$(nproc)
>     - ACCEL=kvm ./run_tests.sh
> -      selftest-setup intercept emulator sieve sthyi skey diag10 diag308 pfmf
> +      selftest-setup intercept emulator sieve sthyi diag10 diag308 pfmf
>         cmm vector gs iep cpumodel diag288 stsi sclp-1g sclp-3g css skrf sie
>         | tee results.txt
>     - grep -q PASS results.txt && ! grep -q FAIL results.txt

Reviewed-by: Thomas Huth <thuth@redhat.com>