[PATCH v2 0/3] ci: Fixes on the recent cross-binary test case

[PATCH v2 0/3] ci: Fixes on the recent cross-binary test case

[peterx]

From: Peter Xu <peterx@redhat.com>

v2:
- Fix a typo in patch 2 on QEMU_PREV_VERSION
- Added R-bs for Dan

Hi,

This small patchset updates the recent cross-binary test for migration on
a few things.

Patch 1 modifies the aarch64 test GIC version to 3 rather than "max",
paving way for enabling it, even if the CPU model is not yet ready.

Patch 2 removes the tag dependency of the new build-previous-qemu job, so
that in personal CI pipelines the job won't fail if the tag is missing, as
reported by Peter Maydell, and solution suggested by Dan.

Patch 3 updates the comment for aarch64 on the test to state the fact, and
what is missing.  Then we don't target it support for v9.0, but only until
we have a stable CPU model for aarch64 (if ever possible to support both
tcg and kvm).

Comments welcomed, thanks.

Peter Xu (3):
  tests/migration-test: Stick with gicv3 in aarch64 test
  ci: Remove tag dependency for build-previous-qemu
  ci: Update comment for migration-compat-aarch64

 tests/qtest/migration-test.c | 2 +-
 .gitlab-ci.d/buildtest.yml   | 9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] tests/migration-test: Stick with gicv3 in aarch64 test

[peterx]

From: Peter Xu <peterx@redhat.com>

Recently we introduced cross-binary migration test.  It's always wanted
that migration-test uses stable guest ABI for both QEMU binaries in this
case, so that both QEMU binaries will be compatible on the migration
stream with the cmdline specified.

Switch to a static gic version "3" rather than using version "max", so that
GIC should be stable now across any future QEMU binaries for migration-test.

Here the version can actually be anything as long as the ABI is stable.  We
choose "3" because it's the majority of what we already use in QEMU while
still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
no direct user yet besides "max".

Note that even with this change, aarch64 won't be able to work yet with
migration cross binary test, but then the only missing piece will be the
stable CPU model.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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..8a5bb1752e 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -819,7 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     } else if (strcmp(arch, "aarch64") == 0) {
         memory_size = "150M";
         machine_alias = "virt";
-        machine_opts = "gic-version=max";
+        machine_opts = "gic-version=3";
         arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
         start_address = ARM_TEST_MEM_START;
         end_address = ARM_TEST_MEM_END;
-- 
2.43.0

[PATCH v2 2/3] ci: Remove tag dependency for build-previous-qemu

[peterx]

From: Peter Xu <peterx@redhat.com>

The new build-previous-qemu job relies on QEMU release tag being present,
while that may not be always true for personal git repositories since by
default tag is not pushed.  The job can fail on those CI kicks, as reported
by Peter Maydell.

Fix it by fetching the tags remotely from the official repository, as
suggested by Dan.

[1] https://lore.kernel.org/r/ZcC9ScKJ7VvqektA@redhat.com

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 .gitlab-ci.d/buildtest.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 79bbc8585b..cfe95c1b17 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -189,6 +189,8 @@ build-previous-qemu:
     TARGETS: x86_64-softmmu aarch64-softmmu
   before_script:
     - export QEMU_PREV_VERSION="$(sed 's/\([0-9.]*\)\.[0-9]*/v\1.0/' VERSION)"
+    - git remote add upstream https://gitlab.com/qemu-project/qemu
+    - git fetch upstream $QEMU_PREV_VERSION
     - git checkout $QEMU_PREV_VERSION
   after_script:
     - mv build build-previous
-- 
2.43.0

[PATCH v2 3/3] ci: Update comment for migration-compat-aarch64

[peterx]

From: Peter Xu <peterx@redhat.com>

It turns out that we may not be able to enable this test even for the
upcoming v9.0.  Document what we're still missing.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 .gitlab-ci.d/buildtest.yml | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfe95c1b17..f56df59c94 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -219,9 +219,10 @@ build-previous-qemu:
     - QTEST_QEMU_BINARY_DST=./qemu-system-${TARGET}
           QTEST_QEMU_BINARY=../build/qemu-system-${TARGET} ./tests/qtest/migration-test
 
-# This job is disabled until we release 9.0. The existing
-# migration-test in 8.2 is broken on aarch64. The fix was already
-# commited, but it will only take effect once 9.0 is out.
+# This job needs to be disabled until we can have an aarch64 CPU model that
+# will both (1) support both KVM and TCG, and (2) provide a stable ABI.
+# Currently only "-cpu max" can provide (1), however it doesn't guarantee
+# (2).  Mark this test skipped until later.
 migration-compat-aarch64:
   extends: .migration-compat-common
   variables:
-- 
2.43.0

Re: [PATCH v2 0/3] ci: Fixes on the recent cross-binary test case

[Peter Xu]

On Wed, Feb 07, 2024 at 08:54:00AM +0800, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> v2:
> - Fix a typo in patch 2 on QEMU_PREV_VERSION
> - Added R-bs for Dan
> 
> Hi,
> 
> This small patchset updates the recent cross-binary test for migration on
> a few things.
> 
> Patch 1 modifies the aarch64 test GIC version to 3 rather than "max",
> paving way for enabling it, even if the CPU model is not yet ready.
> 
> Patch 2 removes the tag dependency of the new build-previous-qemu job, so
> that in personal CI pipelines the job won't fail if the tag is missing, as
> reported by Peter Maydell, and solution suggested by Dan.
> 
> Patch 3 updates the comment for aarch64 on the test to state the fact, and
> what is missing.  Then we don't target it support for v9.0, but only until
> we have a stable CPU model for aarch64 (if ever possible to support both
> tcg and kvm).
> 
> Comments welcomed, thanks.

queued.

-- 
Peter Xu

Re: [PATCH v2 1/3] tests/migration-test: Stick with gicv3 in aarch64 test

[Thomas Huth]

On 07/02/2024 01.54, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> Recently we introduced cross-binary migration test.  It's always wanted
> that migration-test uses stable guest ABI for both QEMU binaries in this
> case, so that both QEMU binaries will be compatible on the migration
> stream with the cmdline specified.
> 
> Switch to a static gic version "3" rather than using version "max", so that
> GIC should be stable now across any future QEMU binaries for migration-test.
> 
> Here the version can actually be anything as long as the ABI is stable.  We
> choose "3" because it's the majority of what we already use in QEMU while
> still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
> no direct user yet besides "max".
> 
> Note that even with this change, aarch64 won't be able to work yet with
> migration cross binary test, but then the only missing piece will be the
> stable CPU model.
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 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..8a5bb1752e 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -819,7 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>       } else if (strcmp(arch, "aarch64") == 0) {
>           memory_size = "150M";
>           machine_alias = "virt";
> -        machine_opts = "gic-version=max";
> +        machine_opts = "gic-version=3";
>           arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
>           start_address = ARM_TEST_MEM_START;
>           end_address = ARM_TEST_MEM_END;

Looks like the migration test now fails on aarch64 when "configure" has been 
run with "--without-default-devices", since that disables the gicv3 in the 
binary ... is there a way to check whether the gicv3 is available, and use 
"=max" instead if it is not?

  Thomas

Re: [PATCH v2 1/3] tests/migration-test: Stick with gicv3 in aarch64 test

[Peter Maydell]

On Mon, 19 Feb 2024 at 11:54, Thomas Huth <thuth@redhat.com> wrote:
>
> On 07/02/2024 01.54, peterx@redhat.com wrote:
> > From: Peter Xu <peterx@redhat.com>
> >
> > Recently we introduced cross-binary migration test.  It's always wanted
> > that migration-test uses stable guest ABI for both QEMU binaries in this
> > case, so that both QEMU binaries will be compatible on the migration
> > stream with the cmdline specified.
> >
> > Switch to a static gic version "3" rather than using version "max", so that
> > GIC should be stable now across any future QEMU binaries for migration-test.
> >
> > Here the version can actually be anything as long as the ABI is stable.  We
> > choose "3" because it's the majority of what we already use in QEMU while
> > still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
> > no direct user yet besides "max".
> >
> > Note that even with this change, aarch64 won't be able to work yet with
> > migration cross binary test, but then the only missing piece will be the
> > stable CPU model.
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > 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..8a5bb1752e 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -819,7 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >       } else if (strcmp(arch, "aarch64") == 0) {
> >           memory_size = "150M";
> >           machine_alias = "virt";
> > -        machine_opts = "gic-version=max";
> > +        machine_opts = "gic-version=3";
> >           arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
> >           start_address = ARM_TEST_MEM_START;
> >           end_address = ARM_TEST_MEM_END;
>
> Looks like the migration test now fails on aarch64 when "configure" has been
> run with "--without-default-devices", since that disables the gicv3 in the
> binary ... is there a way to check whether the gicv3 is available, and use
> "=max" instead if it is not?

A QEMU for AArch64 with no GICv3 is of very little practical use,
so I'm not sure it makes sense to allow users to build one like that.
(I'm also a little surprised that it worked with 'max', because
without a GICv3 it would try to fall back to GICv2, and if we're
going to allow users to compile-time disable one of the GICs then
we should definitely allow them to choose to not build GICv2.)

I think I would go for disabling the migration test entirely if
the build doesn't include the GICv3.

thanks
-- PMM

Re: [PATCH v2 1/3] tests/migration-test: Stick with gicv3 in aarch64 test

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 19 Feb 2024 at 11:54, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 07/02/2024 01.54, peterx@redhat.com wrote:
>> > From: Peter Xu <peterx@redhat.com>
>> >
>> > Recently we introduced cross-binary migration test.  It's always wanted
>> > that migration-test uses stable guest ABI for both QEMU binaries in this
>> > case, so that both QEMU binaries will be compatible on the migration
>> > stream with the cmdline specified.
>> >
>> > Switch to a static gic version "3" rather than using version "max", so that
>> > GIC should be stable now across any future QEMU binaries for migration-test.
>> >
>> > Here the version can actually be anything as long as the ABI is stable.  We
>> > choose "3" because it's the majority of what we already use in QEMU while
>> > still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
>> > no direct user yet besides "max".
>> >
>> > Note that even with this change, aarch64 won't be able to work yet with
>> > migration cross binary test, but then the only missing piece will be the
>> > stable CPU model.
>> >
>> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>> > 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..8a5bb1752e 100644
>> > --- a/tests/qtest/migration-test.c
>> > +++ b/tests/qtest/migration-test.c
>> > @@ -819,7 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >       } else if (strcmp(arch, "aarch64") == 0) {
>> >           memory_size = "150M";
>> >           machine_alias = "virt";
>> > -        machine_opts = "gic-version=max";
>> > +        machine_opts = "gic-version=3";
>> >           arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
>> >           start_address = ARM_TEST_MEM_START;
>> >           end_address = ARM_TEST_MEM_END;
>>
>> Looks like the migration test now fails on aarch64 when "configure" has been
>> run with "--without-default-devices", since that disables the gicv3 in the
>> binary ... is there a way to check whether the gicv3 is available, and use
>> "=max" instead if it is not?

How about adding aarch64 to the set of targets on the without-defaults
CI job?

>
> A QEMU for AArch64 with no GICv3 is of very little practical use,
> so I'm not sure it makes sense to allow users to build one like that.
> (I'm also a little surprised that it worked with 'max', because
> without a GICv3 it would try to fall back to GICv2, and if we're
> going to allow users to compile-time disable one of the GICs then
> we should definitely allow them to choose to not build GICv2.)
>
> I think I would go for disabling the migration test entirely if
> the build doesn't include the GICv3.

Makes sense. I've got my hands full right now, but I'll work on it later
this week along with your other suggestion to only test TCG.

Thanks

Re: [PATCH v2 1/3] tests/migration-test: Stick with gicv3 in aarch64 test

[Thomas Huth]

On 19/02/2024 13.50, Peter Maydell wrote:
> On Mon, 19 Feb 2024 at 11:54, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 07/02/2024 01.54, peterx@redhat.com wrote:
>>> From: Peter Xu <peterx@redhat.com>
>>>
>>> Recently we introduced cross-binary migration test.  It's always wanted
>>> that migration-test uses stable guest ABI for both QEMU binaries in this
>>> case, so that both QEMU binaries will be compatible on the migration
>>> stream with the cmdline specified.
>>>
>>> Switch to a static gic version "3" rather than using version "max", so that
>>> GIC should be stable now across any future QEMU binaries for migration-test.
>>>
>>> Here the version can actually be anything as long as the ABI is stable.  We
>>> choose "3" because it's the majority of what we already use in QEMU while
>>> still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
>>> no direct user yet besides "max".
>>>
>>> Note that even with this change, aarch64 won't be able to work yet with
>>> migration cross binary test, but then the only missing piece will be the
>>> stable CPU model.
>>>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> 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..8a5bb1752e 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
>>> @@ -819,7 +819,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>>        } else if (strcmp(arch, "aarch64") == 0) {
>>>            memory_size = "150M";
>>>            machine_alias = "virt";
>>> -        machine_opts = "gic-version=max";
>>> +        machine_opts = "gic-version=3";
>>>            arch_opts = g_strdup_printf("-cpu max -kernel %s", bootpath);
>>>            start_address = ARM_TEST_MEM_START;
>>>            end_address = ARM_TEST_MEM_END;
>>
>> Looks like the migration test now fails on aarch64 when "configure" has been
>> run with "--without-default-devices", since that disables the gicv3 in the
>> binary ... is there a way to check whether the gicv3 is available, and use
>> "=max" instead if it is not?
> 
> A QEMU for AArch64 with no GICv3 is of very little practical use,
> so I'm not sure it makes sense to allow users to build one like that.

Ok, if it doesn't make too much sense to build without GICv3, maybe a patch 
like this could be the right solution:

diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -12,10 +12,6 @@ config IOAPIC
      bool
      select I8259

-config ARM_GIC
-    bool
-    select MSI_NONBROKEN
-
  config OPENPIC
      bool
      select MSI_NONBROKEN
@@ -25,14 +21,18 @@ config APIC
      select MSI_NONBROKEN
      select I8259

+config ARM_GIC
+    bool
+    select ARM_GICV3_TCG if TCG
+    select ARM_GIC_KVM if KVM
+    select MSI_NONBROKEN
+
  config ARM_GICV3_TCG
      bool
-    default y
      depends on ARM_GIC && TCG

  config ARM_GIC_KVM
      bool
-    default y
      depends on ARM_GIC && KVM

  config XICS

?

At least this seems to fix the the migration-test when compiling with 
--without-default-devices.

  Thomas