| Message ID | 20230426180013.14814-7-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Allow CONFIG_TCG=n builds | expand |
On 26/04/2023 20.00, Fabiano Rosas wrote: > We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the > query-cpu-model-expansion test to check against the cortex-a7, which > is already under CONFIG_TCG. That allows the next patch to contain > only code movement. > > While here add comments clarifying what we're testing. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) Acked-by: Thomas Huth <thuth@redhat.com>
On 4/26/23 19:00, Fabiano Rosas wrote: > We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the > query-cpu-model-expansion test to check against the cortex-a7, which > is already under CONFIG_TCG. That allows the next patch to contain > only code movement. > > While here add comments clarifying what we're testing. > > Signed-off-by: Fabiano Rosas<farosas@suse.de> > Suggested-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- > tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) I don't see why you're changing the cpu model here. Neither cpu will work, of course, but why change? r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 4/26/23 19:00, Fabiano Rosas wrote: >> We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the >> query-cpu-model-expansion test to check against the cortex-a7, which >> is already under CONFIG_TCG. That allows the next patch to contain >> only code movement. >> >> While here add comments clarifying what we're testing. >> >> Signed-off-by: Fabiano Rosas<farosas@suse.de> >> Suggested-by: Philippe Mathieu-Daudé<philmd@linaro.org> >> --- >> tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) > > I don't see why you're changing the cpu model here. > Neither cpu will work, of course, but why change? > Because there's already a patch in master that puts the cortex-a7 under CONFIG_TCG, so I can have the whole if/else in this patch. If I keep the cortex-a15, this change needs to go into the next patch ("move cpu_tcg to tcg/cpu32.c") which moves the rest of the 32bit cpus, which was supposed to be only code movement.
On 4/27/23 14:16, Fabiano Rosas wrote: > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 4/26/23 19:00, Fabiano Rosas wrote: >>> We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the >>> query-cpu-model-expansion test to check against the cortex-a7, which >>> is already under CONFIG_TCG. That allows the next patch to contain >>> only code movement. >>> >>> While here add comments clarifying what we're testing. >>> >>> Signed-off-by: Fabiano Rosas<farosas@suse.de> >>> Suggested-by: Philippe Mathieu-Daudé<philmd@linaro.org> >>> --- >>> tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- >>> 1 file changed, 17 insertions(+), 3 deletions(-) >> >> I don't see why you're changing the cpu model here. >> Neither cpu will work, of course, but why change? >> > > Because there's already a patch in master that puts the cortex-a7 under > CONFIG_TCG, so I can have the whole if/else in this patch. > > If I keep the cortex-a15, this change needs to go into the next patch > ("move cpu_tcg to tcg/cpu32.c") which moves the rest of the 32bit cpus, > which was supposed to be only code movement. Well, I still think the change to a7 is wrong. If the two patches need to be merged to break bisection, then so be it -- just mention that fact in the commit message. Peter, do you agree? r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 4/27/23 14:16, Fabiano Rosas wrote: >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> On 4/26/23 19:00, Fabiano Rosas wrote: >>>> We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the >>>> query-cpu-model-expansion test to check against the cortex-a7, which >>>> is already under CONFIG_TCG. That allows the next patch to contain >>>> only code movement. >>>> >>>> While here add comments clarifying what we're testing. >>>> >>>> Signed-off-by: Fabiano Rosas<farosas@suse.de> >>>> Suggested-by: Philippe Mathieu-Daudé<philmd@linaro.org> >>>> --- >>>> tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- >>>> 1 file changed, 17 insertions(+), 3 deletions(-) >>> >>> I don't see why you're changing the cpu model here. >>> Neither cpu will work, of course, but why change? >>> >> >> Because there's already a patch in master that puts the cortex-a7 under >> CONFIG_TCG, so I can have the whole if/else in this patch. >> >> If I keep the cortex-a15, this change needs to go into the next patch >> ("move cpu_tcg to tcg/cpu32.c") which moves the rest of the 32bit cpus, >> which was supposed to be only code movement. > > Well, I still think the change to a7 is wrong. > If the two patches need to be merged to break bisection, > then so be it -- just mention that fact in the commit message. > I don't get why it would be wrong. The test just needs any cpu model that triggers the message at qmp_query_cpu_model_expansion.
On Fri, 28 Apr 2023 at 08:45, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/27/23 14:16, Fabiano Rosas wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > > > >> On 4/26/23 19:00, Fabiano Rosas wrote: > >>> We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the > >>> query-cpu-model-expansion test to check against the cortex-a7, which > >>> is already under CONFIG_TCG. That allows the next patch to contain > >>> only code movement. > >>> > >>> While here add comments clarifying what we're testing. > >>> > >>> Signed-off-by: Fabiano Rosas<farosas@suse.de> > >>> Suggested-by: Philippe Mathieu-Daudé<philmd@linaro.org> > >>> --- > >>> tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- > >>> 1 file changed, 17 insertions(+), 3 deletions(-) > >> > >> I don't see why you're changing the cpu model here. > >> Neither cpu will work, of course, but why change? > >> > > > > Because there's already a patch in master that puts the cortex-a7 under > > CONFIG_TCG, so I can have the whole if/else in this patch. > > > > If I keep the cortex-a15, this change needs to go into the next patch > > ("move cpu_tcg to tcg/cpu32.c") which moves the rest of the 32bit cpus, > > which was supposed to be only code movement. > > Well, I still think the change to a7 is wrong. > If the two patches need to be merged to break bisection, > then so be it -- just mention that fact in the commit message. > > Peter, do you agree? I see your point, but on the other hand this is only test code, and the situation it's checking is a rather uninteresting one anyway. So given that this series is already on v11 and it's an awkward one to have to keep rebasing, I'd rather take it as-is than ask for a reroll. thanks -- PMM
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c index 1cb08138ad..3fc33fc24d 100644 --- a/tests/qtest/arm-cpu-features.c +++ b/tests/qtest/arm-cpu-features.c @@ -506,9 +506,23 @@ static void test_query_cpu_model_expansion_kvm(const void *data) QDict *resp; char *error; - assert_error(qts, "cortex-a15", - "We cannot guarantee the CPU type 'cortex-a15' works " - "with KVM on this host", NULL); + /* + * When using KVM, only the 'host' and 'max' CPU models are + * supported. Test that we're emitting a suitable error for + * unsupported CPU models. + */ + if (qtest_has_accel("tcg")) { + assert_error(qts, "cortex-a7", + "We cannot guarantee the CPU type 'cortex-a7' works " + "with KVM on this host", NULL); + } else { + /* + * With a KVM-only build the 32-bit CPUs are not present. + */ + assert_error(qts, "cortex-a7", + "The CPU type 'cortex-a7' is not a " + "recognized ARM CPU type", NULL); + } assert_has_feature_enabled(qts, "host", "aarch64");
We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the query-cpu-model-expansion test to check against the cortex-a7, which is already under CONFIG_TCG. That allows the next patch to contain only code movement. While here add comments clarifying what we're testing. Signed-off-by: Fabiano Rosas <farosas@suse.de> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- tests/qtest/arm-cpu-features.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)