Re: [PULL 06/15] tests/qtest/migration: Don't use -cpu max for aarch64

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jan 26, 2024 at 11:54:32AM -0300, Fabiano Rosas wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>> > On Fri, 26 Jan 2024 at 14:36, Fabiano Rosas <farosas@suse.de> wrote:
>> >>
>> >> peterx@redhat.com writes:
>> >>
>> >> > From: Fabiano Rosas <farosas@suse.de>
>> >> >
>> >> > The 'max' cpu is not expected to be stable in terms of features across
>> >> > QEMU versions, so it should not be expected to migrate.
>> >> >
>> >> > While the tests currently all pass with -cpu max, that is only because
>> >> > we're not testing across QEMU versions, which is the more common
>> >> > use-case for migration.
>> >> >
>> >> > We've recently introduced compatibility tests that use two different
>> >> > QEMU versions and the tests are now failing for aarch64. The next
>> >> > patch adds those tests to CI, so we cannot use the 'max' cpu
>> >> > anymore. Replace it with the 'neoverse-n1', which has a fixed set of
>> >> > features.
>> >> >
>> >> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> > Link: 20240118164951.30350-2-farosas@suse.de">https://lore.kernel.org/r/20240118164951.30350-2-farosas@suse.de
>> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> >> > ---
>> >> >  tests/qtest/migration-test.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> >> > index 7675519cfa..15713f3666 100644
>> >> > --- a/tests/qtest/migration-test.c
>> >> > +++ b/tests/qtest/migration-test.c
>> >> > @@ -820,7 +820,7 @@ static int test_migrate_start(QTestState **from, 
>> >> > QTestState **to,
>> >> >          memory_size = "150M";
>> >> >          machine_alias = "virt";
>> >> >          machine_opts = "gic-version=max";
>> >> > -        arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
>> >> > +        arch_opts = g_strdup_printf("-cpu neoverse-n1 -kernel %s", 
>> >> > bootpath);
>> >> >          start_address = ARM_TEST_MEM_START;
>> >> >          end_address = ARM_TEST_MEM_END;
>> >> >      } else {
>> >>
>> >> This breaks the tests on an arm host with KVM support. We could drop
>> >> this patch from the PR, it doesn't affect anything else.
>> >>
>> >> Or squash in:
>> >>
>> >> -->8--
>> >> From b8aa5d8a2b33dcc28e4cd4ce2c4f4eacc3a3b845 Mon Sep 17 00:00:00 2001
>> >> From: Fabiano Rosas <farosas@suse.de>
>> >> Date: Fri, 26 Jan 2024 11:33:15 -0300
>> >> Subject: [PATCH] fixup! tests/qtest/migration: Don't use -cpu max for 
>> >> aarch64
>> >>
>> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> ---
>> >>  tests/qtest/migration-test.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> >> index 15713f3666..2ba9cab684 100644
>> >> --- a/tests/qtest/migration-test.c
>> >> +++ b/tests/qtest/migration-test.c
>> >> @@ -820,7 +820,9 @@ static int test_migrate_start(QTestState **from, 
>> >> QTestState **to,
>> >>          memory_size = "150M";
>> >>          machine_alias = "virt";
>> >>          machine_opts = "gic-version=max";
>> >> -        arch_opts = g_strdup_printf("-cpu neoverse-n1 -kernel %s", 
>> >> bootpath);
>> >> +        arch_opts = g_strdup_printf("-cpu %s -kernel %s",
>> >> +                                    qtest_has_accel("kvm") ?
>> >> +                                    "host" : "neoverse-n1", bootpath);
>> >>          start_address = ARM_TEST_MEM_START;
>> >>          end_address = ARM_TEST_MEM_END;
>> >>      } else {
>> >
>> > If you want to do that then a comment explaining why would be
>> > helpful for future readers, I think.
>> 
>> Ok, let's drop this one then, I'll resend.
>
> I'll drop this one for now then, thanks.
>
> Just to double check: Fabiano, you meant that "-cpu host" won't hit the
> same issue as what "-cpu max" would have for the new "n-1" CI test, right?

Well, no. What we need here is a cpu that works with KVM. Currently
that's 'host'. If that breaks the n-1 test, then it's a regression.

We also need a cpu that works with TCG. Any of them would do. Except max
which changes in incompatible ways (that was the original patch's
purpose).

The issue that occurs to me now is that 'cpu host' will not work with
TCG. We might actually need to go poking /dev/kvm for this to work.