Hi,
On Tue, Feb 20, 2024 at 06:51:51PM +1000, Nicholas Piggin wrote:
> On Tue Feb 20, 2024 at 2:56 AM AEST, Alexandru Elisei wrote:
> > Hi,
> >
> > Thanks for writing this. I've tested it with kvmtool, which emulates a 8250
> > UART:
> >
> > Tested-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> >
> > This fixes a longstanding bug with kvmtool, where migrate_once() would read
> > the last character that was sent, and then think that migration was
> > completed even though it was never performed.
> >
> > While we are on the subject of migration:
> >
> > SKIP: gicv3: its-migration: Test requires at least 4 vcpus
> > Now migrate the VM, then press a key to continue...
> > INFO: gicv3: its-migration: Migration complete
> > SUMMARY: 1 tests, 1 skipped
> >
> > That's extremely confusing. Why is migrate_once() executed after the
> > test_its_pending() function call without checking if the test was skipped?
>
> Seems not too hard, incremental patch on top of multi migration
> series below. After this series is merged I can try that (s390
> could benefit with some changes too).
Thank you for having a look at this so quickly. The changes to the gic test
look good to me.
As an alternative, have you considered modifying the test harness to parse
the SKIP message, and if the test is in the migration group to not mark it
as a migration failure? That would require that the test name printed by a
test matches the test name from unittests.cfg (should probably be the case
already), but any new migration tests will just work, without having to put
migrate_skip() at each failure point.
Thanks,
Alex
>
> Thanks,
> Nick
>
> ---
> diff --git a/arm/gic.c b/arm/gic.c
> index c950b0d15..bbf828f17 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -782,13 +782,15 @@ static void test_its_migration(void)
> struct its_device *dev2, *dev7;
> cpumask_t mask;
>
> - if (its_setup1())
> + if (its_setup1()) {
> + migrate_skip();
> return;
> + }
>
> dev2 = its_get_device(2);
> dev7 = its_get_device(7);
>
> - migrate_once();
> + migrate();
>
> stats_reset();
> cpumask_clear(&mask);
> @@ -819,8 +821,10 @@ static void test_migrate_unmapped_collection(void)
> int pe0 = 0;
> u8 config;
>
> - if (its_setup1())
> + if (its_setup1()) {
> + migrate_skip();
> return;
> + }
>
> if (!errata(ERRATA_UNMAPPED_COLLECTIONS)) {
> report_skip("Skipping test, as this test hangs without the fix. "
> @@ -836,7 +840,7 @@ static void test_migrate_unmapped_collection(void)
> its_send_mapti(dev2, 8192, 0, col);
> gicv3_lpi_set_config(8192, LPI_PROP_DEFAULT);
>
> - migrate_once();
> + migrate();
>
> /* on the destination, map the collection */
> its_send_mapc(col, true);
> @@ -875,8 +879,10 @@ static void test_its_pending_migration(void)
> void *ptr;
> int i;
>
> - if (its_prerequisites(4))
> + if (its_prerequisites(4)) {
> + migrate_skip();
> return;
> + }
>
> dev = its_create_device(2 /* dev id */, 8 /* nb_ites */);
> its_send_mapd(dev, true);
> @@ -923,7 +929,7 @@ static void test_its_pending_migration(void)
> gicv3_lpi_rdist_enable(pe0);
> gicv3_lpi_rdist_enable(pe1);
>
> - migrate_once();
> + migrate();
>
> /* let's wait for the 256 LPIs to be handled */
> mdelay(1000);
> @@ -970,17 +976,14 @@ int main(int argc, char **argv)
> } else if (!strcmp(argv[1], "its-migration")) {
> report_prefix_push(argv[1]);
> test_its_migration();
> - migrate_once();
> report_prefix_pop();
> } else if (!strcmp(argv[1], "its-pending-migration")) {
> report_prefix_push(argv[1]);
> test_its_pending_migration();
> - migrate_once();
> report_prefix_pop();
> } else if (!strcmp(argv[1], "its-migrate-unmapped-collection")) {
> report_prefix_push(argv[1]);
> test_migrate_unmapped_collection();
> - migrate_once();
> report_prefix_pop();
> } else if (strcmp(argv[1], "its-introspection") == 0) {
> report_prefix_push(argv[1]);
> diff --git a/lib/migrate.c b/lib/migrate.c
> index 92d1d957d..dde43a90e 100644
> --- a/lib/migrate.c
> +++ b/lib/migrate.c
> @@ -43,3 +43,13 @@ void migrate_once(void)
> migrated = true;
> migrate();
> }
> +
> +/*
> + * When the test has been started in migration mode, but the test case is
> + * skipped and no migration point is reached, this can be used to tell the
> + * harness not to mark it as a failure to migrate.
> + */
> +void migrate_skip(void)
> +{
> + puts("Skipped VM migration (quiet)\n");
> +}
> diff --git a/lib/migrate.h b/lib/migrate.h
> index 95b9102b0..db6e0c501 100644
> --- a/lib/migrate.h
> +++ b/lib/migrate.h
> @@ -9,3 +9,5 @@
> void migrate(void);
> void migrate_quiet(void);
> void migrate_once(void);
> +
> +void migrate_skip(void);
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> index 2214d940c..3257d5218 100644
> --- a/scripts/arch-run.bash
> +++ b/scripts/arch-run.bash
> @@ -152,7 +152,9 @@ run_migration ()
> -chardev socket,id=mon,path=${src_qmp},server=on,wait=off \
> -mon chardev=mon,mode=control > ${src_outfifo} &
> live_pid=$!
> - cat ${src_outfifo} | tee ${src_out} | grep -v "Now migrate the VM (quiet)" &
> + cat ${src_outfifo} | tee ${src_out} | \
> + grep -v "Now migrate the VM (quiet)" | \
> + grep -v "Skipped VM migration (quiet)" &
>
> # Start the first destination QEMU machine in advance of the test
> # reaching the migration point, since we expect at least one migration.
> @@ -190,16 +192,22 @@ do_migration ()
> -mon chardev=mon,mode=control -incoming unix:${dst_incoming} \
> < <(cat ${dst_infifo}) > ${dst_outfifo} &
> incoming_pid=$!
> - cat ${dst_outfifo} | tee ${dst_out} | grep -v "Now migrate the VM (quiet)" &
> + cat ${dst_outfifo} | tee ${dst_out} | \
> + grep -v "Now migrate the VM (quiet)" | \
> + grep -v "Skipped VM migration (quiet)" &
>
> # The test must prompt the user to migrate, so wait for the
> # "Now migrate VM" console message.
> while ! grep -q -i "Now migrate the VM" < ${src_out} ; do
> if ! ps -p ${live_pid} > /dev/null ; then
> - echo "ERROR: Test exit before migration point." >&2
> echo > ${dst_infifo}
> - qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
> qmp ${dst_qmp} '"quit"'> ${dst_qmpout} 2>/dev/null
> + if grep -q -i "Skipped VM migration" < ${src_out} ; then
> + wait ${live_pid}
> + return $?
> + fi
> + echo "ERROR: Test exit before migration point." >&2
> + qmp ${src_qmp} '"quit"'> ${src_qmpout} 2>/dev/null
> return 3
> fi
> sleep 0.1
