| Message ID | 20210525172628.2088-5-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable LPI and ITS for TCG | expand |
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? without a more elegant solution, 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 >
On Tue, May 25, 2021 at 06:26:28PM +0100, 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 ;-) Yes, bash is a pain for this. I may try to get migration with more than one parameter to work at some point though. But, for generally determining if a unit test is running with tcg or with kvm, we have the QEMU_ACCEL environment variable. So you could just do getenv("QEMU_ACCEL") in the unit test. However, I wouldn't use it for this case, since the purpose is just to force errata to be ignored. We have the "ERRATA_FORCE" environment variable for that already. You can set it yourself, e.g. $ ERRATA_FORCE=y tests/its-migration or, if you plan to run all tests, then with $ ./run_tests.sh -a but that also runs nodefault tests. Maybe we should teach run_tests.sh to always set ERRATA_FORCE=y when running with TCG? Thanks, drew
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. > 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 >>
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
Andrew Jones <drjones@redhat.com> writes: > 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? Sorry - I had a brain fart, I was of course referring to your ERRATA_FORCE suggestion. > > 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
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
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 ;-) 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(-)