| Message ID | 20220516090702.1939253-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: add migration test for storage keys | expand |
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
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
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?
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?
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.
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
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
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