docs/arm-cpu-features.rst | 317 ++++++++++++++++++++++ include/qemu/bitops.h | 1 + qapi/machine-target.json | 6 +- target/arm/cpu.c | 25 +- target/arm/cpu.h | 21 ++ target/arm/cpu64.c | 358 ++++++++++++++++++++++++- target/arm/helper.c | 10 +- target/arm/kvm.c | 25 +- target/arm/kvm32.c | 6 +- target/arm/kvm64.c | 323 ++++++++++++++++++++--- target/arm/kvm_arm.h | 39 +++ target/arm/monitor.c | 158 +++++++++++ tests/Makefile.include | 5 +- tests/arm-cpu-features.c | 542 ++++++++++++++++++++++++++++++++++++++ 14 files changed, 1778 insertions(+), 58 deletions(-) create mode 100644 docs/arm-cpu-features.rst create mode 100644 tests/arm-cpu-features.c
Since Linux kernel v5.2-rc1 KVM has support for enabling SVE in guests.
This series provides the QEMU bits for that enablement. First, we
select existing CPU properties representing features we want to
advertise in addition to the SVE vector lengths and prepare
them for a qmp query. Then we introduce the qmp query, applying
it immediately to those selected features. We also document ARM CPU
features at this time. We next add a qtest for the selected CPU
features that uses the qmp query for its tests - and we continue to
add tests as we add CPU features with the following patches. So then,
once we have the support we need for CPU feature querying and testing,
we add our first SVE CPU feature property, 'sve', which just allows
SVE to be completely enabled/disabled. Following that feature property,
we add all 16 vector length properties along with the input validation
they need and tests to prove the validation works. At this point the
SVE features are still only for TCG, so we provide some patches to
prepare for KVM and then a patch that allows the 'max' CPU type to
enable SVE with KVM, but at first without vector length properties.
After a bit more preparation we add the SVE vector length properties
to the KVM-enabled 'max' CPU type along with the additional input
validation and tests that that needs. Finally we allow the 'host'
CPU type to also enjoy these properties by simply sharing them with it.
v6:
- Adding missing visit_free() to an error path in
qmp_query_cpu_model_expansion() in patch 1/9.
- Rebased on latest master (applied cleanly)
- Picked up r-b's from Richard and Eric
v5:
- Now generate an error if vector lengths have been explicitly
enabled, but SVE is disabled
- Fixed a bug in sve_zcr_len_for_el()
- Fixed a bug in kvm_arch_put/get_sve() and brought back the
put/get of FPSR/FPCR
- A few document clarifications and added some new sentences
- Added a couple more tests
- Added BIT_ULL and use it in the test
- Removed an unnecessary bitmap search
- Moved a cpu_max_get_sve_max_vq() hunk from 4/9 to 3/9 and
added a fix for it in 8/9
- Picked up some more tags from Eric
v4:
- Integrated Richard Henderson's rework for the sve property
validation, in order to do all validating at finalize time
and save several lines of code.
- Fixed 'host' cpu SVE default. It was still off by default.
- Cleaned up #ifdef's for sve_bswap64()
- Removed redundant KVM_CAP_ARM_SVE extension check in
kvm_arm_sve_get_vls()
- Improved the KVM SVE qtest
- Renamed sve<vl-bits> to sve<N> everywhere
- Renamed power-of-2 to power-of-two everywhere
- Picked up some more tags from Richard
Thanks!
drew
Andrew Jones (9):
target/arm/monitor: Introduce qmp_query_cpu_model_expansion
tests: arm: Introduce cpu feature tests
target/arm: Allow SVE to be disabled via a CPU property
target/arm/cpu64: max cpu: Introduce sve<N> properties
target/arm/kvm64: Add kvm_arch_get/put_sve
target/arm/kvm64: max cpu: Enable SVE when available
target/arm/kvm: scratch vcpu: Preserve input kvm_vcpu_init features
target/arm/cpu64: max cpu: Support sve properties with KVM
target/arm/kvm: host cpu: Add support for sve<N> properties
docs/arm-cpu-features.rst | 317 ++++++++++++++++++++++
include/qemu/bitops.h | 1 +
qapi/machine-target.json | 6 +-
target/arm/cpu.c | 25 +-
target/arm/cpu.h | 21 ++
target/arm/cpu64.c | 356 ++++++++++++++++++++++++-
target/arm/helper.c | 10 +-
target/arm/kvm.c | 25 +-
target/arm/kvm32.c | 6 +-
target/arm/kvm64.c | 323 ++++++++++++++++++++---
target/arm/kvm_arm.h | 39 +++
target/arm/monitor.c | 157 +++++++++++
tests/Makefile.include | 5 +-
tests/arm-cpu-features.c | 542 ++++++++++++++++++++++++++++++++++++++
14 files changed, 1776 insertions(+), 57 deletions(-)
create mode 100644 docs/arm-cpu-features.rst
create mode 100644 tests/arm-cpu-features.c
--
2.20.1
*** BLURB HERE ***
Andrew Jones (9):
target/arm/monitor: Introduce qmp_query_cpu_model_expansion
tests: arm: Introduce cpu feature tests
target/arm: Allow SVE to be disabled via a CPU property
target/arm/cpu64: max cpu: Introduce sve<N> properties
target/arm/kvm64: Add kvm_arch_get/put_sve
target/arm/kvm64: max cpu: Enable SVE when available
target/arm/kvm: scratch vcpu: Preserve input kvm_vcpu_init features
target/arm/cpu64: max cpu: Support sve properties with KVM
target/arm/kvm: host cpu: Add support for sve<N> properties
docs/arm-cpu-features.rst | 317 ++++++++++++++++++++++
include/qemu/bitops.h | 1 +
qapi/machine-target.json | 6 +-
target/arm/cpu.c | 25 +-
target/arm/cpu.h | 21 ++
target/arm/cpu64.c | 358 ++++++++++++++++++++++++-
target/arm/helper.c | 10 +-
target/arm/kvm.c | 25 +-
target/arm/kvm32.c | 6 +-
target/arm/kvm64.c | 323 ++++++++++++++++++++---
target/arm/kvm_arm.h | 39 +++
target/arm/monitor.c | 158 +++++++++++
tests/Makefile.include | 5 +-
tests/arm-cpu-features.c | 542 ++++++++++++++++++++++++++++++++++++++
14 files changed, 1778 insertions(+), 58 deletions(-)
create mode 100644 docs/arm-cpu-features.rst
create mode 100644 tests/arm-cpu-features.c
--
2.21.0
On Wed, 16 Oct 2019 at 09:54, Andrew Jones <drjones@redhat.com> wrote:
>
> Since Linux kernel v5.2-rc1 KVM has support for enabling SVE in guests.
> This series provides the QEMU bits for that enablement. First, we
> select existing CPU properties representing features we want to
> advertise in addition to the SVE vector lengths and prepare
> them for a qmp query. Then we introduce the qmp query, applying
> it immediately to those selected features. We also document ARM CPU
> features at this time. We next add a qtest for the selected CPU
> features that uses the qmp query for its tests - and we continue to
> add tests as we add CPU features with the following patches. So then,
> once we have the support we need for CPU feature querying and testing,
> we add our first SVE CPU feature property, 'sve', which just allows
> SVE to be completely enabled/disabled. Following that feature property,
> we add all 16 vector length properties along with the input validation
> they need and tests to prove the validation works. At this point the
> SVE features are still only for TCG, so we provide some patches to
> prepare for KVM and then a patch that allows the 'max' CPU type to
> enable SVE with KVM, but at first without vector length properties.
> After a bit more preparation we add the SVE vector length properties
> to the KVM-enabled 'max' CPU type along with the additional input
> validation and tests that that needs. Finally we allow the 'host'
> CPU type to also enjoy these properties by simply sharing them with it.
This fails 'make check' on an aarch32 box with KVM support:
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm QTEST_QEMU_IMG=qemu-img
tests/arm-cpu-features -m=quick -k --tap < /dev/null |
./scripts/tap-driver.pl --test-name="arm-cpu-features"
PASS 1 arm-cpu-features /arm/arm/query-cpu-model-expansion
**
ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:420:test_query_cpu_model_expansion_kvm:
assertion failed: (resp_has_props(_resp))
Aborted
ERROR - too few tests run (expected 2, got 1)
/home/pm215/qemu/tests/Makefile.include:902: recipe for target
'check-qtest-arm' failed
make: *** [check-qtest-arm] Error 1
make: Leaving directory '/home/pm215/qemu/build/arm'
thanks
-- PMM
On 10/21/19 2:36 PM, Peter Maydell wrote:
> On Wed, 16 Oct 2019 at 09:54, Andrew Jones <drjones@redhat.com> wrote:
>>
>> Since Linux kernel v5.2-rc1 KVM has support for enabling SVE in guests.
>> This series provides the QEMU bits for that enablement. First, we
>> select existing CPU properties representing features we want to
>> advertise in addition to the SVE vector lengths and prepare
>> them for a qmp query. Then we introduce the qmp query, applying
>> it immediately to those selected features. We also document ARM CPU
>> features at this time. We next add a qtest for the selected CPU
>> features that uses the qmp query for its tests - and we continue to
>> add tests as we add CPU features with the following patches. So then,
>> once we have the support we need for CPU feature querying and testing,
>> we add our first SVE CPU feature property, 'sve', which just allows
>> SVE to be completely enabled/disabled. Following that feature property,
>> we add all 16 vector length properties along with the input validation
>> they need and tests to prove the validation works. At this point the
>> SVE features are still only for TCG, so we provide some patches to
>> prepare for KVM and then a patch that allows the 'max' CPU type to
>> enable SVE with KVM, but at first without vector length properties.
>> After a bit more preparation we add the SVE vector length properties
>> to the KVM-enabled 'max' CPU type along with the additional input
>> validation and tests that that needs. Finally we allow the 'host'
>> CPU type to also enjoy these properties by simply sharing them with it.
>
> This fails 'make check' on an aarch32 box with KVM support:
>
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm QTEST_QEMU_IMG=qemu-img
> tests/arm-cpu-features -m=quick -k --tap < /dev/null |
> ./scripts/tap-driver.pl --test-name="arm-cpu-features"
> PASS 1 arm-cpu-features /arm/arm/query-cpu-model-expansion
> **
> ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:420:test_query_cpu_model_expansion_kvm:
> assertion failed: (resp_has_props(_resp))
Glad you could test it, I was wondering how this test work because it
first unconditionally assert the host has PMU feature (failing the test)
then there is a unreachable if(!aarch64) "'pmu' feature not supported"
warning.
On Mon, Oct 21, 2019 at 02:54:30PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/21/19 2:36 PM, Peter Maydell wrote:
> > On Wed, 16 Oct 2019 at 09:54, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > Since Linux kernel v5.2-rc1 KVM has support for enabling SVE in guests.
> > > This series provides the QEMU bits for that enablement. First, we
> > > select existing CPU properties representing features we want to
> > > advertise in addition to the SVE vector lengths and prepare
> > > them for a qmp query. Then we introduce the qmp query, applying
> > > it immediately to those selected features. We also document ARM CPU
> > > features at this time. We next add a qtest for the selected CPU
> > > features that uses the qmp query for its tests - and we continue to
> > > add tests as we add CPU features with the following patches. So then,
> > > once we have the support we need for CPU feature querying and testing,
> > > we add our first SVE CPU feature property, 'sve', which just allows
> > > SVE to be completely enabled/disabled. Following that feature property,
> > > we add all 16 vector length properties along with the input validation
> > > they need and tests to prove the validation works. At this point the
> > > SVE features are still only for TCG, so we provide some patches to
> > > prepare for KVM and then a patch that allows the 'max' CPU type to
> > > enable SVE with KVM, but at first without vector length properties.
> > > After a bit more preparation we add the SVE vector length properties
> > > to the KVM-enabled 'max' CPU type along with the additional input
> > > validation and tests that that needs. Finally we allow the 'host'
> > > CPU type to also enjoy these properties by simply sharing them with it.
> >
> > This fails 'make check' on an aarch32 box with KVM support:
> >
> > MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm QTEST_QEMU_IMG=qemu-img
> > tests/arm-cpu-features -m=quick -k --tap < /dev/null |
> > ./scripts/tap-driver.pl --test-name="arm-cpu-features"
> > PASS 1 arm-cpu-features /arm/arm/query-cpu-model-expansion
> > **
> > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:420:test_query_cpu_model_expansion_kvm:
> > assertion failed: (resp_has_props(_resp))
>
> Glad you could test it, I was wondering how this test work because it first
> unconditionally assert the host has PMU feature (failing the test) then
> there is a unreachable if(!aarch64) "'pmu' feature not supported" warning.
>
Yes, this is the case you were concerned about, but the problem is for a
slightly different reason. *If* the PMU feature was getting added to the
kvm32 'host' CPU type, then both tests here would be correct: the feature
would be present, but arm_set_pmu() would report "feature not supported",
as that's what KVM would report on kvm32. However, I see now that
target/arm/kvm32.c:kvm_arm_get_host_cpu_features() does *not* add
ARM_FEATURE_PMU, like it does in the target/arm/kvm64.c implementation
and in the 32-bit CPU models like the Cortex-A15. Indeed, had this
test been run with '-cpu cortex-a15', and the kvm32 host allowed that
CPU type, then this test would also work. We want to use 'host' here
though, as that's more appropriate, so the fix is simply to remove the
tests on kvm32 like below.
Peter, would you mind running your test on the kvm32 machine with this
change before I send a v7? Actually, do I need to send a v7? Or is
type of fixup OK to do on merge?
Thanks,
drew
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 92668efb8f56..14100ebd8521 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -417,8 +417,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
qts = qtest_init(MACHINE "-accel kvm -cpu host");
- assert_has_feature(qts, "host", "pmu");
-
if (g_str_equal(qtest_get_arch(), "aarch64")) {
bool kvm_supports_sve;
char max_name[8], name[8];
@@ -428,6 +426,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
char *error;
assert_has_feature(qts, "host", "aarch64");
+ assert_has_feature(qts, "host", "pmu");
assert_error(qts, "cortex-a15",
"We cannot guarantee the CPU type 'cortex-a15' works "
@@ -497,9 +496,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
}
} else {
assert_has_not_feature(qts, "host", "sve");
- assert_error(qts, "host",
- "'pmu' feature not supported by KVM on this host",
- "{ 'pmu': true }");
}
qtest_quit(qts);
On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote: > Peter, would you mind running your test on the kvm32 machine with this > change before I send a v7? Still fails: pm215@pm-ct:~/qemu/build/arm$ QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features /arm/arm/query-cpu-model-expansion: OK /arm/arm/kvm/query-cpu-model-expansion: ** ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm: assertion failed: (resp_has_props(_resp)) Aborted This is asserting on the line: 498 assert_has_not_feature(qts, "host", "sve"); thanks -- PMM
On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote:
> > Peter, would you mind running your test on the kvm32 machine with this
> > change before I send a v7?
>
> Still fails:
>
> pm215@pm-ct:~/qemu/build/arm$
> QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> /arm/arm/query-cpu-model-expansion: OK
> /arm/arm/kvm/query-cpu-model-expansion: **
> ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> assertion failed: (resp_has_props(_resp))
> Aborted
>
> This is asserting on the line:
> 498 assert_has_not_feature(qts, "host", "sve");
>
Oh, I see. It's not failing the specific absence of 'sve', but the test
code (assert_has_not_feature()) is assuming at least one property is
present. This isn't the case for kvm32 'host' cpus. They apparently
have none. We need this patch too, then
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 14100ebd8521..9aa728ed8469 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const char *feature)
({ \
QDict *_resp = do_query_no_props(qts, cpu_type); \
g_assert(_resp); \
- g_assert(resp_has_props(_resp)); \
- g_assert(!qdict_get(resp_get_props(_resp), feature)); \
+ g_assert(!resp_has_props(_resp) || \
+ !qdict_get(resp_get_props(_resp), feature)); \
qobject_unref(_resp); \
})
Thanks,
drew
On Mon, 21 Oct 2019 at 17:12, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> > On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote:
> > > Peter, would you mind running your test on the kvm32 machine with this
> > > change before I send a v7?
> >
> > Still fails:
> >
> > pm215@pm-ct:~/qemu/build/arm$
> > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> > /arm/arm/query-cpu-model-expansion: OK
> > /arm/arm/kvm/query-cpu-model-expansion: **
> > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> > assertion failed: (resp_has_props(_resp))
> > Aborted
> >
> > This is asserting on the line:
> > 498 assert_has_not_feature(qts, "host", "sve");
> >
>
> Oh, I see. It's not failing the specific absence of 'sve', but the test
> code (assert_has_not_feature()) is assuming at least one property is
> present. This isn't the case for kvm32 'host' cpus. They apparently
> have none. We need this patch too, then
>
> diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> index 14100ebd8521..9aa728ed8469 100644
> --- a/tests/arm-cpu-features.c
> +++ b/tests/arm-cpu-features.c
> @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> ({ \
> QDict *_resp = do_query_no_props(qts, cpu_type); \
> g_assert(_resp); \
> - g_assert(resp_has_props(_resp)); \
> - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> + g_assert(!resp_has_props(_resp) || \
> + !qdict_get(resp_get_props(_resp), feature)); \
> qobject_unref(_resp); \
> })
Yep, with that extra the test passes. I'm just rerunning the
full 'make check'...
thanks
-- PMM
On Mon, 21 Oct 2019 at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 21 Oct 2019 at 17:12, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> > > On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote:
> > > > Peter, would you mind running your test on the kvm32 machine with this
> > > > change before I send a v7?
> > >
> > > Still fails:
> > >
> > > pm215@pm-ct:~/qemu/build/arm$
> > > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> > > /arm/arm/query-cpu-model-expansion: OK
> > > /arm/arm/kvm/query-cpu-model-expansion: **
> > > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> > > assertion failed: (resp_has_props(_resp))
> > > Aborted
> > >
> > > This is asserting on the line:
> > > 498 assert_has_not_feature(qts, "host", "sve");
> > >
> >
> > Oh, I see. It's not failing the specific absence of 'sve', but the test
> > code (assert_has_not_feature()) is assuming at least one property is
> > present. This isn't the case for kvm32 'host' cpus. They apparently
> > have none. We need this patch too, then
> >
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 14100ebd8521..9aa728ed8469 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> > ({ \
> > QDict *_resp = do_query_no_props(qts, cpu_type); \
> > g_assert(_resp); \
> > - g_assert(resp_has_props(_resp)); \
> > - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> > + g_assert(!resp_has_props(_resp) || \
> > + !qdict_get(resp_get_props(_resp), feature)); \
> > qobject_unref(_resp); \
> > })
>
> Yep, with that extra the test passes. I'm just rerunning the
> full 'make check'...
...which passed OK. For the record, the changes on top of the
patchset were:
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 92668efb8f5..9aa728ed846 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const
char *feature)
({ \
QDict *_resp = do_query_no_props(qts, cpu_type); \
g_assert(_resp); \
- g_assert(resp_has_props(_resp)); \
- g_assert(!qdict_get(resp_get_props(_resp), feature)); \
+ g_assert(!resp_has_props(_resp) || \
+ !qdict_get(resp_get_props(_resp), feature)); \
qobject_unref(_resp); \
})
@@ -417,8 +417,6 @@ static void
test_query_cpu_model_expansion_kvm(const void *data)
qts = qtest_init(MACHINE "-accel kvm -cpu host");
- assert_has_feature(qts, "host", "pmu");
-
if (g_str_equal(qtest_get_arch(), "aarch64")) {
bool kvm_supports_sve;
char max_name[8], name[8];
@@ -428,6 +426,7 @@ static void
test_query_cpu_model_expansion_kvm(const void *data)
char *error;
assert_has_feature(qts, "host", "aarch64");
+ assert_has_feature(qts, "host", "pmu");
assert_error(qts, "cortex-a15",
"We cannot guarantee the CPU type 'cortex-a15' works "
@@ -497,9 +496,6 @@ static void
test_query_cpu_model_expansion_kvm(const void *data)
}
} else {
assert_has_not_feature(qts, "host", "sve");
- assert_error(qts, "host",
- "'pmu' feature not supported by KVM on this host",
- "{ 'pmu': true }");
}
qtest_quit(qts);
thanks
-- PMM
On Tue, Oct 22, 2019 at 11:29:05AM +0100, Peter Maydell wrote:
> On Mon, 21 Oct 2019 at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 21 Oct 2019 at 17:12, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> > > > On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote:
> > > > > Peter, would you mind running your test on the kvm32 machine with this
> > > > > change before I send a v7?
> > > >
> > > > Still fails:
> > > >
> > > > pm215@pm-ct:~/qemu/build/arm$
> > > > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> > > > /arm/arm/query-cpu-model-expansion: OK
> > > > /arm/arm/kvm/query-cpu-model-expansion: **
> > > > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> > > > assertion failed: (resp_has_props(_resp))
> > > > Aborted
> > > >
> > > > This is asserting on the line:
> > > > 498 assert_has_not_feature(qts, "host", "sve");
> > > >
> > >
> > > Oh, I see. It's not failing the specific absence of 'sve', but the test
> > > code (assert_has_not_feature()) is assuming at least one property is
> > > present. This isn't the case for kvm32 'host' cpus. They apparently
> > > have none. We need this patch too, then
> > >
> > > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > > index 14100ebd8521..9aa728ed8469 100644
> > > --- a/tests/arm-cpu-features.c
> > > +++ b/tests/arm-cpu-features.c
> > > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> > > ({ \
> > > QDict *_resp = do_query_no_props(qts, cpu_type); \
> > > g_assert(_resp); \
> > > - g_assert(resp_has_props(_resp)); \
> > > - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> > > + g_assert(!resp_has_props(_resp) || \
> > > + !qdict_get(resp_get_props(_resp), feature)); \
> > > qobject_unref(_resp); \
> > > })
> >
> > Yep, with that extra the test passes. I'm just rerunning the
> > full 'make check'...
>
> ...which passed OK. For the record, the changes on top of the
> patchset were:
>
> diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> index 92668efb8f5..9aa728ed846 100644
> --- a/tests/arm-cpu-features.c
> +++ b/tests/arm-cpu-features.c
> @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const
> char *feature)
> ({ \
> QDict *_resp = do_query_no_props(qts, cpu_type); \
> g_assert(_resp); \
> - g_assert(resp_has_props(_resp)); \
> - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> + g_assert(!resp_has_props(_resp) || \
> + !qdict_get(resp_get_props(_resp), feature)); \
> qobject_unref(_resp); \
> })
>
> @@ -417,8 +417,6 @@ static void
> test_query_cpu_model_expansion_kvm(const void *data)
>
> qts = qtest_init(MACHINE "-accel kvm -cpu host");
>
> - assert_has_feature(qts, "host", "pmu");
> -
> if (g_str_equal(qtest_get_arch(), "aarch64")) {
> bool kvm_supports_sve;
> char max_name[8], name[8];
> @@ -428,6 +426,7 @@ static void
> test_query_cpu_model_expansion_kvm(const void *data)
> char *error;
>
> assert_has_feature(qts, "host", "aarch64");
> + assert_has_feature(qts, "host", "pmu");
>
> assert_error(qts, "cortex-a15",
> "We cannot guarantee the CPU type 'cortex-a15' works "
> @@ -497,9 +496,6 @@ static void
> test_query_cpu_model_expansion_kvm(const void *data)
> }
> } else {
> assert_has_not_feature(qts, "host", "sve");
> - assert_error(qts, "host",
> - "'pmu' feature not supported by KVM on this host",
> - "{ 'pmu': true }");
> }
>
> qtest_quit(qts);
>
Thanks Peter!
The changes look good to me. If we wanted to, we could also add
assert_has_not_feature(qts, "host", "pmu");
in that kvm32 block where we have the
assert_has_not_feature(qts, "host", "sve");
and we could even add
assert_has_not_feature(qts, "host", "aarch64");
there as well.
If I need to spin a v7, then I'll do that. I could also submit just those
additions as another patch, though, or even just not worry too much about
those cases and not add the tests...
Thanks,
drew
On Tue, Oct 22, 2019 at 03:49:51PM +0200, Andrew Jones wrote:
> On Tue, Oct 22, 2019 at 11:29:05AM +0100, Peter Maydell wrote:
> > On Mon, 21 Oct 2019 at 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Mon, 21 Oct 2019 at 17:12, Andrew Jones <drjones@redhat.com> wrote:
> > > >
> > > > On Mon, Oct 21, 2019 at 04:43:22PM +0100, Peter Maydell wrote:
> > > > > On Mon, 21 Oct 2019 at 15:23, Andrew Jones <drjones@redhat.com> wrote:
> > > > > > Peter, would you mind running your test on the kvm32 machine with this
> > > > > > change before I send a v7?
> > > > >
> > > > > Still fails:
> > > > >
> > > > > pm215@pm-ct:~/qemu/build/arm$
> > > > > QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm tests/arm-cpu-features
> > > > > /arm/arm/query-cpu-model-expansion: OK
> > > > > /arm/arm/kvm/query-cpu-model-expansion: **
> > > > > ERROR:/home/pm215/qemu/tests/arm-cpu-features.c:498:test_query_cpu_model_expansion_kvm:
> > > > > assertion failed: (resp_has_props(_resp))
> > > > > Aborted
> > > > >
> > > > > This is asserting on the line:
> > > > > 498 assert_has_not_feature(qts, "host", "sve");
> > > > >
> > > >
> > > > Oh, I see. It's not failing the specific absence of 'sve', but the test
> > > > code (assert_has_not_feature()) is assuming at least one property is
> > > > present. This isn't the case for kvm32 'host' cpus. They apparently
> > > > have none. We need this patch too, then
> > > >
> > > > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > > > index 14100ebd8521..9aa728ed8469 100644
> > > > --- a/tests/arm-cpu-features.c
> > > > +++ b/tests/arm-cpu-features.c
> > > > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const char *feature)
> > > > ({ \
> > > > QDict *_resp = do_query_no_props(qts, cpu_type); \
> > > > g_assert(_resp); \
> > > > - g_assert(resp_has_props(_resp)); \
> > > > - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> > > > + g_assert(!resp_has_props(_resp) || \
> > > > + !qdict_get(resp_get_props(_resp), feature)); \
> > > > qobject_unref(_resp); \
> > > > })
> > >
> > > Yep, with that extra the test passes. I'm just rerunning the
> > > full 'make check'...
> >
> > ...which passed OK. For the record, the changes on top of the
> > patchset were:
> >
> > diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> > index 92668efb8f5..9aa728ed846 100644
> > --- a/tests/arm-cpu-features.c
> > +++ b/tests/arm-cpu-features.c
> > @@ -136,8 +136,8 @@ static bool resp_get_feature(QDict *resp, const
> > char *feature)
> > ({ \
> > QDict *_resp = do_query_no_props(qts, cpu_type); \
> > g_assert(_resp); \
> > - g_assert(resp_has_props(_resp)); \
> > - g_assert(!qdict_get(resp_get_props(_resp), feature)); \
> > + g_assert(!resp_has_props(_resp) || \
> > + !qdict_get(resp_get_props(_resp), feature)); \
> > qobject_unref(_resp); \
> > })
> >
> > @@ -417,8 +417,6 @@ static void
> > test_query_cpu_model_expansion_kvm(const void *data)
> >
> > qts = qtest_init(MACHINE "-accel kvm -cpu host");
> >
> > - assert_has_feature(qts, "host", "pmu");
> > -
> > if (g_str_equal(qtest_get_arch(), "aarch64")) {
> > bool kvm_supports_sve;
> > char max_name[8], name[8];
> > @@ -428,6 +426,7 @@ static void
> > test_query_cpu_model_expansion_kvm(const void *data)
> > char *error;
> >
> > assert_has_feature(qts, "host", "aarch64");
> > + assert_has_feature(qts, "host", "pmu");
> >
> > assert_error(qts, "cortex-a15",
> > "We cannot guarantee the CPU type 'cortex-a15' works "
> > @@ -497,9 +496,6 @@ static void
> > test_query_cpu_model_expansion_kvm(const void *data)
> > }
> > } else {
> > assert_has_not_feature(qts, "host", "sve");
> > - assert_error(qts, "host",
> > - "'pmu' feature not supported by KVM on this host",
> > - "{ 'pmu': true }");
> > }
> >
> > qtest_quit(qts);
> >
>
> Thanks Peter!
>
> The changes look good to me. If we wanted to, we could also add
>
> assert_has_not_feature(qts, "host", "pmu");
>
> in that kvm32 block where we have the
>
> assert_has_not_feature(qts, "host", "sve");
>
> and we could even add
>
> assert_has_not_feature(qts, "host", "aarch64");
>
> there as well.
>
> If I need to spin a v7, then I'll do that. I could also submit just those
> additions as another patch, though, or even just not worry too much about
> those cases and not add the tests...
>
I've decided to send a v7 because the changes above generate several
collisions, I want to add those two has-not tests, and because I have
more tags to pick up. I'll be sending it shortly.
Thanks,
drew
© 2016 - 2024 Red Hat, Inc.