[kvm-unit-tests PATCH 0/6] s390x: uv-host: Access check extensions and improvements

[kvm-unit-tests PATCH 0/6] s390x: uv-host: Access check extensions and improvements

[Janosch Frank]

Over the last few weeks I had a few ideas on how to extend the uv-host
test to get more coverage. Most checks are access checks for secure
pages or the UVCB and it's satellites.

I've had limited time to cleanup this series, so consider having a
closer look.

Janosch Frank (6):
  s390x: uv-host: Add access checks for donated memory
  s390x: uv-host: Add uninitialized UV tests
  s390x: uv-host: Test uv immediate parameter
  s390x: uv-host: Add access exception test
  s390x: uv-host: Add a set secure config parameters test function
  s390x: uv-host: Remove duplicated +

 s390x/uv-host.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 239 insertions(+), 5 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH 1/6] s390x: uv-host: Add access checks for donated memory

[Janosch Frank]

Let's check if the UV really protected all the memory we donated.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index a1a6d120..0f0b18a1 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -142,7 +142,8 @@ static void test_cpu_destroy(void)
 static void test_cpu_create(void)
 {
 	int rc;
-	unsigned long tmp;
+	unsigned long tmp, i;
+	uint8_t *access_ptr;
 
 	report_prefix_push("csc");
 	uvcb_csc.header.len = sizeof(uvcb_csc);
@@ -194,6 +195,18 @@ static void test_cpu_create(void)
 	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
 	       uvcb_csc.cpu_handle, "success");
 
+	rc = 1;
+	for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) {
+		expect_pgm_int();
+		access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE * i;
+		*access_ptr = 42;
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) {
+			rc = 0;
+			break;
+		}
+	}
+	report(rc, "Storage protection");
+
 	tmp = uvcb_csc.stor_origin;
 	uvcb_csc.stor_origin = (unsigned long)memalign(PAGE_SIZE, uvcb_qui.cpu_stor_len);
 	rc = uv_call(0, (uint64_t)&uvcb_csc);
@@ -205,8 +218,9 @@ static void test_cpu_create(void)
 static void test_config_create(void)
 {
 	int rc;
-	unsigned long vsize, tmp;
+	unsigned long vsize, tmp, i;
 	static struct uv_cb_cgc uvcb;
+	uint8_t *access_ptr;
 
 	uvcb_cgc.header.cmd = UVC_CMD_CREATE_SEC_CONF;
 	uvcb_cgc.header.len = sizeof(uvcb_cgc);
@@ -292,6 +306,30 @@ static void test_config_create(void)
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
 	report(rc == 0 && uvcb_cgc.header.rc == UVC_RC_EXECUTED, "successful");
 
+	rc = 1;
+	for (i = 0; i < vsize / PAGE_SIZE; i++) {
+		expect_pgm_int();
+		access_ptr = (void *)uvcb_cgc.conf_var_stor_origin + PAGE_SIZE * i;
+		*access_ptr = 42;
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) {
+			rc = 0;
+			break;
+		}
+	}
+	report(rc, "Base storage protection");
+
+	rc = 1;
+	for (i = 0; i < uvcb_qui.conf_base_phys_stor_len / PAGE_SIZE; i++) {
+		expect_pgm_int();
+		access_ptr = (void *)uvcb_cgc.conf_base_stor_origin + PAGE_SIZE * i;
+		*access_ptr = 42;
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS) {
+			rc = 0;
+			break;
+		}
+	}
+	report(rc, "Variable storage protection");
+
 	uvcb_cgc.header.rc = 0;
 	uvcb_cgc.header.rrc = 0;
 	tmp = uvcb_cgc.guest_handle;
-- 
2.34.1

Re: [kvm-unit-tests PATCH 1/6] s390x: uv-host: Add access checks for donated memory

[Nico Boehr]

On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote:
> Let's check if the UV really protected all the memory we donated.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

One suggestion below for you to consider.

> ---
>  s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index a1a6d120..0f0b18a1 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -142,7 +142,8 @@ static void test_cpu_destroy(void)
>  static void test_cpu_create(void)
>  {
>         int rc;
> -       unsigned long tmp;
> +       unsigned long tmp, i;
> +       uint8_t *access_ptr;
>  
>         report_prefix_push("csc");
>         uvcb_csc.header.len = sizeof(uvcb_csc);
> @@ -194,6 +195,18 @@ static void test_cpu_create(void)
>         report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>                uvcb_csc.cpu_handle, "success");
>  
> +       rc = 1;
> +       for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) {
> +               expect_pgm_int();
> +               access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE
> * i;
> +               *access_ptr = 42;
> +               if (clear_pgm_int() !=
> PGM_INT_CODE_SECURE_STOR_ACCESS) {
> +                       rc = 0;
> +                       break;
> +               }
> +       }
> +       report(rc, "Storage protection");

All of these for loops look pretty similar, would it make sense to move
them to their own function like:

assert_range_write_protected(void *start, size_t len, int
pgm_int_code)?

Re: [kvm-unit-tests PATCH 1/6] s390x: uv-host: Add access checks for donated memory

[Janosch Frank]

On 5/16/22 10:21, Nico Boehr wrote:
> On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote:
>> Let's check if the UV really protected all the memory we donated.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Thanks

> 
> One suggestion below for you to consider.

Sure, I'll rework the loops

> 
>> ---
>>   s390x/uv-host.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index a1a6d120..0f0b18a1 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -142,7 +142,8 @@ static void test_cpu_destroy(void)
>>   static void test_cpu_create(void)
>>   {
>>          int rc;
>> -       unsigned long tmp;
>> +       unsigned long tmp, i;
>> +       uint8_t *access_ptr;
>>   
>>          report_prefix_push("csc");
>>          uvcb_csc.header.len = sizeof(uvcb_csc);
>> @@ -194,6 +195,18 @@ static void test_cpu_create(void)
>>          report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>>                 uvcb_csc.cpu_handle, "success");
>>   
>> +       rc = 1;
>> +       for (i = 0; i < uvcb_qui.cpu_stor_len / PAGE_SIZE; i++) {
>> +               expect_pgm_int();
>> +               access_ptr = (void *)uvcb_csc.stor_origin + PAGE_SIZE
>> * i;
>> +               *access_ptr = 42;
>> +               if (clear_pgm_int() !=
>> PGM_INT_CODE_SECURE_STOR_ACCESS) {
>> +                       rc = 0;
>> +                       break;
>> +               }
>> +       }
>> +       report(rc, "Storage protection");
> 
> All of these for loops look pretty similar, would it make sense to move
> them to their own function like:
> 
> assert_range_write_protected(void *start, size_t len, int
> pgm_int_code)?

[kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Janosch Frank]

Let's also test for rc 0x3

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 0f0b18a1..f846fc42 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -83,6 +83,24 @@ static void test_priv(void)
 	report_prefix_pop();
 }
 
+static void test_uv_uninitialized(void)
+{
+	struct uv_cb_header uvcb = {};
+	int i;
+
+	report_prefix_push("uninitialized");
+
+	/* i = 1 to skip over initialize */
+	for (i = 1; cmds[i].name; i++) {
+		expect_pgm_int();
+		uvcb.cmd = cmds[i].cmd;
+		uvcb.len = cmds[i].len;
+		uv_call_once(0, (uint64_t)&uvcb);
+		report(uvcb.rc == UVC_RC_INV_STATE, "%s", cmds[i].name);
+	}
+	report_prefix_pop();
+}
+
 static void test_config_destroy(void)
 {
 	int rc;
@@ -477,13 +495,68 @@ static void test_invalid(void)
 	report_prefix_pop();
 }
 
+static void test_clear_setup(void)
+{
+	unsigned long vsize;
+	int rc;
+
+	uvcb_cgc.header.cmd = UVC_CMD_CREATE_SEC_CONF;
+	uvcb_cgc.header.len = sizeof(uvcb_cgc);
+
+	uvcb_cgc.guest_stor_origin = 0;
+	uvcb_cgc.guest_stor_len = 42 * (1UL << 20);
+	vsize = uvcb_qui.conf_base_virt_stor_len +
+		((uvcb_cgc.guest_stor_len / (1UL << 20)) * uvcb_qui.conf_virt_var_stor_len);
+
+	uvcb_cgc.conf_base_stor_origin = (uint64_t)memalign(PAGE_SIZE * 4, uvcb_qui.conf_base_phys_stor_len);
+	uvcb_cgc.conf_var_stor_origin = (uint64_t)memalign(PAGE_SIZE, vsize);
+	uvcb_cgc.guest_asce = (uint64_t)memalign(PAGE_SIZE, 4 * PAGE_SIZE) | ASCE_DT_SEGMENT | REGION_TABLE_LENGTH | ASCE_P;
+	uvcb_cgc.guest_sca = (uint64_t)memalign(PAGE_SIZE * 4, PAGE_SIZE * 4);
+
+	rc = uv_call(0, (uint64_t)&uvcb_cgc);
+	assert(rc == 0);
+
+	uvcb_csc.header.len = sizeof(uvcb_csc);
+	uvcb_csc.header.cmd = UVC_CMD_CREATE_SEC_CPU;
+	uvcb_csc.guest_handle = uvcb_cgc.guest_handle;
+	uvcb_csc.stor_origin = (unsigned long)memalign(PAGE_SIZE, uvcb_qui.cpu_stor_len);
+	uvcb_csc.state_origin = (unsigned long)memalign(PAGE_SIZE, PAGE_SIZE);
+
+	rc = uv_call(0, (uint64_t)&uvcb_csc);
+	assert(rc == 0);
+}
+
 static void test_clear(void)
 {
-	uint64_t *tmp = (void *)uvcb_init.stor_origin;
+	uint64_t *tmp;
+
+	report_prefix_push("load normal reset");
+
+	/*
+	 * Setup a config and a cpu so we can check if a diag308 reset
+	 * clears the donated memory and makes the pages unsecure.
+	 */
+	test_clear_setup();
 
 	diag308_load_reset(1);
 	sclp_console_setup();
-	report(!*tmp, "memory cleared after reset 1");
+
+	tmp = (void *)uvcb_init.stor_origin;
+	report(!*tmp, "uv init donated memory cleared");
+
+	tmp = (void *)uvcb_cgc.conf_base_stor_origin;
+	report(!*tmp, "config base donated memory cleared");
+
+	tmp = (void *)uvcb_cgc.conf_base_stor_origin;
+	report(!*tmp, "config variable donated memory cleared");
+
+	tmp = (void *)uvcb_csc.stor_origin;
+	report(!*tmp, "cpu donated memory cleared after reset 1");
+
+	/* Check if uninitialized after reset */
+	test_uv_uninitialized();
+
+	report_prefix_pop();
 }
 
 static void setup_vmem(void)
@@ -514,6 +587,7 @@ int main(void)
 
 	test_priv();
 	test_invalid();
+	test_uv_uninitialized();
 	test_query();
 	test_init();
 
-- 
2.34.1

Re: [kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Nico Boehr]

On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote:
> Let's also test for rc 0x3
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
if you fix the nit below.

> ---
>  s390x/uv-host.c | 78
> +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 0f0b18a1..f846fc42 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -83,6 +83,24 @@ static void test_priv(void)
>         report_prefix_pop();
>  }
>  
> +static void test_uv_uninitialized(void)
> +{
> +       struct uv_cb_header uvcb = {};
> +       int i;
> +
> +       report_prefix_push("uninitialized");
> +
> +       /* i = 1 to skip over initialize */

Just say

if (cmds[i].cmd == UVC_CMD_INIT_UV)
  continue;

inside the loop.

Re: [kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Steffen Eiden]

On 5/13/22 11:50, Janosch Frank wrote:
> Let's also test for rc 0x3
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
I, however, have some nits below.

> ---
>   s390x/uv-host.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 0f0b18a1..f846fc42 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -83,6 +83,24 @@ static void test_priv(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_uv_uninitialized(void)
> +{
> +	struct uv_cb_header uvcb = {};
> +	int i;
> +
> +	report_prefix_push("uninitialized");
> +
> +	/* i = 1 to skip over initialize */
> +	for (i = 1; cmds[i].name; i++) {
> +		expect_pgm_int();
> +		uvcb.cmd = cmds[i].cmd;
> +		uvcb.len = cmds[i].len;
> +		uv_call_once(0, (uint64_t)&uvcb);
> +		report(uvcb.rc == UVC_RC_INV_STATE, "%s", cmds[i].name);
> +	}
> +	report_prefix_pop();
> +}
> +
>   static void test_config_destroy(void)
>   {
>   	int rc;
> @@ -477,13 +495,68 @@ static void test_invalid(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_clear_setup(void)
maybe rename this to setup_test_clear(void)
I initially mistook this function as a test and not a setup function for
a test

> +{
> +	unsigned long vsize;
> +	int rc;
> +
> +	uvcb_cgc.header.cmd = UVC_CMD_CREATE_SEC_CONF;
> +	uvcb_cgc.header.len = sizeof(uvcb_cgc);
> +
> +	uvcb_cgc.guest_stor_origin = 0;
> +	uvcb_cgc.guest_stor_len = 42 * (1UL << 20);
> +	vsize = uvcb_qui.conf_base_virt_stor_len +
> +		((uvcb_cgc.guest_stor_len / (1UL << 20)) * uvcb_qui.conf_virt_var_stor_len);
> +
> +	uvcb_cgc.conf_base_stor_origin = (uint64_t)memalign(PAGE_SIZE * 4, uvcb_qui.conf_base_phys_stor_len);
> +	uvcb_cgc.conf_var_stor_origin = (uint64_t)memalign(PAGE_SIZE, vsize);
> +	uvcb_cgc.guest_asce = (uint64_t)memalign(PAGE_SIZE, 4 * PAGE_SIZE) | ASCE_DT_SEGMENT | REGION_TABLE_LENGTH | ASCE_P;
> +	uvcb_cgc.guest_sca = (uint64_t)memalign(PAGE_SIZE * 4, PAGE_SIZE * 4);
> +
> +	rc = uv_call(0, (uint64_t)&uvcb_cgc);
> +	assert(rc == 0);
> +
> +	uvcb_csc.header.len = sizeof(uvcb_csc);
> +	uvcb_csc.header.cmd = UVC_CMD_CREATE_SEC_CPU;
> +	uvcb_csc.guest_handle = uvcb_cgc.guest_handle;
> +	uvcb_csc.stor_origin = (unsigned long)memalign(PAGE_SIZE, uvcb_qui.cpu_stor_len);
> +	uvcb_csc.state_origin = (unsigned long)memalign(PAGE_SIZE, PAGE_SIZE);
> +
> +	rc = uv_call(0, (uint64_t)&uvcb_csc);
> +	assert(rc == 0);
> +}
> +
>   static void test_clear(void)
>   {
> -	uint64_t *tmp = (void *)uvcb_init.stor_origin;
> +	uint64_t *tmp;
> +
> +	report_prefix_push("load normal reset");
> +
> +	/*
> +	 * Setup a config and a cpu so we can check if a diag308 reset
> +	 * clears the donated memory and makes the pages unsecure.
> +	 */
> +	test_clear_setup();
>   
>   	diag308_load_reset(1);
>   	sclp_console_setup();
> -	report(!*tmp, "memory cleared after reset 1");
> +
> +	tmp = (void *)uvcb_init.stor_origin;
> +	report(!*tmp, "uv init donated memory cleared");
> +
> +	tmp = (void *)uvcb_cgc.conf_base_stor_origin;
> +	report(!*tmp, "config base donated memory cleared");
> +
> +	tmp = (void *)uvcb_cgc.conf_base_stor_origin;
> +	report(!*tmp, "config variable donated memory cleared");
> +
> +	tmp = (void *)uvcb_csc.stor_origin;
> +	report(!*tmp, "cpu donated memory cleared after reset 1");
> +
> +	/* Check if uninitialized after reset */
> +	test_uv_uninitialized();
> +
> +	report_prefix_pop();
>   }
>   
>   static void setup_vmem(void)
> @@ -514,6 +587,7 @@ int main(void)
>   
>   	test_priv();
>   	test_invalid();
> +	test_uv_uninitialized();
>   	test_query();
>   	test_init();
IIRC this test must be done last, as a following test has an 
uninitialized UV. Maybe add a short comment for that here.
>   

Re: [kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Janosch Frank]

On 5/16/22 17:02, Steffen Eiden wrote:
> 
> 
> On 5/13/22 11:50, Janosch Frank wrote:
>> Let's also test for rc 0x3
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> I, however, have some nits below.
> 
>> ---
>>    s390x/uv-host.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++--
>>    1 file changed, 76 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index 0f0b18a1..f846fc42 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -83,6 +83,24 @@ static void test_priv(void)
>>    	report_prefix_pop();
>>    }
>>    
>> +static void test_uv_uninitialized(void)
>> +{
>> +	struct uv_cb_header uvcb = {};
>> +	int i;
>> +
>> +	report_prefix_push("uninitialized");
>> +
>> +	/* i = 1 to skip over initialize */
>> +	for (i = 1; cmds[i].name; i++) {
>> +		expect_pgm_int();
>> +		uvcb.cmd = cmds[i].cmd;
>> +		uvcb.len = cmds[i].len;
>> +		uv_call_once(0, (uint64_t)&uvcb);
>> +		report(uvcb.rc == UVC_RC_INV_STATE, "%s", cmds[i].name);
>> +	}
>> +	report_prefix_pop();
>> +}
>> +
>>    static void test_config_destroy(void)
>>    {
>>    	int rc;
>> @@ -477,13 +495,68 @@ static void test_invalid(void)
>>    	report_prefix_pop();
>>    }
>>    
>> +static void test_clear_setup(void)
> maybe rename this to setup_test_clear(void)
> I initially mistook this function as a test and not a setup function for
> a test

Sure

> 
>> +{
>> +	unsigned long vsize;
>> +	int rc;
>> +
[...]
>>    static void setup_vmem(void)
>> @@ -514,6 +587,7 @@ int main(void)
>>    
>>    	test_priv();
>>    	test_invalid();
>> +	test_uv_uninitialized();
>>    	test_query();
>>    	test_init();
> IIRC this test must be done last, as a following test has an
> uninitialized UV. Maybe add a short comment for that here.

You're referring to the test_init()?

The test_clear() function must be done last but you're commenting under 
the test_init() call. So I'm a bit confused about what you want me to do 
here.

>>    
> 

Re: [kvm-unit-tests PATCH 2/6] s390x: uv-host: Add uninitialized UV tests

[Steffen Eiden]

Hi Janosch,

On 5/16/22 17:15, Janosch Frank wrote:
> On 5/16/22 17:02, Steffen Eiden wrote:
>>
[snip]
> [...]
>>>    static void setup_vmem(void)
>>> @@ -514,6 +587,7 @@ int main(void)
>>>        test_priv();
>>>        test_invalid();
>>> +    test_uv_uninitialized();
>>>        test_query();
>>>        test_init();
>> IIRC this test must be done last, as a following test has an
>> uninitialized UV. Maybe add a short comment for that here.
> 
> You're referring to the test_init()?
> 
> The test_clear() function must be done last but you're commenting under 
> the test_init() call. So I'm a bit confused about what you want me to do 
> here.

Sorry, my fault. Yes I wanted to suggest a comment for the 'test_clear' 
function.

[kvm-unit-tests PATCH 3/6] s390x: uv-host: Test uv immediate parameter

[Janosch Frank]

Let's check if we get a specification PGM exception if we set a
non-zero i3 when doing a UV call.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index f846fc42..fcb82d24 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -64,6 +64,28 @@ static struct cmd_list cmds[] = {
 	{ NULL, 0, 0 },
 };
 
+static void test_i3(void)
+{
+	struct uv_cb_header uvcb = {
+		.cmd = UVC_CMD_INIT_UV,
+		.len = sizeof(struct uv_cb_init),
+	};
+	unsigned long r1 = 0;
+	int cc;
+
+	report_prefix_push("i3");
+	expect_pgm_int();
+	asm volatile(
+		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],4,2\n"
+		"		ipm	%[cc]\n"
+		"		srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: [r1] "a" (r1), [r2] "a" (&uvcb)
+		: "memory", "cc");
+	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+	report_prefix_pop();
+}
+
 static void test_priv(void)
 {
 	struct uv_cb_header uvcb = {};
@@ -585,6 +607,7 @@ int main(void)
 		goto done;
 	}
 
+	test_i3();
 	test_priv();
 	test_invalid();
 	test_uv_uninitialized();
-- 
2.34.1

Re: [kvm-unit-tests PATCH 3/6] s390x: uv-host: Test uv immediate parameter

[Steffen Eiden]

Hey Janosch,

On 5/13/22 11:50, Janosch Frank wrote:
> Let's check if we get a specification PGM exception if we set a
> non-zero i3 when doing a UV call.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/uv-host.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index f846fc42..fcb82d24 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -64,6 +64,28 @@ static struct cmd_list cmds[] = {
>   	{ NULL, 0, 0 },
>   };
>   
> +static void test_i3(void)
> +{
> +	struct uv_cb_header uvcb = {
> +		.cmd = UVC_CMD_INIT_UV,
> +		.len = sizeof(struct uv_cb_init),
> +	};
> +	unsigned long r1 = 0;
Did you forgot 'r2' or is it missing for a reason?

> +	int cc;
> +
> +	report_prefix_push("i3");
> +	expect_pgm_int();
> +	asm volatile(
> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],4,2\n"
> +		"		ipm	%[cc]\n"
> +		"		srl	%[cc],28\n"
> +		: [cc] "=d" (cc)
> +		: [r1] "a" (r1), [r2] "a" (&uvcb)
> +		: "memory", "cc");
> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +	report_prefix_pop();
> +}
> +
>   static void test_priv(void)
>   {
>   	struct uv_cb_header uvcb = {};
> @@ -585,6 +607,7 @@ int main(void)
>   		goto done;
>   	}
>   
> +	test_i3();
>   	test_priv();
>   	test_invalid();
>   	test_uv_uninitialized();

Re: [kvm-unit-tests PATCH 3/6] s390x: uv-host: Test uv immediate parameter

[Janosch Frank]

On 5/17/22 10:29, Steffen Eiden wrote:
> Hey Janosch,
> 
> On 5/13/22 11:50, Janosch Frank wrote:
>> Let's check if we get a specification PGM exception if we set a
>> non-zero i3 when doing a UV call.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>    s390x/uv-host.c | 23 +++++++++++++++++++++++
>>    1 file changed, 23 insertions(+)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index f846fc42..fcb82d24 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -64,6 +64,28 @@ static struct cmd_list cmds[] = {
>>    	{ NULL, 0, 0 },
>>    };
>>    
>> +static void test_i3(void)
>> +{
>> +	struct uv_cb_header uvcb = {
>> +		.cmd = UVC_CMD_INIT_UV,
>> +		.len = sizeof(struct uv_cb_init),
>> +	};
>> +	unsigned long r1 = 0;
> Did you forgot 'r2' or is it missing for a reason?

The uvcb is the r2, have a look at the clobbers below

> 
>> +	int cc;
>> +
>> +	report_prefix_push("i3");
>> +	expect_pgm_int();
>> +	asm volatile(
>> +		"0:	.insn rrf,0xB9A40000,%[r1],%[r2],4,2\n"
>> +		"		ipm	%[cc]\n"
>> +		"		srl	%[cc],28\n"
>> +		: [cc] "=d" (cc)
>> +		: [r1] "a" (r1), [r2] "a" (&uvcb)
>> +		: "memory", "cc");
>> +	check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>> +	report_prefix_pop();
>> +}
>> +
>>    static void test_priv(void)
>>    {
>>    	struct uv_cb_header uvcb = {};
>> @@ -585,6 +607,7 @@ int main(void)
>>    		goto done;
>>    	}
>>    
>> +	test_i3();
>>    	test_priv();
>>    	test_invalid();
>>    	test_uv_uninitialized();

Re: [kvm-unit-tests PATCH 3/6] s390x: uv-host: Test uv immediate parameter

[Steffen Eiden]

On 5/17/22 11:03, Janosch Frank wrote:
> On 5/17/22 10:29, Steffen Eiden wrote:
>> Hey Janosch,
>>
>> On 5/13/22 11:50, Janosch Frank wrote:
>>> Let's check if we get a specification PGM exception if we set a
>>> non-zero i3 when doing a UV call.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    s390x/uv-host.c | 23 +++++++++++++++++++++++
>>>    1 file changed, 23 insertions(+)
>>>
>>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>>> index f846fc42..fcb82d24 100644
>>> --- a/s390x/uv-host.c
>>> +++ b/s390x/uv-host.c
>>> @@ -64,6 +64,28 @@ static struct cmd_list cmds[] = {
>>>        { NULL, 0, 0 },
>>>    };
>>> +static void test_i3(void)
>>> +{
>>> +    struct uv_cb_header uvcb = {
>>> +        .cmd = UVC_CMD_INIT_UV,
>>> +        .len = sizeof(struct uv_cb_init),
>>> +    };
>>> +    unsigned long r1 = 0;
>> Did you forgot 'r2' or is it missing for a reason?
> 
> The uvcb is the r2, have a look at the clobbers below

Oh right, my bad; Sorry.
> 
>>
>>> +    int cc;
>>> +
>>> +    report_prefix_push("i3");
>>> +    expect_pgm_int();
>>> +    asm volatile(
>>> +        "0:    .insn rrf,0xB9A40000,%[r1],%[r2],4,2\n"
>>> +        "        ipm    %[cc]\n"
>>> +        "        srl    %[cc],28\n"
>>> +        : [cc] "=d" (cc)
>>> +        : [r1] "a" (r1), [r2] "a" (&uvcb)
>>> +        : "memory", "cc");
>>> +    check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
>>> +    report_prefix_pop();
>>> +}
>>> +
>>>    static void test_priv(void)
>>>    {
>>>        struct uv_cb_header uvcb = {};
>>> @@ -585,6 +607,7 @@ int main(void)
>>>            goto done;
>>>        }
>>> +    test_i3();
>>>        test_priv();
>>>        test_invalid();
>>>        test_uv_uninitialized();
> 

-- 
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

[kvm-unit-tests PATCH 4/6] s390x: uv-host: Add access exception test

[Janosch Frank]

Let's check that we get access exceptions if the UVCB is on an invalid
page or starts at a valid page and crosses into an invalid one.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index fcb82d24..153a94e1 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -15,12 +15,14 @@
 #include <sclp.h>
 #include <smp.h>
 #include <uv.h>
+#include <mmu.h>
 #include <asm/page.h>
 #include <asm/sigp.h>
 #include <asm/pgtable.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/facility.h>
+#include <asm/pgtable.h>
 #include <asm/uv.h>
 #include <asm-generic/barrier.h>
 
@@ -123,6 +125,54 @@ static void test_uv_uninitialized(void)
 	report_prefix_pop();
 }
 
+static void test_access(void)
+{
+	struct uv_cb_header *uvcb;
+	void *pages =  alloc_pages(1);
+	uint16_t pgm;
+	int i;
+
+	/* Put UVCB on second page which we will protect later */
+	uvcb = pages + PAGE_SIZE;
+
+	report_prefix_push("access");
+
+	report_prefix_push("non-crossing");
+	protect_page(uvcb, PAGE_ENTRY_I);
+	for (i = 0; cmds[i].name; i++) {
+		expect_pgm_int();
+		mb();
+		uv_call_once(0, (uint64_t)uvcb);
+		pgm = clear_pgm_int();
+		report(pgm == PGM_INT_CODE_PAGE_TRANSLATION, "%s", cmds[i].name);
+	}
+	report_prefix_pop();
+
+	report_prefix_push("crossing");
+	/*
+	 * Put the header into the readable page 1, everything after
+	 * the header will be on the second, invalid page.
+	 */
+	uvcb -= 1;
+	for (i = 0; cmds[i].name; i++) {
+		uvcb->cmd = cmds[i].cmd;
+		uvcb->len = cmds[i].len;
+
+		expect_pgm_int();
+		mb();
+		uv_call_once(0, (uint64_t)uvcb);
+		pgm = clear_pgm_int();
+		report(pgm == PGM_INT_CODE_PAGE_TRANSLATION, "%s", cmds[i].name);
+	}
+	report_prefix_pop();
+
+	uvcb += 1;
+	unprotect_page(uvcb, PAGE_ENTRY_I);
+
+	free_pages(pages);
+	report_prefix_pop();
+}
+
 static void test_config_destroy(void)
 {
 	int rc;
@@ -615,6 +665,8 @@ int main(void)
 	test_init();
 
 	setup_vmem();
+	test_access();
+
 	test_config_create();
 	test_cpu_create();
 	test_cpu_destroy();
-- 
2.34.1

Re: [kvm-unit-tests PATCH 4/6] s390x: uv-host: Add access exception test

[Nico Boehr]

On Fri, 2022-05-13 at 09:50 +0000, Janosch Frank wrote:
> Let's check that we get access exceptions if the UVCB is on an
> invalid
> page or starts at a valid page and crosses into an invalid one.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH 4/6] s390x: uv-host: Add access exception test

[Steffen Eiden]

On 5/13/22 11:50, Janosch Frank wrote:
> Let's check that we get access exceptions if the UVCB is on an invalid
> page or starts at a valid page and crosses into an invalid one.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>

[kvm-unit-tests PATCH 5/6] s390x: uv-host: Add a set secure config parameters test function

[Janosch Frank]

Time for more tests.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 153a94e1..20d805b8 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -229,6 +229,52 @@ static void test_cpu_destroy(void)
 	report_prefix_pop();
 }
 
+static void test_set_se_header(void)
+{
+	struct uv_cb_ssc uvcb = {
+		.header.cmd = UVC_CMD_SET_SEC_CONF_PARAMS,
+		.header.len = sizeof(uvcb),
+		.guest_handle = uvcb_cgc.guest_handle,
+		.sec_header_origin = 0,
+		.sec_header_len = 0x1000,
+	};
+	void *pages =  alloc_pages(1);
+	void *inv;
+	int rc;
+
+	report_prefix_push("sscp");
+
+	uvcb.header.len -= 8;
+	rc = uv_call(0, (uint64_t)&uvcb);
+	report(rc == 1 && uvcb.header.rc == UVC_RC_INV_LEN,
+	       "hdr invalid length");
+	uvcb.header.len += 8;
+
+	uvcb.guest_handle += 1;
+	rc = uv_call(0, (uint64_t)&uvcb);
+	report(rc == 1 && uvcb.header.rc == UVC_RC_INV_GHANDLE, "invalid handle");
+	uvcb.guest_handle -= 1;
+
+	inv = pages + PAGE_SIZE;
+	uvcb.sec_header_origin = (uint64_t)inv;
+	protect_page(inv, PAGE_ENTRY_I);
+	rc = uv_call(0, (uint64_t)&uvcb);
+	report(rc == 1 && uvcb.header.rc == 0x103,
+	       "se hdr access exception");
+
+	/*
+	 * Shift the ptr so the first few DWORDs are accessible but
+	 * the following are on an invalid page.
+	 */
+	uvcb.sec_header_origin -= 0x20;
+	rc = uv_call(0, (uint64_t)&uvcb);
+	report(rc == 1 && uvcb.header.rc == 0x103,
+	       "se hdr access exception crossing");
+	unprotect_page(inv, PAGE_ENTRY_I);
+
+	report_prefix_pop();
+}
+
 static void test_cpu_create(void)
 {
 	int rc;
@@ -669,6 +715,7 @@ int main(void)
 
 	test_config_create();
 	test_cpu_create();
+	test_set_se_header();
 	test_cpu_destroy();
 	test_config_destroy();
 	test_clear();
-- 
2.34.1

Re: [kvm-unit-tests PATCH 5/6] s390x: uv-host: Add a set secure config parameters test function

[Steffen Eiden]

Hey Janosch,

On 5/13/22 11:50, Janosch Frank wrote:
> Time for more tests.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
If you free 'pages'

> ---
>   s390x/uv-host.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 153a94e1..20d805b8 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -229,6 +229,52 @@ static void test_cpu_destroy(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_set_se_header(void)
> +{
> +	struct uv_cb_ssc uvcb = {
> +		.header.cmd = UVC_CMD_SET_SEC_CONF_PARAMS,
> +		.header.len = sizeof(uvcb),
> +		.guest_handle = uvcb_cgc.guest_handle,
> +		.sec_header_origin = 0,
> +		.sec_header_len = 0x1000,
> +	};
> +	void *pages =  alloc_pages(1);
> +	void *inv;
> +	int rc;
> +
> +	report_prefix_push("sscp");
> +
> +	uvcb.header.len -= 8;
> +	rc = uv_call(0, (uint64_t)&uvcb);
> +	report(rc == 1 && uvcb.header.rc == UVC_RC_INV_LEN,
> +	       "hdr invalid length");
> +	uvcb.header.len += 8;
> +
> +	uvcb.guest_handle += 1;
> +	rc = uv_call(0, (uint64_t)&uvcb);
> +	report(rc == 1 && uvcb.header.rc == UVC_RC_INV_GHANDLE, "invalid handle");
> +	uvcb.guest_handle -= 1;
> +
> +	inv = pages + PAGE_SIZE;
> +	uvcb.sec_header_origin = (uint64_t)inv;
> +	protect_page(inv, PAGE_ENTRY_I);
> +	rc = uv_call(0, (uint64_t)&uvcb);
> +	report(rc == 1 && uvcb.header.rc == 0x103,
> +	       "se hdr access exception");
> +
> +	/*
> +	 * Shift the ptr so the first few DWORDs are accessible but
> +	 * the following are on an invalid page.
> +	 */
> +	uvcb.sec_header_origin -= 0x20;
> +	rc = uv_call(0, (uint64_t)&uvcb);
> +	report(rc == 1 && uvcb.header.rc == 0x103,
> +	       "se hdr access exception crossing");
> +	unprotect_page(inv, PAGE_ENTRY_I);

please free 'pages'.

> +
> +	report_prefix_pop();
> +}
> +
>   static void test_cpu_create(void)
>   {
>   	int rc;
> @@ -669,6 +715,7 @@ int main(void)
>   
>   	test_config_create();
>   	test_cpu_create();
> +	test_set_se_header();
>   	test_cpu_destroy();
>   	test_config_destroy();
>   	test_clear();

Steffen

[kvm-unit-tests PATCH 6/6] s390x: uv-host: Remove duplicated +

[Janosch Frank]

One + is definitely enough here.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/uv-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 20d805b8..ed16f850 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -433,7 +433,7 @@ static void test_config_create(void)
 	uvcb_cgc.guest_sca = tmp;
 
 	tmp = uvcb_cgc.guest_sca;
-	uvcb_cgc.guest_sca = get_max_ram_size() + + PAGE_SIZE * 4;
+	uvcb_cgc.guest_sca = get_max_ram_size() + PAGE_SIZE * 4;
 	rc = uv_call(0, (uint64_t)&uvcb_cgc);
 	report(uvcb_cgc.header.rc == 0x10d && rc == 1,
 	       "sca inaccessible");
-- 
2.34.1

Re: [kvm-unit-tests PATCH 6/6] s390x: uv-host: Remove duplicated +

[Claudio Imbrenda]

On Fri, 13 May 2022 09:50:17 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> One + is definitely enough here.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

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

> ---
>  s390x/uv-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 20d805b8..ed16f850 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -433,7 +433,7 @@ static void test_config_create(void)
>  	uvcb_cgc.guest_sca = tmp;
>  
>  	tmp = uvcb_cgc.guest_sca;
> -	uvcb_cgc.guest_sca = get_max_ram_size() + + PAGE_SIZE * 4;
> +	uvcb_cgc.guest_sca = get_max_ram_size() + PAGE_SIZE * 4;
>  	rc = uv_call(0, (uint64_t)&uvcb_cgc);
>  	report(uvcb_cgc.header.rc == 0x10d && rc == 1,
>  	       "sca inaccessible");

Re: [kvm-unit-tests PATCH 6/6] s390x: uv-host: Remove duplicated +

[Steffen Eiden]

Hey Janosch,

On 5/13/22 11:50, Janosch Frank wrote:
> One + is definitely enough here.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   s390x/uv-host.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 20d805b8..ed16f850 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -433,7 +433,7 @@ static void test_config_create(void)
>   	uvcb_cgc.guest_sca = tmp;
>   
>   	tmp = uvcb_cgc.guest_sca;
> -	uvcb_cgc.guest_sca = get_max_ram_size() + + PAGE_SIZE * 4;
> +	uvcb_cgc.guest_sca = get_max_ram_size() + PAGE_SIZE * 4;
>   	rc = uv_call(0, (uint64_t)&uvcb_cgc);
>   	report(uvcb_cgc.header.rc == 0x10d && rc == 1,
>   	       "sca inaccessible");

Steffen