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

[kvm-unit-tests PATCH v2 0/8] 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 its satellites.

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


v2:
	* Moved 3d access check in function
	* Small fixes
	* Added two more fix patches


Janosch Frank (8):
  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: Fence against being run as a PV guest
  s390x: uv-host: Fix init storage origin and length check

 s390x/uv-host.c | 256 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 246 insertions(+), 10 deletions(-)

-- 
2.34.1

[kvm-unit-tests PATCH v2 1/8] 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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index a1a6d120..983cb4a1 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -43,6 +43,24 @@ static void cpu_loop(void)
 	for (;;) {}
 }
 
+/*
+ * Checks if a memory area is protected as secure memory.
+ * Will return true if all pages are protected, false otherwise.
+ */
+static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
+{
+	while (len) {
+		expect_pgm_int();
+		*access_ptr += 42;
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+		access_ptr += PAGE_SIZE / sizeof(access_ptr);
+		len -= PAGE_SIZE;
+	}
+
+	return true;
+}
+
 static struct cmd_list cmds[] = {
 	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
 	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
@@ -194,6 +212,10 @@ static void test_cpu_create(void)
 	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
 	       uvcb_csc.cpu_handle, "success");
 
+	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
+			     uvcb_qui.cpu_stor_len);
+	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);
@@ -292,6 +314,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
+	report(rc, "Base storage protection");
+
+	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
+			     uvcb_qui.conf_base_phys_stor_len);
+	report(rc, "Variable storage protection");
+
 	uvcb_cgc.header.rc = 0;
 	uvcb_cgc.header.rrc = 0;
 	tmp = uvcb_cgc.guest_handle;
-- 
2.34.1

[kvm-unit-tests PATCH v2 2/8] 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>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 983cb4a1..5aeacb42 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -101,6 +101,25 @@ 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");
+
+	for (i = 0; cmds[i].name; i++) {
+		if (cmds[i].cmd == UVC_CMD_INIT_UV)
+			continue;
+		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;
@@ -468,13 +487,68 @@ static void test_invalid(void)
 	report_prefix_pop();
 }
 
+static void setup_test_clear(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.
+	 */
+	setup_test_clear();
 
 	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)
@@ -505,6 +579,7 @@ int main(void)
 
 	test_priv();
 	test_invalid();
+	test_uv_uninitialized();
 	test_query();
 	test_init();
 
-- 
2.34.1

[kvm-unit-tests PATCH v2 3/8] 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 5aeacb42..0762e690 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -82,6 +82,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 = {};
@@ -577,6 +599,7 @@ int main(void)
 		goto done;
 	}
 
+	test_i3();
 	test_priv();
 	test_invalid();
 	test_uv_uninitialized();
-- 
2.34.1

[kvm-unit-tests PATCH v2 4/8] 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>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Reviewed-by: Nico Boehr <nrb@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 0762e690..a626a651 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>
 
@@ -142,6 +144,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;
@@ -607,6 +657,8 @@ int main(void)
 	test_init();
 
 	setup_vmem();
+	test_access();
+
 	test_config_create();
 	test_cpu_create();
 	test_cpu_destroy();
-- 
2.34.1

[kvm-unit-tests PATCH v2 5/8] 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>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
---
 s390x/uv-host.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index a626a651..0044e8b9 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -248,6 +248,53 @@ 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);
+
+	free_pages(pages);
+	report_prefix_pop();
+}
+
 static void test_cpu_create(void)
 {
 	int rc;
@@ -661,6 +708,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

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

[Janosch Frank]

One + is definitely enough here.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@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 0044e8b9..af3122a8 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -443,7 +443,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

[kvm-unit-tests PATCH v2 7/8] s390x: uv-host: Fence against being run as a PV guest

[Janosch Frank]

This test checks the UV host API so we should skip it if we're a PV
guest.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index af3122a8..1ed8ded1 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -695,6 +695,10 @@ int main(void)
 		report_skip("Ultravisor call facility is not available");
 		goto done;
 	}
+	if (!uv_os_is_host()) {
+		report_skip("This test needs to be run in a UV host environment");
+		goto done;
+	}
 
 	test_i3();
 	test_priv();
-- 
2.34.1

[kvm-unit-tests PATCH v2 8/8] s390x: uv-host: Fix init storage origin and length check

[Janosch Frank]

The origin and length are masked with the HPAGE_MASK and PAGE_MASK
respectively so adding a few bytes doesn't matter at all.

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

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index 1ed8ded1..b1412a20 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -516,17 +516,22 @@ static void test_init(void)
 	       "storage invalid length");
 	uvcb_init.stor_len += 8;
 
-	uvcb_init.stor_origin =  get_max_ram_size() + 8;
+	/* Storage origin is 1MB aligned, the length is 4KB aligned */
+	uvcb_init.stor_origin = get_max_ram_size();
 	rc = uv_call(0, (uint64_t)&uvcb_init);
-	report(rc == 1 && uvcb_init.header.rc == 0x104,
+	report(rc == 1 && (uvcb_init.header.rc == 0x104 || uvcb_init.header.rc == 0x105),
 	       "storage origin invalid");
 	uvcb_init.stor_origin = mem;
 
-	uvcb_init.stor_origin = get_max_ram_size() - 8;
-	rc = uv_call(0, (uint64_t)&uvcb_init);
-	report(rc == 1 && uvcb_init.header.rc == 0x105,
-	       "storage + length invalid");
-	uvcb_init.stor_origin = mem;
+	if (uvcb_init.stor_len >= HPAGE_SIZE) {
+		uvcb_init.stor_origin = get_max_ram_size() - HPAGE_SIZE;
+		rc = uv_call(0, (uint64_t)&uvcb_init);
+		report(rc == 1 && uvcb_init.header.rc == 0x105,
+		       "storage + length invalid");
+		uvcb_init.stor_origin = mem;
+	} else {
+		report_skip("storage + length invalid, stor_len < HPAGE_SIZE");
+	}
 
 	uvcb_init.stor_origin = 1UL << 30;
 	rc = uv_call(0, (uint64_t)&uvcb_init);
-- 
2.34.1

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

[Claudio Imbrenda]

On Wed,  6 Jul 2022 06:40:17 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index a1a6d120..983cb4a1 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -43,6 +43,24 @@ static void cpu_loop(void)
>  	for (;;) {}
>  }
>  
> +/*
> + * Checks if a memory area is protected as secure memory.
> + * Will return true if all pages are protected, false otherwise.
> + */
> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
> +{
> +	while (len) {
> +		expect_pgm_int();
> +		*access_ptr += 42;

I'm surprised this works, you will get an (expected) exception when
reading from the pointer, and then you should get another one (at this
point unexpected) when writing

> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
> +		len -= PAGE_SIZE;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cmd_list cmds[] = {
>  	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>  	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
> @@ -194,6 +212,10 @@ static void test_cpu_create(void)
>  	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>  	       uvcb_csc.cpu_handle, "success");
>  
> +	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
> +			     uvcb_qui.cpu_stor_len);
> +	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);
> @@ -292,6 +314,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
> +	report(rc, "Base storage protection");
> +
> +	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
> +			     uvcb_qui.conf_base_phys_stor_len);
> +	report(rc, "Variable storage protection");
> +
>  	uvcb_cgc.header.rc = 0;
>  	uvcb_cgc.header.rrc = 0;
>  	tmp = uvcb_cgc.guest_handle;

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

[Steffen Eiden]

Hi Janosch,

On 7/6/22 08:40, 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>
> ---
>   s390x/uv-host.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index a1a6d120..983cb4a1 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -43,6 +43,24 @@ static void cpu_loop(void)
>   	for (;;) {}
>   }
>   
> +/*
> + * Checks if a memory area is protected as secure memory.
> + * Will return true if all pages are protected, false otherwise.
> + */
> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
> +{
> +	while (len) {
> +		expect_pgm_int();
> +		*access_ptr += 42;
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
> +		len -= PAGE_SIZE;
If someone uses this function with 'len' not being a multiple of
PAGE_SIZE this test does not for what is was intended by testing more 
memory than expected.

I suggest adding an explicit assert at the beginning of the function
that ensures 'len' is a multiple of PAGE_SIZE.

> +	}
> +
> +	return true;
> +}
> +

[snip]

Steffen

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

[Janosch Frank]

On 7/6/22 18:33, Claudio Imbrenda wrote:
> On Wed,  6 Jul 2022 06:40:17 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> 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 | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index a1a6d120..983cb4a1 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -43,6 +43,24 @@ static void cpu_loop(void)
>>   	for (;;) {}
>>   }
>>   
>> +/*
>> + * Checks if a memory area is protected as secure memory.
>> + * Will return true if all pages are protected, false otherwise.
>> + */
>> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
>> +{
>> +	while (len) {
>> +		expect_pgm_int();
>> +		*access_ptr += 42;
> 
> I'm surprised this works, you will get an (expected) exception when
> reading from the pointer, and then you should get another one (at this
> point unexpected) when writing
> 

Let me introduce you to "AGSI" add grand storage immediate.
But I get your point, inline assembly would make this much more explicit.

>> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
>> +			return false;
>> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
>> +		len -= PAGE_SIZE;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   static struct cmd_list cmds[] = {
>>   	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>>   	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
>> @@ -194,6 +212,10 @@ static void test_cpu_create(void)
>>   	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>>   	       uvcb_csc.cpu_handle, "success");
>>   
>> +	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
>> +			     uvcb_qui.cpu_stor_len);
>> +	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);
>> @@ -292,6 +314,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
>> +	report(rc, "Base storage protection");
>> +
>> +	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
>> +			     uvcb_qui.conf_base_phys_stor_len);
>> +	report(rc, "Variable storage protection");
>> +
>>   	uvcb_cgc.header.rc = 0;
>>   	uvcb_cgc.header.rrc = 0;
>>   	tmp = uvcb_cgc.guest_handle;
> 

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

[Janosch Frank]

On 7/7/22 10:11, Steffen Eiden wrote:
> Hi Janosch,
> 
> On 7/6/22 08:40, 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>
>> ---
>>    s390x/uv-host.c | 29 +++++++++++++++++++++++++++++
>>    1 file changed, 29 insertions(+)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index a1a6d120..983cb4a1 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -43,6 +43,24 @@ static void cpu_loop(void)
>>    	for (;;) {}
>>    }
>>    
>> +/*
>> + * Checks if a memory area is protected as secure memory.
>> + * Will return true if all pages are protected, false otherwise.
>> + */
>> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
>> +{
>> +	while (len) {
>> +		expect_pgm_int();
>> +		*access_ptr += 42;
>> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
>> +			return false;
>> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
>> +		len -= PAGE_SIZE;
> If someone uses this function with 'len' not being a multiple of
> PAGE_SIZE this test does not for what is was intended by testing more
> memory than expected.
> 
> I suggest adding an explicit assert at the beginning of the function
> that ensures 'len' is a multiple of PAGE_SIZE.

Sure

> 
>> +	}
>> +
>> +	return true;
>> +}
>> +
> 
> [snip]
> 
> Steffen

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

[Claudio Imbrenda]

On Thu, 7 Jul 2022 10:16:44 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/6/22 18:33, Claudio Imbrenda wrote:
> > On Wed,  6 Jul 2022 06:40:17 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> 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 | 29 +++++++++++++++++++++++++++++
> >>   1 file changed, 29 insertions(+)
> >>
> >> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> >> index a1a6d120..983cb4a1 100644
> >> --- a/s390x/uv-host.c
> >> +++ b/s390x/uv-host.c
> >> @@ -43,6 +43,24 @@ static void cpu_loop(void)
> >>   	for (;;) {}
> >>   }
> >>   
> >> +/*
> >> + * Checks if a memory area is protected as secure memory.
> >> + * Will return true if all pages are protected, false otherwise.
> >> + */
> >> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
> >> +{
> >> +	while (len) {
> >> +		expect_pgm_int();
> >> +		*access_ptr += 42;  
> > 
> > I'm surprised this works, you will get an (expected) exception when
> > reading from the pointer, and then you should get another one (at this
> > point unexpected) when writing
> >   
> 
> Let me introduce you to "AGSI" add grand storage immediate.

wow, of course there is an instruction for that :D

> But I get your point, inline assembly would make this much more explicit.

actually, I think you should separately check for read and write access.

something like 

expect_pgm_int();
READ_ONCE(*access_ptr);
...

expect_pgm_int();
WRITE_ONCE(*access_ptr, 42);

to really make sure both read and write access are blocked

> 
> >> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> >> +			return false;
> >> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
> >> +		len -= PAGE_SIZE;
> >> +	}
> >> +
> >> +	return true;
> >> +}
> >> +
> >>   static struct cmd_list cmds[] = {
> >>   	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
> >>   	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
> >> @@ -194,6 +212,10 @@ static void test_cpu_create(void)
> >>   	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
> >>   	       uvcb_csc.cpu_handle, "success");
> >>   
> >> +	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
> >> +			     uvcb_qui.cpu_stor_len);
> >> +	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);
> >> @@ -292,6 +314,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
> >> +	report(rc, "Base storage protection");
> >> +
> >> +	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
> >> +			     uvcb_qui.conf_base_phys_stor_len);
> >> +	report(rc, "Variable storage protection");
> >> +
> >>   	uvcb_cgc.header.rc = 0;
> >>   	uvcb_cgc.header.rrc = 0;
> >>   	tmp = uvcb_cgc.guest_handle;  
> >   
> 

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

[Steffen Eiden]

On 7/6/22 08:40, 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>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   s390x/uv-host.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 983cb4a1..5aeacb42 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -101,6 +101,25 @@ 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");
> +
> +	for (i = 0; cmds[i].name; i++) {
> +		if (cmds[i].cmd == UVC_CMD_INIT_UV)
> +			continue;
> +		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;
> @@ -468,13 +487,68 @@ static void test_invalid(void)
>   	report_prefix_pop();
>   }
>   
> +static void setup_test_clear(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.
> +	 */
> +	setup_test_clear();
>   
>   	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)
> @@ -505,6 +579,7 @@ int main(void)
>   
>   	test_priv();
>   	test_invalid();
> +	test_uv_uninitialized();
>   	test_query();
>   	test_init();
>   

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

[Steffen Eiden]

On 7/6/22 08:40, 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>
Reviewed-by: Steffen Eiden <seiden@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 5aeacb42..0762e690 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -82,6 +82,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 = {};
> @@ -577,6 +599,7 @@ int main(void)
>   		goto done;
>   	}
>   
> +	test_i3();
>   	test_priv();
>   	test_invalid();
>   	test_uv_uninitialized();

Re: [kvm-unit-tests PATCH v2 7/8] s390x: uv-host: Fence against being run as a PV guest

[Steffen Eiden]

On 7/6/22 08:40, Janosch Frank wrote:
> This test checks the UV host API so we should skip it if we're a PV
> guest.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   s390x/uv-host.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index af3122a8..1ed8ded1 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -695,6 +695,10 @@ int main(void)
>   		report_skip("Ultravisor call facility is not available");
>   		goto done;
>   	}
> +	if (!uv_os_is_host()) {
> +		report_skip("This test needs to be run in a UV host environment");
> +		goto done;
> +	}
>   
>   	test_i3();
>   	test_priv();

Re: [kvm-unit-tests PATCH v2 8/8] s390x: uv-host: Fix init storage origin and length check

[Steffen Eiden]

On 7/6/22 08:40, Janosch Frank wrote:
> The origin and length are masked with the HPAGE_MASK and PAGE_MASK
> respectively so adding a few bytes doesn't matter at all.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   s390x/uv-host.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index 1ed8ded1..b1412a20 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -516,17 +516,22 @@ static void test_init(void)
>   	       "storage invalid length");
>   	uvcb_init.stor_len += 8;
>   
> -	uvcb_init.stor_origin =  get_max_ram_size() + 8;
> +	/* Storage origin is 1MB aligned, the length is 4KB aligned */
> +	uvcb_init.stor_origin = get_max_ram_size();
>   	rc = uv_call(0, (uint64_t)&uvcb_init);
> -	report(rc == 1 && uvcb_init.header.rc == 0x104,
> +	report(rc == 1 && (uvcb_init.header.rc == 0x104 || uvcb_init.header.rc == 0x105),
>   	       "storage origin invalid");
>   	uvcb_init.stor_origin = mem;
>   
> -	uvcb_init.stor_origin = get_max_ram_size() - 8;
> -	rc = uv_call(0, (uint64_t)&uvcb_init);
> -	report(rc == 1 && uvcb_init.header.rc == 0x105,
> -	       "storage + length invalid");
> -	uvcb_init.stor_origin = mem;
> +	if (uvcb_init.stor_len >= HPAGE_SIZE) {
> +		uvcb_init.stor_origin = get_max_ram_size() - HPAGE_SIZE;
> +		rc = uv_call(0, (uint64_t)&uvcb_init);
> +		report(rc == 1 && uvcb_init.header.rc == 0x105,
> +		       "storage + length invalid");
> +		uvcb_init.stor_origin = mem;
> +	} else {
> +		report_skip("storage + length invalid, stor_len < HPAGE_SIZE");
> +	}
>   
>   	uvcb_init.stor_origin = 1UL << 30;
>   	rc = uv_call(0, (uint64_t)&uvcb_init);

[kvm-unit-tests PATCH v3] 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 | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index dfcebe10..ba6c9008 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -45,6 +45,32 @@ static void cpu_loop(void)
 	for (;;) {}
 }
 
+/*
+ * Checks if a memory area is protected as secure memory.
+ * Will return true if all pages are protected, false otherwise.
+ */
+static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
+{
+	assert(!(len & ~PAGE_MASK));
+	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
+
+	while (len) {
+		expect_pgm_int();
+		READ_ONCE(*access_ptr);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+		expect_pgm_int();
+		WRITE_ONCE(*access_ptr, 42);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+
+		access_ptr += PAGE_SIZE / sizeof(access_ptr);
+		len -= PAGE_SIZE;
+	}
+
+	return true;
+}
+
 static struct cmd_list cmds[] = {
 	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
 	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
@@ -332,6 +358,10 @@ static void test_cpu_create(void)
 	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
 	       uvcb_csc.cpu_handle, "success");
 
+	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
+			     uvcb_qui.cpu_stor_len);
+	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);
@@ -430,6 +460,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
+	report(rc, "Base storage protection");
+
+	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
+			     uvcb_qui.conf_base_phys_stor_len);
+	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 v3] s390x: uv-host: Add access checks for donated memory

[Nico Boehr]

Quoting Janosch Frank (2022-07-25 15:08:59)
> 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>

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

[Claudio Imbrenda]

On Mon, 25 Jul 2022 13:08:59 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> 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 | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index dfcebe10..ba6c9008 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -45,6 +45,32 @@ static void cpu_loop(void)
>  	for (;;) {}
>  }
>  
> +/*
> + * Checks if a memory area is protected as secure memory.
> + * Will return true if all pages are protected, false otherwise.
> + */
> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
> +{
> +	assert(!(len & ~PAGE_MASK));
> +	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
> +
> +	while (len) {
> +		expect_pgm_int();
> +		READ_ONCE(*access_ptr);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +		expect_pgm_int();
> +		WRITE_ONCE(*access_ptr, 42);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +
> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);

this looks ugly, although in principle the correct way to handle a
pointer. In this specific case it's techically wrong though (you
actually want sizeof(*access_ptr) )

what about making access_ptr a char*?

then you can do just

	access_ptr += PAGE_SIZE;

and you can keep the READ_ONCE and WRITE_ONCE as they are

> +		len -= PAGE_SIZE;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cmd_list cmds[] = {
>  	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>  	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
> @@ -332,6 +358,10 @@ static void test_cpu_create(void)
>  	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>  	       uvcb_csc.cpu_handle, "success");
>  
> +	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
> +			     uvcb_qui.cpu_stor_len);
> +	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);
> @@ -430,6 +460,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
> +	report(rc, "Base storage protection");
> +
> +	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
> +			     uvcb_qui.conf_base_phys_stor_len);
> +	report(rc, "Variable storage protection");
> +
>  	uvcb_cgc.header.rc = 0;
>  	uvcb_cgc.header.rrc = 0;
>  	tmp = uvcb_cgc.guest_handle;

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

[Janosch Frank]

On 8/3/22 11:46, Claudio Imbrenda wrote:
> On Mon, 25 Jul 2022 13:08:59 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> 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 | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
>> index dfcebe10..ba6c9008 100644
>> --- a/s390x/uv-host.c
>> +++ b/s390x/uv-host.c
>> @@ -45,6 +45,32 @@ static void cpu_loop(void)
>>   	for (;;) {}
>>   }
>>   
>> +/*
>> + * Checks if a memory area is protected as secure memory.
>> + * Will return true if all pages are protected, false otherwise.
>> + */
>> +static bool access_check_3d(uint64_t *access_ptr, uint64_t len)
>> +{
>> +	assert(!(len & ~PAGE_MASK));
>> +	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
>> +
>> +	while (len) {
>> +		expect_pgm_int();
>> +		READ_ONCE(*access_ptr);
>> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
>> +			return false;
>> +		expect_pgm_int();
>> +		WRITE_ONCE(*access_ptr, 42);
>> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
>> +			return false;
>> +
>> +		access_ptr += PAGE_SIZE / sizeof(access_ptr);
> 
> this looks ugly, although in principle the correct way to handle a
> pointer. In this specific case it's techically wrong though (you
> actually want sizeof(*access_ptr) )
> 
> what about making access_ptr a char*?
> 
> then you can do just
> 
> 	access_ptr += PAGE_SIZE;
> 
> and you can keep the READ_ONCE and WRITE_ONCE as they are

Sure

> 
>> +		len -= PAGE_SIZE;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   static struct cmd_list cmds[] = {
>>   	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>>   	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
>> @@ -332,6 +358,10 @@ static void test_cpu_create(void)
>>   	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>>   	       uvcb_csc.cpu_handle, "success");
>>   
>> +	rc = access_check_3d((uint64_t *)uvcb_csc.stor_origin,
>> +			     uvcb_qui.cpu_stor_len);
>> +	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);
>> @@ -430,6 +460,13 @@ 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 = access_check_3d((uint64_t *)uvcb_cgc.conf_var_stor_origin, vsize);
>> +	report(rc, "Base storage protection");
>> +
>> +	rc = access_check_3d((uint64_t *)uvcb_cgc.conf_base_stor_origin,
>> +			     uvcb_qui.conf_base_phys_stor_len);
>> +	report(rc, "Variable storage protection");
>> +
>>   	uvcb_cgc.header.rc = 0;
>>   	uvcb_cgc.header.rrc = 0;
>>   	tmp = uvcb_cgc.guest_handle;
> 

[kvm-unit-tests PATCH v4] 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>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/uv-host.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index dfcebe10..8d2da5d3 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -45,6 +45,32 @@ static void cpu_loop(void)
 	for (;;) {}
 }
 
+/*
+ * Checks if a memory area is protected as secure memory.
+ * Will return true if all pages are protected, false otherwise.
+ */
+static bool access_check_3d(uint8_t *access_ptr, uint64_t len)
+{
+	assert(!(len & ~PAGE_MASK));
+	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
+
+	while (len) {
+		expect_pgm_int();
+		READ_ONCE(*access_ptr);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+		expect_pgm_int();
+		WRITE_ONCE(*access_ptr, 42);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+
+		access_ptr += PAGE_SIZE;
+		len -= PAGE_SIZE;
+	}
+
+	return true;
+}
+
 static struct cmd_list cmds[] = {
 	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
 	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
@@ -332,6 +358,10 @@ static void test_cpu_create(void)
 	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
 	       uvcb_csc.cpu_handle, "success");
 
+	rc = access_check_3d((uint8_t *)uvcb_csc.stor_origin,
+			     uvcb_qui.cpu_stor_len);
+	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);
@@ -430,6 +460,13 @@ 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 = access_check_3d((uint8_t *)uvcb_cgc.conf_var_stor_origin, vsize);
+	report(rc, "Base storage protection");
+
+	rc = access_check_3d((uint8_t *)uvcb_cgc.conf_base_stor_origin,
+			     uvcb_qui.conf_base_phys_stor_len);
+	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 v4] s390x: uv-host: Add access checks for donated memory

[Claudio Imbrenda]

On Thu, 11 Aug 2022 13:18:24 +0000
Janosch Frank <frankja@linux.ibm.com> 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>
> ---
>  s390x/uv-host.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index dfcebe10..8d2da5d3 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -45,6 +45,32 @@ static void cpu_loop(void)
>  	for (;;) {}
>  }
>  
> +/*
> + * Checks if a memory area is protected as secure memory.
> + * Will return true if all pages are protected, false otherwise.
> + */
> +static bool access_check_3d(uint8_t *access_ptr, uint64_t len)
> +{
> +	assert(!(len & ~PAGE_MASK));
> +	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
> +
> +	while (len) {
> +		expect_pgm_int();
> +		READ_ONCE(*access_ptr);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +		expect_pgm_int();
> +		WRITE_ONCE(*access_ptr, 42);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +
> +		access_ptr += PAGE_SIZE;
> +		len -= PAGE_SIZE;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cmd_list cmds[] = {
>  	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>  	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
> @@ -332,6 +358,10 @@ static void test_cpu_create(void)
>  	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>  	       uvcb_csc.cpu_handle, "success");
>  
> +	rc = access_check_3d((uint8_t *)uvcb_csc.stor_origin,
> +			     uvcb_qui.cpu_stor_len);
> +	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);
> @@ -430,6 +460,13 @@ 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 = access_check_3d((uint8_t *)uvcb_cgc.conf_var_stor_origin, vsize);
> +	report(rc, "Base storage protection");

I think you mixed up this ^

> +
> +	rc = access_check_3d((uint8_t *)uvcb_cgc.conf_base_stor_origin,
> +			     uvcb_qui.conf_base_phys_stor_len);
> +	report(rc, "Variable storage protection");

with this ^

> +
>  	uvcb_cgc.header.rc = 0;
>  	uvcb_cgc.header.rrc = 0;
>  	tmp = uvcb_cgc.guest_handle;

[kvm-unit-tests PATCH v5] 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>
Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
---
This patch is clearly cursed :)
---
 s390x/uv-host.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/s390x/uv-host.c b/s390x/uv-host.c
index dfcebe10..191e8b3f 100644
--- a/s390x/uv-host.c
+++ b/s390x/uv-host.c
@@ -45,6 +45,32 @@ static void cpu_loop(void)
 	for (;;) {}
 }
 
+/*
+ * Checks if a memory area is protected as secure memory.
+ * Will return true if all pages are protected, false otherwise.
+ */
+static bool access_check_3d(uint8_t *access_ptr, uint64_t len)
+{
+	assert(!(len & ~PAGE_MASK));
+	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
+
+	while (len) {
+		expect_pgm_int();
+		READ_ONCE(*access_ptr);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+		expect_pgm_int();
+		WRITE_ONCE(*access_ptr, 42);
+		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
+			return false;
+
+		access_ptr += PAGE_SIZE;
+		len -= PAGE_SIZE;
+	}
+
+	return true;
+}
+
 static struct cmd_list cmds[] = {
 	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
 	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
@@ -332,6 +358,10 @@ static void test_cpu_create(void)
 	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
 	       uvcb_csc.cpu_handle, "success");
 
+	rc = access_check_3d((uint8_t *)uvcb_csc.stor_origin,
+			     uvcb_qui.cpu_stor_len);
+	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);
@@ -430,6 +460,13 @@ 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 = access_check_3d((uint8_t *)uvcb_cgc.conf_base_stor_origin,
+			     uvcb_qui.conf_base_phys_stor_len);
+	report(rc, "Base storage protection");
+
+	rc = access_check_3d((uint8_t *)uvcb_cgc.conf_var_stor_origin, vsize);
+	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 v5] s390x: uv-host: Add access checks for donated memory

[Claudio Imbrenda]

On Thu, 11 Aug 2022 15:00:39 +0000
Janosch Frank <frankja@linux.ibm.com> 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>

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

> ---
> This patch is clearly cursed :)
> ---
>  s390x/uv-host.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/s390x/uv-host.c b/s390x/uv-host.c
> index dfcebe10..191e8b3f 100644
> --- a/s390x/uv-host.c
> +++ b/s390x/uv-host.c
> @@ -45,6 +45,32 @@ static void cpu_loop(void)
>  	for (;;) {}
>  }
>  
> +/*
> + * Checks if a memory area is protected as secure memory.
> + * Will return true if all pages are protected, false otherwise.
> + */
> +static bool access_check_3d(uint8_t *access_ptr, uint64_t len)
> +{
> +	assert(!(len & ~PAGE_MASK));
> +	assert(!((uint64_t)access_ptr & ~PAGE_MASK));
> +
> +	while (len) {
> +		expect_pgm_int();
> +		READ_ONCE(*access_ptr);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +		expect_pgm_int();
> +		WRITE_ONCE(*access_ptr, 42);
> +		if (clear_pgm_int() != PGM_INT_CODE_SECURE_STOR_ACCESS)
> +			return false;
> +
> +		access_ptr += PAGE_SIZE;
> +		len -= PAGE_SIZE;
> +	}
> +
> +	return true;
> +}
> +
>  static struct cmd_list cmds[] = {
>  	{ "init", UVC_CMD_INIT_UV, sizeof(struct uv_cb_init), BIT_UVC_CMD_INIT_UV },
>  	{ "create conf", UVC_CMD_CREATE_SEC_CONF, sizeof(struct uv_cb_cgc), BIT_UVC_CMD_CREATE_SEC_CONF },
> @@ -332,6 +358,10 @@ static void test_cpu_create(void)
>  	report(rc == 0 && uvcb_csc.header.rc == UVC_RC_EXECUTED &&
>  	       uvcb_csc.cpu_handle, "success");
>  
> +	rc = access_check_3d((uint8_t *)uvcb_csc.stor_origin,
> +			     uvcb_qui.cpu_stor_len);
> +	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);
> @@ -430,6 +460,13 @@ 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 = access_check_3d((uint8_t *)uvcb_cgc.conf_base_stor_origin,
> +			     uvcb_qui.conf_base_phys_stor_len);
> +	report(rc, "Base storage protection");
> +
> +	rc = access_check_3d((uint8_t *)uvcb_cgc.conf_var_stor_origin, vsize);
> +	report(rc, "Variable storage protection");
> +
>  	uvcb_cgc.header.rc = 0;
>  	uvcb_cgc.header.rrc = 0;
>  	tmp = uvcb_cgc.guest_handle;