* [kvm-unit-tests PATCH v2 0/1] s390x: add migration test for storage keys @ 2022-05-16 9:07 Nico Boehr 2022-05-16 9:07 ` [kvm-unit-tests PATCH v2 1/1] " Nico Boehr 0 siblings, 1 reply; 8+ messages in thread From: Nico Boehr @ 2022-05-16 9:07 UTC (permalink / raw) To: kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-16 9:07 [kvm-unit-tests PATCH v2 0/1] s390x: add migration test for storage keys Nico Boehr @ 2022-05-16 9:07 ` Nico Boehr 2022-05-16 15:59 ` Claudio Imbrenda 2022-05-16 16:47 ` Janis Schoetterl-Glausch 0 siblings, 2 replies; 8+ messages in thread From: Nico Boehr @ 2022-05-16 9:07 UTC (permalink / raw) To: kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-16 9:07 ` [kvm-unit-tests PATCH v2 1/1] " Nico Boehr @ 2022-05-16 15:59 ` Claudio Imbrenda 2022-05-16 16:47 ` Janis Schoetterl-Glausch 1 sibling, 0 replies; 8+ messages in thread From: Claudio Imbrenda @ 2022-05-16 15:59 UTC (permalink / raw) To: Nico Boehr; +Cc: kvm, linux-s390, frankja, thuth 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-16 9:07 ` [kvm-unit-tests PATCH v2 1/1] " Nico Boehr 2022-05-16 15:59 ` Claudio Imbrenda @ 2022-05-16 16:47 ` Janis Schoetterl-Glausch 2022-05-17 8:17 ` Nico Boehr 1 sibling, 1 reply; 8+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-16 16:47 UTC (permalink / raw) To: Nico Boehr, kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-16 16:47 ` Janis Schoetterl-Glausch @ 2022-05-17 8:17 ` Nico Boehr 2022-05-17 11:44 ` Janis Schoetterl-Glausch 0 siblings, 1 reply; 8+ messages in thread From: Nico Boehr @ 2022-05-17 8:17 UTC (permalink / raw) To: Janis Schoetterl-Glausch, kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-17 8:17 ` Nico Boehr @ 2022-05-17 11:44 ` Janis Schoetterl-Glausch 2022-05-17 11:53 ` Nico Boehr 2022-05-17 12:29 ` Claudio Imbrenda 0 siblings, 2 replies; 8+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-17 11:44 UTC (permalink / raw) To: Nico Boehr, kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-17 11:44 ` Janis Schoetterl-Glausch @ 2022-05-17 11:53 ` Nico Boehr 2022-05-17 12:29 ` Claudio Imbrenda 1 sibling, 0 replies; 8+ messages in thread From: Nico Boehr @ 2022-05-17 11:53 UTC (permalink / raw) To: Janis Schoetterl-Glausch, kvm, linux-s390; +Cc: frankja, imbrenda, thuth 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/1] s390x: add migration test for storage keys 2022-05-17 11:44 ` Janis Schoetterl-Glausch 2022-05-17 11:53 ` Nico Boehr @ 2022-05-17 12:29 ` Claudio Imbrenda 1 sibling, 0 replies; 8+ messages in thread From: Claudio Imbrenda @ 2022-05-17 12:29 UTC (permalink / raw) To: Janis Schoetterl-Glausch; +Cc: Nico Boehr, kvm, linux-s390, frankja, thuth 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-17 12:29 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-05-16 9:07 [kvm-unit-tests PATCH v2 0/1] s390x: add migration test for storage keys Nico Boehr 2022-05-16 9:07 ` [kvm-unit-tests PATCH v2 1/1] " Nico Boehr 2022-05-16 15:59 ` Claudio Imbrenda 2022-05-16 16:47 ` Janis Schoetterl-Glausch 2022-05-17 8:17 ` Nico Boehr 2022-05-17 11:44 ` Janis Schoetterl-Glausch 2022-05-17 11:53 ` Nico Boehr 2022-05-17 12:29 ` Claudio Imbrenda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.