On Fri, 9 Dec 2022 11:21:22 +0100
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> Right now, we have a test which sets storage keys, then migrates the VM
> and - after migration finished - verifies the skeys are still there.
>
> Add a new version of the test which changes storage keys while the
> migration is in progress. This is achieved by adding a command line
> argument to the existing migration-skey test.
>
> Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
looks good, except for a few small details below
> ---
> s390x/migration-skey.c | 214 +++++++++++++++++++++++++++++++++++------
> s390x/unittests.cfg | 15 ++-
> 2 files changed, 198 insertions(+), 31 deletions(-)
>
> diff --git a/s390x/migration-skey.c b/s390x/migration-skey.c
> index b7bd82581abe..9b9a45f4ad3b 100644
> --- a/s390x/migration-skey.c
> +++ b/s390x/migration-skey.c
> @@ -2,6 +2,12 @@
> /*
> * Storage Key migration tests
> *
> + * There are two variants of this test:
> + * - sequential: sets some storage keys on pages, migrates the VM and then
> + * verifies the storage keys are still as we expect.
> + * - parallel: start migration of a VM and set and check storage keys on some
> + * pages while migration is in process.
> + *
> * Copyright IBM Corp. 2022
> *
> * Authors:
> @@ -10,20 +16,44 @@
>
> #include <libcflat.h>
> #include <asm/facility.h>
> -#include <asm/page.h>
> #include <asm/mem.h>
> -#include <asm/interrupt.h>
> +#include <asm/page.h>
> +#include <asm/barrier.h>
> #include <hardware.h>
> +#include <smp.h>
> +
> +struct skey_verify_result {
> + bool verify_failed;
> + union skey expected_key;
> + union skey actual_key;
> + unsigned long page_mismatch_idx;
> + unsigned long page_mismatch_addr;
> +};
>
> #define NUM_PAGES 128
> -static uint8_t pagebuf[NUM_PAGES][PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +static uint8_t pagebuf[NUM_PAGES * PAGE_SIZE] __attribute__((aligned(PAGE_SIZE)));
> +
> +static struct skey_verify_result result;
>
> -static void test_migration(void)
> +static unsigned int thread_iters;
> +static bool thread_should_exit;
> +static bool thread_exited;
> +
> +/*
> + * Set storage key test pattern on pagebuf with a seed for the storage keys.
> + *
> + * Each page's storage key is generated by taking the page's index in pagebuf,
> + * XOR-ing that with the given seed and then multipling the result with two.
> + *
> + * pagebuf must point to page_count consecutive pages.
> + * Only the lower seven bits of the seed are considered.
> + */
> +static void skey_set_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
> {
> - union skey expected_key, actual_key;
> - int i, key_to_set, key_mismatches = 0;
> + unsigned char key_to_set;
> + unsigned long i;
>
> - for (i = 0; i < NUM_PAGES; i++) {
> + for (i = 0; i < page_count; i++) {
> /*
> * Storage keys are 7 bit, lowest bit is always returned as zero
> * by iske.
> @@ -31,16 +61,34 @@ static void test_migration(void)
> * protection as well as reference and change indication for
> * some keys.
> */
> - key_to_set = i * 2;
> - set_storage_key(pagebuf[i], key_to_set, 1);
> + key_to_set = (i ^ seed) * 2;
> + set_storage_key(pagebuf + i * PAGE_SIZE, key_to_set, 1);
> }
> +}
>
> - puts("Please migrate me, then press return\n");
> - (void)getchar();
> +/*
> + * Verify storage keys on pagebuf.
> + * Storage keys must have been set by skey_set_test_pattern on pagebuf before.
> + * skey_set_keys must have been called with the same seed value.
> + *
> + * If storage keys match the expected result, will return a skey_verify_result
> + * with verify_failed false. All other fields are then invalid.
> + * If there is a mismatch, returned struct will have verify_failed true and will
> + * be filled with the details on the first mismatch encountered.
> + */
> +static struct skey_verify_result skey_verify_test_pattern(uint8_t *pagebuf, unsigned long page_count, unsigned char seed)
this line is a little too long, even for the KVM unit tests; find a way
to shorten it by a couple of bytes (it would look better to keep it on
one line)
(rename pagebuf? use size_t or uint64_t instead of unsigned long? use
uint8_t for seed? shorten the name of struct verify_result?)
> +{
> + union skey expected_key, actual_key;
> + struct skey_verify_result result = {
> + .verify_failed = true
> + };
> + uint8_t *cur_page;
> + unsigned long i;
>
> - for (i = 0; i < NUM_PAGES; i++) {
> - actual_key.val = get_storage_key(pagebuf[i]);
> - expected_key.val = i * 2;
> + for (i = 0; i < page_count; i++) {
> + cur_page = pagebuf + i * PAGE_SIZE;
> + actual_key.val = get_storage_key(cur_page);
> + expected_key.val = (i ^ seed) * 2;
>
> /*
> * The PoP neither gives a guarantee that the reference bit is
> @@ -51,33 +99,145 @@ static void test_migration(void)
> actual_key.str.rf = 0;
> expected_key.str.rf = 0;
>
> - /* don't log anything when key matches to avoid spamming the log */
> if (actual_key.val != expected_key.val) {
> - key_mismatches++;
> - report_fail("page %d expected_key=0x%x actual_key=0x%x", i, expected_key.val, actual_key.val);
> + result.expected_key.val = expected_key.val;
> + result.actual_key.val = actual_key.val;
> + result.page_mismatch_idx = i;
> + result.page_mismatch_addr = (unsigned long)cur_page;
> + return result;
> }
> }
>
> - report(!key_mismatches, "skeys after migration match");
> + result.verify_failed = false;
> + return result;
> +}
> +
> +static void skey_report_verify(struct skey_verify_result * const result)
> +{
> + if (result->verify_failed)
> + report_fail("page skey mismatch: first page idx = %lu, addr = 0x%lx, "
> + "expected_key = 0x%x, actual_key = 0x%x",
> + result->page_mismatch_idx, result->page_mismatch_addr,
> + result->expected_key.val, result->actual_key.val);
> + else
> + report_pass("skeys match");
> +}
> +
> +static void migrate_once(void)
> +{
> + static bool migrated;
> +
> + if (migrated)
> + return;
> +
> + migrated = true;
> + puts("Please migrate me, then press return\n");
> + (void)getchar();
> }
you don't need this any more, since your migration patches are
upstream now :)
>
> -int main(void)
> +static void test_skey_migration_sequential(void)
> +{
> + report_prefix_push("sequential");
> +
> + skey_set_test_pattern(pagebuf, NUM_PAGES, 0);
> +
> + migrate_once();
> +
> + result = skey_verify_test_pattern(pagebuf, NUM_PAGES, 0);
> + skey_report_verify(&result);
> +
> + report_prefix_pop();
> +}
> +
> +static void set_skeys_thread(void)
> +{
> + while (!READ_ONCE(thread_should_exit)) {
> + skey_set_test_pattern(pagebuf, NUM_PAGES, thread_iters);
> +
> + result = skey_verify_test_pattern(pagebuf, NUM_PAGES, thread_iters);
> +
> + /*
> + * Always increment even if the verify fails. This ensures primary CPU knows where
> + * we left off and can do an additional verify round after migration finished.
> + */
> + thread_iters++;
> +
> + if (result.verify_failed)
> + break;
> + }
> +
> + WRITE_ONCE(thread_exited, 1);
> +}
> +
> +static void test_skey_migration_parallel(void)
> +{
> + report_prefix_push("parallel");
> +
> + if (smp_query_num_cpus() == 1) {
> + report_skip("need at least 2 cpus for this test");
> + goto error;
> + }
> +
> + smp_cpu_setup(1, PSW_WITH_CUR_MASK(set_skeys_thread));
> +
> + migrate_once();
> +
> + WRITE_ONCE(thread_should_exit, 1);
> +
> + while (!thread_exited)
> + mb();
this was discussed in the other subthread :)
> +
> + report_info("thread completed %u iterations", thread_iters);
> +
> + report_prefix_push("during migration");
> + skey_report_verify(&result);
> + report_prefix_pop();
> +
> + /*
> + * Verification of skeys occurs on the thread. We don't know if we
> + * were still migrating during the verification.
> + * To be sure, make another verification round after the migration
> + * finished to catch skeys which might not have been migrated
> + * correctly.
> + */
> + report_prefix_push("after migration");
> + assert(thread_iters > 0);
> + result = skey_verify_test_pattern(pagebuf, NUM_PAGES, thread_iters - 1);
> + skey_report_verify(&result);
> + report_prefix_pop();
> +
> +error:
> + report_prefix_pop();
> +}
> +
> +static void print_usage(void)
> +{
> + report_info("Usage: migration-skey [parallel|sequential]");
> +}
> +
> +int main(int argc, char **argv)
> {
> report_prefix_push("migration-skey");
> if (test_facility(169)) {
> report_skip("storage key removal facility is active");
> + goto error;
> + }
>
> - /*
> - * 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();
> + if (argc < 2) {
> + print_usage();
> + goto error;
> + }
> +
> + if (!strcmp("parallel", argv[1])) {
> + test_skey_migration_parallel();
> + } else if (!strcmp("sequential", argv[1])) {
> + test_skey_migration_sequential();
> } else {
> - test_migration();
> + print_usage();
hmm I don't like this whole main.
I think the best approach would be to have some kind of separate
parse_args function that will set some flags (or one flag, in this case)
then the main becomes just a bunch of if/else, something like this:
parse_args(argc, argv);
if (test_facility(169))
skip
else if (parallel_flag)
parallel();
else
sequential();
also, default to sequential if there are no parameters, and use
posix-style parameters (--parallel, --sequential)
only if you get an invalid parameter you can print the usage and fail
optionally if you really want you can print the usage info when called
without parameter and inform that the test will default to sequential
one of these days I have to write an argument parsing library :)
> }
>
> +error:
> + migrate_once();
> report_prefix_pop();
> return report_summary();
> }
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 3caf81eda396..7763cb857954 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -185,10 +185,6 @@ smp = 2
> file = migration-cmm.elf
> groups = migration
>
> -[migration-skey]
> -file = migration-skey.elf
> -groups = migration
> -
> [panic-loop-extint]
> file = panic-loop-extint.elf
> groups = panic
> @@ -208,3 +204,14 @@ groups = migration
> [exittime]
> file = exittime.elf
> smp = 2
> +
> +[migration-skey-sequential]
> +file = migration-skey.elf
> +groups = migration
> +extra_params = -append 'sequential'
> +
> +[migration-skey-parallel]
> +file = migration-skey.elf
> +smp = 2
> +groups = migration
> +extra_params = -append 'parallel'
