Re: [kvm-unit-tests PATCH v2 4/4] arm64: split its-migrate-unmapped-collection into KVM and TCG variants

From: Andrew Jones <drjones@redhat.com>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: Auger Eric <eric.auger@redhat.com>,
	kvm@vger.kernel.org, maz@kernel.org, shashi.mallela@linaro.org,
	qemu-arm@nongnu.org, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [kvm-unit-tests PATCH v2 4/4] arm64: split its-migrate-unmapped-collection into KVM and TCG variants
Date: Tue, 1 Jun 2021 21:46:48 +0200	[thread overview]
Message-ID: <20210601194648.fdymtxiz6lkxycsx@gator.home> (raw)
In-Reply-To: <87sg21bk7r.fsf@linaro.org>

On Tue, Jun 01, 2021 at 05:49:01PM +0100, Alex Bennée wrote:
> 
> Auger Eric <eric.auger@redhat.com> writes:
> 
> > Hi Alex,
> >
> > On 5/25/21 7:26 PM, Alex Bennée wrote:
> >> When running the test in TCG we are basically running on bare metal so
> >> don't rely on having a particular kernel errata applied.
> >> 
> >> You might wonder why we handle this with a totally new test name
> >> instead of adjusting the append to take an extra parameter? Well the
> >> run_migration shell script uses eval "$@" which unwraps the -append
> >> leading to any second parameter being split and leaving QEMU very
> >> confused and the test hanging. This seemed simpler than re-writing all
> >> the test running logic in something sane ;-)
> >
> > there is
> > lib/s390x/vm.h:bool vm_is_tcg(void)
> >
> > but I don't see any particular ID we could use to differentiate both the
> > KVM and the TCG mode, do you?
> 
> For -cpu max we do:
> 
>         /*
>          * Reset MIDR so the guest doesn't mistake our 'max' CPU type for a real
>          * one and try to apply errata workarounds or use impdef features we
>          * don't provide.
>          * An IMPLEMENTER field of 0 means "reserved for software use";
>          * ARCHITECTURE must be 0xf indicating "v7 or later, check ID registers
>          * to see which features are present";
>          * the VARIANT, PARTNUM and REVISION fields are all implementation
>          * defined and we choose to define PARTNUM just in case guest
>          * code needs to distinguish this QEMU CPU from other software
>          * implementations, though this shouldn't be needed.
>          */
>         t = FIELD_DP64(0, MIDR_EL1, IMPLEMENTER, 0);
>         t = FIELD_DP64(t, MIDR_EL1, ARCHITECTURE, 0xf);
>         t = FIELD_DP64(t, MIDR_EL1, PARTNUM, 'Q');
>         t = FIELD_DP64(t, MIDR_EL1, VARIANT, 0);
>         t = FIELD_DP64(t, MIDR_EL1, REVISION, 0);
>         cpu->midr = t;
> 
> However for the default -cpu cortex-a57 we aim to look just like the
> real thing - only without any annoying micro-architecture bugs ;-)
> 
> >
> > without a more elegant solution,
> 
> I'll look into the suggestion made by Richard.

Where did Richard make a suggestion? And what is it?

Thanks,
drew

> 
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> >
> > Thanks
> >
> > Eric
> >
> >
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Shashi Mallela <shashi.mallela@linaro.org>
> >> ---
> >>  arm/gic.c         |  8 +++++++-
> >>  arm/unittests.cfg | 10 +++++++++-
> >>  2 files changed, 16 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arm/gic.c b/arm/gic.c
> >> index bef061a..0fce2a4 100644
> >> --- a/arm/gic.c
> >> +++ b/arm/gic.c
> >> @@ -36,6 +36,7 @@ static struct gic *gic;
> >>  static int acked[NR_CPUS], spurious[NR_CPUS];
> >>  static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
> >>  static cpumask_t ready;
> >> +static bool under_tcg;
> >>  
> >>  static void nr_cpu_check(int nr)
> >>  {
> >> @@ -834,7 +835,7 @@ static void test_migrate_unmapped_collection(void)
> >>  		goto do_migrate;
> >>  	}
> >>  
> >> -	if (!errata(ERRATA_UNMAPPED_COLLECTIONS)) {
> >> +	if (!errata(ERRATA_UNMAPPED_COLLECTIONS) && !under_tcg) {
> >>  		report_skip("Skipping test, as this test hangs without the fix. "
> >>  			    "Set %s=y to enable.", ERRATA_UNMAPPED_COLLECTIONS);
> >>  		test_skipped = true;
> >> @@ -1005,6 +1006,11 @@ int main(int argc, char **argv)
> >>  		report_prefix_push(argv[1]);
> >>  		test_migrate_unmapped_collection();
> >>  		report_prefix_pop();
> >> +	} else if (!strcmp(argv[1], "its-migrate-unmapped-collection-tcg")) {
> >> +		under_tcg = true;
> >> +		report_prefix_push(argv[1]);
> >> +		test_migrate_unmapped_collection();
> >> +		report_prefix_pop();
> >>  	} else if (strcmp(argv[1], "its-introspection") == 0) {
> >>  		report_prefix_push(argv[1]);
> >>  		test_its_introspection();
> >> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> >> index 1a39428..adc1bbf 100644
> >> --- a/arm/unittests.cfg
> >> +++ b/arm/unittests.cfg
> >> @@ -205,7 +205,7 @@ extra_params = -machine gic-version=3 -append 'its-pending-migration'
> >>  groups = its migration
> >>  arch = arm64
> >>  
> >> -[its-migrate-unmapped-collection]
> >> +[its-migrate-unmapped-collection-kvm]
> >>  file = gic.flat
> >>  smp = $MAX_SMP
> >>  accel = kvm
> >> @@ -213,6 +213,14 @@ extra_params = -machine gic-version=3 -append 'its-migrate-unmapped-collection'
> >>  groups = its migration
> >>  arch = arm64
> >>  
> >> +[its-migrate-unmapped-collection-tcg]
> >> +file = gic.flat
> >> +smp = $MAX_SMP
> >> +accel = tcg
> >> +extra_params = -machine gic-version=3 -append 'its-migrate-unmapped-collection-tcg'
> >> +groups = its migration
> >> +arch = arm64
> >> +
> >>  # Test PSCI emulation
> >>  [psci]
> >>  file = psci.flat
> >> 
> 
> 
> -- 
> Alex Bennée
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm