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

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

[Nico Boehr]

v1->v2:
----
* As per discussion with Janis and Claudio, remove the actual access check from
  the test. This also allows us to remove the check_pgm_int_code_xfail() patch.
* Typos/Style suggestions (thanks Janis)

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.

Nico Boehr (1):
  s390x: add migration test for storage keys

 s390x/Makefile         |  1 +
 s390x/migration-skey.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg    |  4 +++
 3 files changed, 83 insertions(+)
 create mode 100644 s390x/migration-skey.c

-- 
2.31.1

[kvm-unit-tests PATCH v2 1/1] 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.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile         |  1 +
 s390x/migration-skey.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 s390x/unittests.cfg    |  4 +++
 3 files changed, 83 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..ee4622eb94ba
--- /dev/null
+++ b/s390x/migration-skey.c
@@ -0,0 +1,78 @@
+/* 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;
+
+	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);
+
+		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 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
-- 
2.31.1

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

[Claudio Imbrenda]

On Mon, 16 May 2022 11:07:02 +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.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile         |  1 +
>  s390x/migration-skey.c | 78 ++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg    |  4 +++
>  3 files changed, 83 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..ee4622eb94ba
> --- /dev/null
> +++ b/s390x/migration-skey.c
> @@ -0,0 +1,78 @@
> +/* 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;

please reverse Christmas tree ^

with that fixed:

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

> +
> +	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);
> +
> +		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 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 v2 1/1] s390x: add migration test for storage keys

[Janis Schoetterl-Glausch]

On 5/16/22 11:07, Nico Boehr wrote:
> Upon migration, we expect storage keys being set by the guest to be preserved,
> so add a test for it.

"being set" implies that keys are set while the migration is going on.
That's not the case, is 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

... check that they ...

> the access key in the PSW doesn't match.

The latter half of the sentence doesn't apply anymore, now that you simplified the test.
So maybe something like: ... and check that they can be read back and match the value
originally set.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/Makefile         |  1 +
>  s390x/migration-skey.c | 78 ++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg    |  4 +++
>  3 files changed, 83 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..ee4622eb94ba
> --- /dev/null
> +++ b/s390x/migration-skey.c
> @@ -0,0 +1,78 @@
> +/* 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;
> +
> +	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);

The page variable is kinda useless now, I'd just do get_storage_key(pagebuf[0]).
> +		expected_key.val = i * 2;
> +
> +		/* ignore reference bit */

Why? Are there any implicit references I'm missing?
> +		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);
> +
> +		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 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 v2 1/1] s390x: add migration test for storage keys

[Nico Boehr]

On Mon, 2022-05-16 at 18:47 +0200, Janis Schoetterl-Glausch wrote:
> On 5/16/22 11:07, Nico Boehr wrote:
> > Upon migration, we expect storage keys being set by the guest to be
> > preserved,
> > so add a test for it.
> 
> "being set" implies that keys are set while the migration is going
> on.
> That's not the case, is it?

Fixed.

> > 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
> 
> ... check that they ...

Added that.

> 
> > the access key in the PSW doesn't match.
> 
> The latter half of the sentence doesn't apply anymore, now that you
> simplified the test.
> So maybe something like: ... and check that they can be read back and
> match the value
> originally set.

Fixed.

> > diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> > new file mode 100644
> > index 000000000000..ee4622eb94ba
> > --- /dev/null
> > +++ b/s390x/migration-skey.c
> > @@ -0,0 +1,78 @@
> > +/* 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;
> > +
> > +       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);
> 
> The page variable is kinda useless now, I'd just do
> get_storage_key(pagebuf[0]).

Removed.

> > +               expected_key.val = i * 2;
> > +
> > +               /* ignore reference bit */
> 
> Why? Are there any implicit references I'm missing?

Since the PoP specifies (p. 5-122):

"The record of references provided by the reference
bit is not necessarily accurate. However, in the major-
ity of situations, reference recording approximately
coincides with the related storage reference."

I don't really see a way to test this properly.

Maybe I missed something?

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

[Janis Schoetterl-Glausch]

On Tue, 2022-05-17 at 10:17 +0200, Nico Boehr wrote:
> On Mon, 2022-05-16 at 18:47 +0200, Janis Schoetterl-Glausch wrote:
> > On 5/16/22 11:07, Nico Boehr wrote:

[...]

> > > +               expected_key.val = i * 2;
> > > +
> > > +               /* ignore reference bit */
> > 
> > Why? Are there any implicit references I'm missing?
> 
> Since the PoP specifies (p. 5-122):
> 
> "The record of references provided by the reference
> bit is not necessarily accurate. However, in the major-
> ity of situations, reference recording approximately
> coincides with the related storage reference."
> 
> I don't really see a way to test this properly.
> 
> Maybe I missed something?

No I think you're right, although in practice the reference bits should
match. Or did you observe a mismatch?

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

[Nico Boehr]

On Tue, 2022-05-17 at 13:44 +0200, Janis Schoetterl-Glausch wrote:
> No I think you're right, although in practice the reference bits
> should
> match. Or did you observe a mismatch?

Yes, I can sometimes observe mismatches.

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

[Claudio Imbrenda]

On Tue, 17 May 2022 13:44:51 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> On Tue, 2022-05-17 at 10:17 +0200, Nico Boehr wrote:
> > On Mon, 2022-05-16 at 18:47 +0200, Janis Schoetterl-Glausch wrote:  
> > > On 5/16/22 11:07, Nico Boehr wrote:  
> 
> [...]
> 
> > > > +               expected_key.val = i * 2;
> > > > +
> > > > +               /* ignore reference bit */  
> > > 
> > > Why? Are there any implicit references I'm missing?  
> > 
> > Since the PoP specifies (p. 5-122):
> > 
> > "The record of references provided by the reference
> > bit is not necessarily accurate. However, in the major-
> > ity of situations, reference recording approximately
> > coincides with the related storage reference."
> > 
> > I don't really see a way to test this properly.
> > 
> > Maybe I missed something?  
> 
> No I think you're right, although in practice the reference bits should
> match. Or did you observe a mismatch?

the point is that the architecture allows for mismatches (in particular
I think it is allowed to overindicate changes)

ignoring that bit is the correct thing to do