[kvm-unit-tests PATCH v1 0/2] s390x: add migration test for storage keys

[kvm-unit-tests PATCH v1 0/2] s390x: add migration test for storage keys

[Nico Boehr]

Upon migration, we expect storage keys being set by the guest to be preserved,
so add a test for it.

We keep 128 pages and set predictable storage keys. Then, we migrate and check
they can be read back and the respective access restrictions are in place when
the access key in the PSW doesn't match.

TCG currently doesn't implement key-controlled protection, hence add the
relevant tests as xfails. To this end, a check_pgm_int_xfail() is useful, which
is also added in this series.

Nico Boehr (2):
  lib: s390x: introduce check_pgm_int_code_xfail()
  s390x: add migration test for storage keys

 lib/s390x/asm/interrupt.h |  1 +
 lib/s390x/interrupt.c     |  9 +++-
 s390x/Makefile            |  1 +
 s390x/migration-skey.c    | 98 +++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg       |  4 ++
 5 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 s390x/migration-skey.c

-- 
2.31.1

[kvm-unit-tests PATCH v1 1/2] lib: s390x: introduce check_pgm_int_code_xfail()

[Nico Boehr]

Right now, it is not very convenient to have expected failures when checking for
program interrupts. Let's introduce check_pgm_int_code_xfail() with an API
similar to report_xfail() to make the programmer's life easier.

With this, we can express check_pgm_int_code() as a special case of
check_pgm_int_code_xfail() with xfail = false.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h | 1 +
 lib/s390x/interrupt.c     | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index d9ab0bd781c9..88731da9e341 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -46,6 +46,7 @@ void handle_svc_int(void);
 void expect_pgm_int(void);
 void expect_ext_int(void);
 uint16_t clear_pgm_int(void);
+void check_pgm_int_code_xfail(bool xfail, uint16_t code);
 void check_pgm_int_code(uint16_t code);
 
 /* Activate low-address protection */
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 27d3b767210f..b61f7d588550 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -47,14 +47,19 @@ uint16_t clear_pgm_int(void)
 	return code;
 }
 
-void check_pgm_int_code(uint16_t code)
+void check_pgm_int_code_xfail(bool xfail, uint16_t code)
 {
 	mb();
-	report(code == lc->pgm_int_code,
+	report_xfail(xfail, code == lc->pgm_int_code,
 	       "Program interrupt: expected(%d) == received(%d)", code,
 	       lc->pgm_int_code);
 }
 
+void check_pgm_int_code(uint16_t code)
+{
+	check_pgm_int_code_xfail(false, code);
+}
+
 void register_pgm_cleanup_func(void (*f)(void))
 {
 	pgm_cleanup_func = f;
-- 
2.31.1

Re: [kvm-unit-tests PATCH v1 1/2] lib: s390x: introduce check_pgm_int_code_xfail()

[Claudio Imbrenda]

On Thu, 12 May 2022 16:01:06 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Right now, it is not very convenient to have expected failures when checking for
> program interrupts. Let's introduce check_pgm_int_code_xfail() with an API
> similar to report_xfail() to make the programmer's life easier.
> 
> With this, we can express check_pgm_int_code() as a special case of
> check_pgm_int_code_xfail() with xfail = false.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 1 +
>  lib/s390x/interrupt.c     | 9 +++++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index d9ab0bd781c9..88731da9e341 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -46,6 +46,7 @@ void handle_svc_int(void);
>  void expect_pgm_int(void);
>  void expect_ext_int(void);
>  uint16_t clear_pgm_int(void);
> +void check_pgm_int_code_xfail(bool xfail, uint16_t code);
>  void check_pgm_int_code(uint16_t code);

... here ^ (see below)

>  
>  /* Activate low-address protection */
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 27d3b767210f..b61f7d588550 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -47,14 +47,19 @@ uint16_t clear_pgm_int(void)
>  	return code;
>  }
>  
> -void check_pgm_int_code(uint16_t code)
> +void check_pgm_int_code_xfail(bool xfail, uint16_t code)
>  {
>  	mb();
> -	report(code == lc->pgm_int_code,
> +	report_xfail(xfail, code == lc->pgm_int_code,
>  	       "Program interrupt: expected(%d) == received(%d)", code,
>  	       lc->pgm_int_code);
>  }
>  
> +void check_pgm_int_code(uint16_t code)
> +{
> +	check_pgm_int_code_xfail(false, code);
> +}
> +

... maybe at this point make it a macroid (static inline function)
directly in the .h ?

>  void register_pgm_cleanup_func(void (*f)(void))
>  {
>  	pgm_cleanup_func = f;

[kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Nico Boehr]

Upon migration, we expect storage keys being set by the guest to be preserved,
so add a test for it.

We keep 128 pages and set predictable storage keys. Then, we migrate and check
they can be read back and the respective access restrictions are in place when
the access key in the PSW doesn't match.

TCG currently doesn't implement key-controlled protection, see
target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
tests as xfails.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile         |  1 +
 s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg    |  4 ++
 3 files changed, 103 insertions(+)
 create mode 100644 s390x/migration-skey.c

diff --git a/s390x/Makefile b/s390x/Makefile
index a8e04aa6fe4d..f8ea594b641d 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -32,6 +32,7 @@ tests += $(TEST_DIR)/epsw.elf
 tests += $(TEST_DIR)/adtl-status.elf
 tests += $(TEST_DIR)/migration.elf
 tests += $(TEST_DIR)/pv-attest.elf
+tests += $(TEST_DIR)/migration-skey.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
new file mode 100644
index 000000000000..6f3053d8ab40
--- /dev/null
+++ b/s390x/migration-skey.c
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Storage Key migration tests
+ *
+ * Copyright IBM Corp. 2022
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+
+#include <libcflat.h>
+#include <asm/facility.h>
+#include <asm/page.h>
+#include <asm/mem.h>
+#include <asm/interrupt.h>
+#include <hardware.h>
+
+#define NUM_PAGES 128
+static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
+
+static void test_migration(void)
+{
+	int i, key_to_set;
+	uint8_t *page;
+	union skey expected_key, actual_key, mismatching_key;
+
+	for (i = 0; i < NUM_PAGES; i++) {
+		/*
+		 * Storage keys are 7 bit, lowest bit is always returned as zero
+		 * by iske
+		 */
+		key_to_set = i * 2;
+		set_storage_key(pagebuf + i, key_to_set, 1);
+	}
+
+	puts("Please migrate me, then press return\n");
+	(void)getchar();
+
+	for (i = 0; i < NUM_PAGES; i++) {
+		report_prefix_pushf("page %d", i);
+
+		page = &pagebuf[i][0];
+		actual_key.val = get_storage_key(page);
+		expected_key.val = i * 2;
+
+		/* ignore reference bit */
+		actual_key.str.rf = 0;
+		expected_key.str.rf = 0;
+
+		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
+
+		/* ensure access key doesn't match storage key and is never zero */
+		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
+		*page = 0xff;
+
+		expect_pgm_int();
+		asm volatile (
+			/* set access key */
+			"spka 0(%[mismatching_key])\n"
+			/* try to write page */
+			"mvi 0(%[page]), 42\n"
+			/* reset access key */
+			"spka 0\n"
+			:
+			: [mismatching_key] "a"(mismatching_key.val),
+			  [page] "a"(page)
+			: "memory"
+		);
+		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
+		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");
+
+		report_prefix_pop();
+	}
+}
+
+int main(void)
+{
+	report_prefix_push("migration-skey");
+	if (test_facility(169)) {
+		report_skip("storage key removal facility is active");
+
+		/*
+		 * If we just exit and don't ask migrate_cmd to migrate us, it
+		 * will just hang forever. Hence, also ask for migration when we
+		 * skip this test alltogether.
+		 */
+		puts("Please migrate me, then press return\n");
+		(void)getchar();
+
+		goto done;
+	}
+
+	test_migration();
+
+done:
+	report_prefix_pop();
+	return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index b456b2881448..1e851d8e3dd8 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -176,3 +176,7 @@ extra_params = -cpu qemu,gs=off,vx=off
 file = migration.elf
 groups = migration
 smp = 2
+
+[migration-skey]
+file = migration-skey.elf
+groups = migration
-- 
2.31.1

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Janosch Frank]

On 5/12/22 16:01, Nico Boehr wrote:
> Upon migration, we expect storage keys being set by the guest to be preserved,
> so add a test for it.
> 
> We keep 128 pages and set predictable storage keys. Then, we migrate and check
> they can be read back and the respective access restrictions are in place when
> the access key in the PSW doesn't match.
> 
> TCG currently doesn't implement key-controlled protection, see
> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
> tests as xfails.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/Makefile         |  1 +
>   s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>   s390x/unittests.cfg    |  4 ++
>   3 files changed, 103 insertions(+)
>   create mode 100644 s390x/migration-skey.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index a8e04aa6fe4d..f8ea594b641d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -32,6 +32,7 @@ tests += $(TEST_DIR)/epsw.elf
>   tests += $(TEST_DIR)/adtl-status.elf
>   tests += $(TEST_DIR)/migration.elf
>   tests += $(TEST_DIR)/pv-attest.elf
> +tests += $(TEST_DIR)/migration-skey.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> new file mode 100644
> index 000000000000..6f3053d8ab40
> --- /dev/null
> +++ b/s390x/migration-skey.c
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage Key migration tests
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/page.h>
> +#include <asm/mem.h>
> +#include <asm/interrupt.h>
> +#include <hardware.h>
> +
> +#define NUM_PAGES 128
> +static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +
> +static void test_migration(void)
> +{
> +	int i, key_to_set;
> +	uint8_t *page;
> +	union skey expected_key, actual_key, mismatching_key;
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		/*
> +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> +		 * by iske
> +		 */
> +		key_to_set = i * 2;
> +		set_storage_key(pagebuf + i, key_to_set, 1);
> +	}
> +
> +	puts("Please migrate me, then press return\n");
> +	(void)getchar();
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		report_prefix_pushf("page %d", i);
> +
> +		page = &pagebuf[i][0];
> +		actual_key.val = get_storage_key(page);
> +		expected_key.val = i * 2;
> +
> +		/* ignore reference bit */
> +		actual_key.str.rf = 0;
> +		expected_key.str.rf = 0;
> +
> +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
> +
> +		/* ensure access key doesn't match storage key and is never zero */
> +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
> +		*page = 0xff;
> +
> +		expect_pgm_int();
> +		asm volatile (
> +			/* set access key */
> +			"spka 0(%[mismatching_key])\n"
> +			/* try to write page */
> +			"mvi 0(%[page]), 42\n"
> +			/* reset access key */
> +			"spka 0\n"
> +			:
> +			: [mismatching_key] "a"(mismatching_key.val),
> +			  [page] "a"(page)
> +			: "memory"
> +		);
> +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);

What's the expected pgm code?
Is it 0 because no pgm was injected?

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Claudio Imbrenda]

On Thu, 12 May 2022 16:01:07 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Upon migration, we expect storage keys being set by the guest to be preserved,
> so add a test for it.
> 
> We keep 128 pages and set predictable storage keys. Then, we migrate and check
> they can be read back and the respective access restrictions are in place when
> the access key in the PSW doesn't match.
> 
> TCG currently doesn't implement key-controlled protection, see
> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
> tests as xfails.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile         |  1 +
>  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg    |  4 ++
>  3 files changed, 103 insertions(+)
>  create mode 100644 s390x/migration-skey.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index a8e04aa6fe4d..f8ea594b641d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -32,6 +32,7 @@ tests += $(TEST_DIR)/epsw.elf
>  tests += $(TEST_DIR)/adtl-status.elf
>  tests += $(TEST_DIR)/migration.elf
>  tests += $(TEST_DIR)/pv-attest.elf
> +tests += $(TEST_DIR)/migration-skey.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> new file mode 100644
> index 000000000000..6f3053d8ab40
> --- /dev/null
> +++ b/s390x/migration-skey.c
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage Key migration tests
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/page.h>
> +#include <asm/mem.h>
> +#include <asm/interrupt.h>
> +#include <hardware.h>
> +
> +#define NUM_PAGES 128
> +static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +
> +static void test_migration(void)
> +{
> +	int i, key_to_set;
> +	uint8_t *page;
> +	union skey expected_key, actual_key, mismatching_key;
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		/*
> +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> +		 * by iske
> +		 */
> +		key_to_set = i * 2;
> +		set_storage_key(pagebuf + i, key_to_set, 1);
> +	}
> +
> +	puts("Please migrate me, then press return\n");
> +	(void)getchar();
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		report_prefix_pushf("page %d", i);
> +
> +		page = &pagebuf[i][0];
> +		actual_key.val = get_storage_key(page);
> +		expected_key.val = i * 2;
> +
> +		/* ignore reference bit */
> +		actual_key.str.rf = 0;
> +		expected_key.str.rf = 0;
> +
> +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
> +
> +		/* ensure access key doesn't match storage key and is never zero */
> +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;

mismatching_key.str.acc = (expected_key.str.acc ^ 2) | 1;

> +		*page = 0xff;
> +
> +		expect_pgm_int();
> +		asm volatile (
> +			/* set access key */
> +			"spka 0(%[mismatching_key])\n"
> +			/* try to write page */
> +			"mvi 0(%[page]), 42\n"
> +			/* reset access key */
> +			"spka 0\n"
> +			:
> +			: [mismatching_key] "a"(mismatching_key.val),
> +			  [page] "a"(page)
> +			: "memory"
> +		);
> +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
> +		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");
> +
> +		report_prefix_pop();
> +	}
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("migration-skey");
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +
> +		/*
> +		 * If we just exit and don't ask migrate_cmd to migrate us, it
> +		 * will just hang forever. Hence, also ask for migration when we
> +		 * skip this test alltogether.
> +		 */
> +		puts("Please migrate me, then press return\n");
> +		(void)getchar();
> +
> +		goto done;
> +	}
> +
> +	test_migration();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b456b2881448..1e851d8e3dd8 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -176,3 +176,7 @@ extra_params = -cpu qemu,gs=off,vx=off
>  file = migration.elf
>  groups = migration
>  smp = 2
> +
> +[migration-skey]
> +file = migration-skey.elf
> +groups = migration

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Nico Boehr]

On Thu, 2022-05-12 at 17:41 +0200, Claudio Imbrenda wrote:
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > new file mode 100644
> > index 000000000000..6f3053d8ab40
[...]
> > +static void test_migration(void)
> > +{
[...]
> > +               /* ensure access key doesn't match storage key and
> > is never zero */
> > +               mismatching_key.str.acc = expected_key.str.acc < 15
> > ? expected_key.str.acc + 1 : 1;
> 
> mismatching_key.str.acc = (expected_key.str.acc ^ 2) | 1;

As discussed in person: I had something like this before and thought it is
easier to understand with the tertiary operator.  So I'd prefer to leave as-is.

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Janis Schoetterl-Glausch]

On 5/12/22 16:01, Nico Boehr wrote:
> Upon migration, we expect storage keys being set by the guest to be preserved,
> so add a test for it.
> 
> We keep 128 pages and set predictable storage keys. Then, we migrate and check
> they can be read back and the respective access restrictions are in place when
> the access key in the PSW doesn't match.
> 
> TCG currently doesn't implement key-controlled protection, see
> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
> tests as xfails.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile         |  1 +
>  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg    |  4 ++
>  3 files changed, 103 insertions(+)
>  create mode 100644 s390x/migration-skey.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index a8e04aa6fe4d..f8ea594b641d 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -32,6 +32,7 @@ tests += $(TEST_DIR)/epsw.elf
>  tests += $(TEST_DIR)/adtl-status.elf
>  tests += $(TEST_DIR)/migration.elf
>  tests += $(TEST_DIR)/pv-attest.elf
> +tests += $(TEST_DIR)/migration-skey.elf
>  
>  pv-tests += $(TEST_DIR)/pv-diags.elf
>  
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> new file mode 100644
> index 000000000000..6f3053d8ab40
> --- /dev/null
> +++ b/s390x/migration-skey.c
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Storage Key migration tests
> + *
> + * Copyright IBM Corp. 2022
> + *
> + * Authors:
> + *  Nico Boehr <nrb@linux.ibm.com>
> + */
> +
> +#include <libcflat.h>
> +#include <asm/facility.h>
> +#include <asm/page.h>
> +#include <asm/mem.h>
> +#include <asm/interrupt.h>
> +#include <hardware.h>
> +
> +#define NUM_PAGES 128
> +static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +
> +static void test_migration(void)
> +{
> +	int i, key_to_set;
> +	uint8_t *page;
> +	union skey expected_key, actual_key, mismatching_key;

I would tend to scope those to the bodies of the respective loop,
but I don't know if that's in accordance with the coding style.
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		/*
> +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> +		 * by iske
> +		 */
> +		key_to_set = i * 2;
> +		set_storage_key(pagebuf + i, key_to_set, 1);

Why not just pagebuf[i]?
> +	}
> +
> +	puts("Please migrate me, then press return\n");
> +	(void)getchar();
> +
> +	for (i = 0; i < NUM_PAGES; i++) {
> +		report_prefix_pushf("page %d", i);
> +
> +		page = &pagebuf[i][0];
> +		actual_key.val = get_storage_key(page);
> +		expected_key.val = i * 2;
> +
> +		/* ignore reference bit */
> +		actual_key.str.rf = 0;
> +		expected_key.str.rf = 0;
> +
> +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
> +
> +		/* ensure access key doesn't match storage key and is never zero */
> +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
> +		*page = 0xff;
> +
> +		expect_pgm_int();
> +		asm volatile (
> +			/* set access key */
> +			"spka 0(%[mismatching_key])\n"
> +			/* try to write page */
> +			"mvi 0(%[page]), 42\n"
> +			/* reset access key */
> +			"spka 0\n"
> +			:
> +			: [mismatching_key] "a"(mismatching_key.val),
> +			  [page] "a"(page)
> +			: "memory"
> +		);
> +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
> +		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");

What are you testing with this bit? If storage keys are really effective after the migration?
I'm wondering if using tprot would not be better, it should simplify the code a lot.
Plus you'd easily test for fetch protection, too.
> +
> +		report_prefix_pop();
> +	}
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("migration-skey");
> +	if (test_facility(169)) {
> +		report_skip("storage key removal facility is active");
> +
> +		/*
> +		 * If we just exit and don't ask migrate_cmd to migrate us, it
> +		 * will just hang forever. Hence, also ask for migration when we
> +		 * skip this test alltogether.

s/alltogether/altogether/

> +		 */
> +		puts("Please migrate me, then press return\n");
> +		(void)getchar();
> +
> +		goto done;
> +	}
> +
> +	test_migration();
> +
> +done:
> +	report_prefix_pop();
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index b456b2881448..1e851d8e3dd8 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -176,3 +176,7 @@ extra_params = -cpu qemu,gs=off,vx=off
>  file = migration.elf
>  groups = migration
>  smp = 2
> +
> +[migration-skey]
> +file = migration-skey.elf
> +groups = migration

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Claudio Imbrenda]

On Fri, 13 May 2022 13:04:34 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On 5/12/22 16:01, Nico Boehr wrote:
> > Upon migration, we expect storage keys being set by the guest to be preserved,
> > so add a test for it.
> > 
> > We keep 128 pages and set predictable storage keys. Then, we migrate and check
> > they can be read back and the respective access restrictions are in place when
> > the access key in the PSW doesn't match.
> > 
> > TCG currently doesn't implement key-controlled protection, see
> > target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
> > tests as xfails.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >  s390x/Makefile         |  1 +
> >  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
> >  s390x/unittests.cfg    |  4 ++
> >  3 files changed, 103 insertions(+)
> >  create mode 100644 s390x/migration-skey.c
> > 
> > diff --git a/s390x/Makefile b/s390x/Makefile
> > index a8e04aa6fe4d..f8ea594b641d 100644
> > --- a/s390x/Makefile
> > +++ b/s390x/Makefile
> > @@ -32,6 +32,7 @@ tests += $(TEST_DIR)/epsw.elf
> >  tests += $(TEST_DIR)/adtl-status.elf
> >  tests += $(TEST_DIR)/migration.elf
> >  tests += $(TEST_DIR)/pv-attest.elf
> > +tests += $(TEST_DIR)/migration-skey.elf
> >  
> >  pv-tests += $(TEST_DIR)/pv-diags.elf
> >  
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > new file mode 100644
> > index 000000000000..6f3053d8ab40
> > --- /dev/null
> > +++ b/s390x/migration-skey.c
> > @@ -0,0 +1,98 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Storage Key migration tests
> > + *
> > + * Copyright IBM Corp. 2022
> > + *
> > + * Authors:
> > + *  Nico Boehr <nrb@linux.ibm.com>
> > + */
> > +
> > +#include <libcflat.h>
> > +#include <asm/facility.h>
> > +#include <asm/page.h>
> > +#include <asm/mem.h>
> > +#include <asm/interrupt.h>
> > +#include <hardware.h>
> > +
> > +#define NUM_PAGES 128
> > +static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> > +
> > +static void test_migration(void)
> > +{
> > +	int i, key_to_set;
> > +	uint8_t *page;
> > +	union skey expected_key, actual_key, mismatching_key;  
> 
> I would tend to scope those to the bodies of the respective loop,
> but I don't know if that's in accordance with the coding style.

I don't think this is specified explicitly; personally I have a light
preference for declaring everything upfront (like here), but again,
this is not a big deal for me (and maybe Janosch and Thomas should
also chime in and tell what their preference is)

> > +
> > +	for (i = 0; i < NUM_PAGES; i++) {
> > +		/*
> > +		 * Storage keys are 7 bit, lowest bit is always returned as zero
> > +		 * by iske
> > +		 */
> > +		key_to_set = i * 2;
> > +		set_storage_key(pagebuf + i, key_to_set, 1);  
> 
> Why not just pagebuf[i]?
> > +	}
> > +
> > +	puts("Please migrate me, then press return\n");
> > +	(void)getchar();
> > +
> > +	for (i = 0; i < NUM_PAGES; i++) {
> > +		report_prefix_pushf("page %d", i);
> > +
> > +		page = &pagebuf[i][0];
> > +		actual_key.val = get_storage_key(page);
> > +		expected_key.val = i * 2;
> > +
> > +		/* ignore reference bit */
> > +		actual_key.str.rf = 0;
> > +		expected_key.str.rf = 0;
> > +
> > +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
> > +
> > +		/* ensure access key doesn't match storage key and is never zero */
> > +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
> > +		*page = 0xff;
> > +
> > +		expect_pgm_int();
> > +		asm volatile (
> > +			/* set access key */
> > +			"spka 0(%[mismatching_key])\n"
> > +			/* try to write page */
> > +			"mvi 0(%[page]), 42\n"
> > +			/* reset access key */
> > +			"spka 0\n"
> > +			:
> > +			: [mismatching_key] "a"(mismatching_key.val),
> > +			  [page] "a"(page)
> > +			: "memory"
> > +		);
> > +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
> > +		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");  
> 
> What are you testing with this bit? If storage keys are really effective after the migration?
> I'm wondering if using tprot would not be better, it should simplify the code a lot.
> Plus you'd easily test for fetch protection, too.

on the other hand you could have tprot successful, but then not honour
the protection it indicates (I don't know how TPROT is implemented in
TCG)

to be fair, this test is only about checking that storage keys are
correctly migrated, maybe the check for actual protection is out of
scope

> > +
> > +		report_prefix_pop();
> > +	}
> > +}
> > +
> > +int main(void)
> > +{
> > +	report_prefix_push("migration-skey");
> > +	if (test_facility(169)) {
> > +		report_skip("storage key removal facility is active");
> > +
> > +		/*
> > +		 * If we just exit and don't ask migrate_cmd to migrate us, it
> > +		 * will just hang forever. Hence, also ask for migration when we
> > +		 * skip this test alltogether.  
> 
> s/alltogether/altogether/
> 
> > +		 */
> > +		puts("Please migrate me, then press return\n");
> > +		(void)getchar();
> > +
> > +		goto done;
> > +	}
> > +
> > +	test_migration();
> > +
> > +done:
> > +	report_prefix_pop();
> > +	return report_summary();
> > +}
> > diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> > index b456b2881448..1e851d8e3dd8 100644
> > --- a/s390x/unittests.cfg
> > +++ b/s390x/unittests.cfg
> > @@ -176,3 +176,7 @@ extra_params = -cpu qemu,gs=off,vx=off
> >  file = migration.elf
> >  groups = migration
> >  smp = 2
> > +
> > +[migration-skey]
> > +file = migration-skey.elf
> > +groups = migration  
> 

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Janis Schoetterl-Glausch]

On 5/13/22 14:33, Claudio Imbrenda wrote:
> On Fri, 13 May 2022 13:04:34 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> On 5/12/22 16:01, Nico Boehr wrote:
>>> Upon migration, we expect storage keys being set by the guest to be preserved,
>>> so add a test for it.
>>>
>>> We keep 128 pages and set predictable storage keys. Then, we migrate and check
>>> they can be read back and the respective access restrictions are in place when
>>> the access key in the PSW doesn't match.
>>>
>>> TCG currently doesn't implement key-controlled protection, see
>>> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
>>> tests as xfails.
>>>
>>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>>> ---
>>>  s390x/Makefile         |  1 +
>>>  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
>>>  s390x/unittests.cfg    |  4 ++
>>>  3 files changed, 103 insertions(+)
>>>  create mode 100644 s390x/migration-skey.c
>>>

[...]

>>> +	for (i = 0; i < NUM_PAGES; i++) {
>>> +		report_prefix_pushf("page %d", i);
>>> +
>>> +		page = &pagebuf[i][0];
>>> +		actual_key.val = get_storage_key(page);
>>> +		expected_key.val = i * 2;
>>> +
>>> +		/* ignore reference bit */
>>> +		actual_key.str.rf = 0;
>>> +		expected_key.str.rf = 0;
>>> +
>>> +		report(actual_key.val == expected_key.val, "expected_key=0x%x actual_key=0x%x", expected_key.val, actual_key.val);
>>> +
>>> +		/* ensure access key doesn't match storage key and is never zero */
>>> +		mismatching_key.str.acc = expected_key.str.acc < 15 ? expected_key.str.acc + 1 : 1;
>>> +		*page = 0xff;
>>> +
>>> +		expect_pgm_int();
>>> +		asm volatile (
>>> +			/* set access key */
>>> +			"spka 0(%[mismatching_key])\n"
>>> +			/* try to write page */
>>> +			"mvi 0(%[page]), 42\n"
>>> +			/* reset access key */
>>> +			"spka 0\n"
>>> +			:
>>> +			: [mismatching_key] "a"(mismatching_key.val),
>>> +			  [page] "a"(page)
>>> +			: "memory"
>>> +		);
>>> +		check_pgm_int_code_xfail(host_is_tcg(), PGM_INT_CODE_PROTECTION);
>>> +		report_xfail(host_is_tcg(), *page == 0xff, "no store occured");  
>>
>> What are you testing with this bit? If storage keys are really effective after the migration?
>> I'm wondering if using tprot would not be better, it should simplify the code a lot.
>> Plus you'd easily test for fetch protection, too.
> 
> on the other hand you could have tprot successful, but then not honour
> the protection it indicates (I don't know how TPROT is implemented in
> TCG)

Not at all with regards to skeys. But neither is checking the keys on access.
And for kvm, both TPROT and checking is handled by SIE.
> 
> to be fair, this test is only about checking that storage keys are
> correctly migrated, maybe the check for actual protection is out of
> scope
> 

Having more tests does no harm and might uncover things nobody thought of,
but I'd also be fine with keeping it short and sweet.
[...]

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Claudio Imbrenda]

On Fri, 13 May 2022 14:46:04 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On 5/13/22 14:33, Claudio Imbrenda wrote:
> > On Fri, 13 May 2022 13:04:34 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> >> On 5/12/22 16:01, Nico Boehr wrote:  
> >>> Upon migration, we expect storage keys being set by the guest to be preserved,
> >>> so add a test for it.
> >>>
> >>> We keep 128 pages and set predictable storage keys. Then, we migrate and check
> >>> they can be read back and the respective access restrictions are in place when
> >>> the access key in the PSW doesn't match.
> >>>
> >>> TCG currently doesn't implement key-controlled protection, see
> >>> target/s390x/mmu_helper.c, function mmu_handle_skey(), hence add the relevant
> >>> tests as xfails.
> >>>
> >>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> >>> ---
> >>>  s390x/Makefile         |  1 +
> >>>  s390x/migration-skey.c | 98 ++++++++++++++++++++++++++++++++++++++++++
> >>>  s390x/unittests.cfg    |  4 ++
> >>>  3 files changed, 103 insertions(+)
> >>>  create mode 100644 s390x/migration-skey.c
> >>>  

[...]

> Not at all with regards to skeys. But neither is checking the keys on access.
> And for kvm, both TPROT and checking is handled by SIE.

fair enough

> > 
> > to be fair, this test is only about checking that storage keys are
> > correctly migrated, maybe the check for actual protection is out of
> > scope
> >   
> 
> Having more tests does no harm and might uncover things nobody thought of,
> but I'd also be fine with keeping it short and sweet.
> [...]

I think this migration test should be kept more on focus about migration

we can always have a storage keys "torture test" separately

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Nico Boehr]

On Fri, 2022-05-13 at 15:04 +0200, Claudio Imbrenda wrote:
> I think this migration test should be kept more on focus about
> migration

Makes sense to me. In my next version, I will remove this check.
Thanks.

Re: [kvm-unit-tests PATCH v1 2/2] s390x: add migration test for storage keys

[Nico Boehr]

On Fri, 2022-05-13 at 13:04 +0200, Janis Schoetterl-Glausch wrote:
[...]
> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > new file mode 100644
> > index 000000000000..6f3053d8ab40
> > --- /dev/null
> > +++ b/s390x/migration-skey.c
[...]
> > +static void test_migration(void)
> > +{
> > +       int i, key_to_set;
> > +       uint8_t *page;
> > +       union skey expected_key, actual_key, mismatching_key;
> 
> I would tend to scope those to the bodies of the respective loop,
> but I don't know if that's in accordance with the coding style.

Seems to me the more common thing is to declare variables outside. But sure can change that, what do the maintainers say?

> > +
> > +       for (i = 0; i < NUM_PAGES; i++) {
> > +               /*
> > +                * Storage keys are 7 bit, lowest bit is always
> > returned as zero
> > +                * by iske
> > +                */
> > +               key_to_set = i * 2;
> > +               set_storage_key(pagebuf + i, key_to_set, 1);
> 
> Why not just pagebuf[i]?

Works as well and looks nicer, changed, thanks.

[...]
> > +       for (i = 0; i < NUM_PAGES; i++) {
[...]
> > +               expect_pgm_int();
> > +               asm volatile (
> > +                       /* set access key */
> > +                       "spka 0(%[mismatching_key])\n"
> > +                       /* try to write page */
> > +                       "mvi 0(%[page]), 42\n"
> > +                       /* reset access key */
> > +                       "spka 0\n"
> > +                       :
> > +                       : [mismatching_key]
> > "a"(mismatching_key.val),
> > +                         [page] "a"(page)
> > +                       : "memory"
> > +               );
> > +               check_pgm_int_code_xfail(host_is_tcg(),
> > PGM_INT_CODE_PROTECTION);
> > +               report_xfail(host_is_tcg(), *page == 0xff, "no
> > store occured");
> 
> What are you testing with this bit? If storage keys are really
> effective after the migration?

Yes.

> I'm wondering if using tprot would not be better, it should simplify
> the code a lot.

Hmm, good point. If I am not mistaken, tprot is intercepted, am I? Then it might make sense to actually do both, won't it?

> Plus you'd easily test for fetch protection, too.
> > +
> > +               report_prefix_pop();
> > +       }
> > +}
> > +
> > +int main(void)
> > +{
> > +       report_prefix_push("migration-skey");
> > +       if (test_facility(169)) {
> > +               report_skip("storage key removal facility is
> > active");
> > +
> > +               /*
> > +                * If we just exit and don't ask migrate_cmd to
> > migrate us, it
> > +                * will just hang forever. Hence, also ask for
> > migration when we
> > +                * skip this test alltogether.
> 
> s/alltogether/altogether/

Thanks fixed.