* [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
@ 2020-06-17 13:08 Philippe Mathieu-Daudé
2020-06-17 13:29 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-17 13:08 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Andrew Jones, kvm, qemu-arm, Haibo Xu,
Paolo Bonzini, Philippe Mathieu-Daudé
Since commit d70c996df23f, when enabling the PMU we get:
$ qemu-system-aarch64 -cpu host,pmu=on -M virt,accel=kvm,gic-version=3
Segmentation fault (core dumped)
Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
2588 ret = ioctl(s->fd, type, arg);
(gdb) bt
#0 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
#1 0x0000aaaaaae31568 in kvm_check_extension (s=0x0, extension=126) at accel/kvm/kvm-all.c:916
#2 0x0000aaaaaafce254 in kvm_arm_pmu_supported (cpu=0xaaaaac214ab0) at target/arm/kvm.c:213
#3 0x0000aaaaaafc0f94 in arm_set_pmu (obj=0xaaaaac214ab0, value=true, errp=0xffffffffe438) at target/arm/cpu.c:1111
#4 0x0000aaaaab5533ac in property_set_bool (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", opaque=0xaaaaac222730, errp=0xffffffffe438) at qom/object.c:2170
#5 0x0000aaaaab5512f0 in object_property_set (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1328
#6 0x0000aaaaab551e10 in object_property_parse (obj=0xaaaaac214ab0, string=0xaaaaac11b4c0 "on", name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1561
#7 0x0000aaaaab54ee8c in object_apply_global_props (obj=0xaaaaac214ab0, props=0xaaaaac018e20, errp=0xaaaaabd6fd88 <error_fatal>) at qom/object.c:407
#8 0x0000aaaaab1dd5a4 in qdev_prop_set_globals (dev=0xaaaaac214ab0) at hw/core/qdev-properties.c:1218
#9 0x0000aaaaab1d9fac in device_post_init (obj=0xaaaaac214ab0) at hw/core/qdev.c:1050
...
#15 0x0000aaaaab54f310 in object_initialize_with_type (obj=0xaaaaac214ab0, size=52208, type=0xaaaaabe237f0) at qom/object.c:512
#16 0x0000aaaaab54fa24 in object_new_with_type (type=0xaaaaabe237f0) at qom/object.c:687
#17 0x0000aaaaab54fa80 in object_new (typename=0xaaaaabe23970 "host-arm-cpu") at qom/object.c:702
#18 0x0000aaaaaaf04a74 in machvirt_init (machine=0xaaaaac0a8550) at hw/arm/virt.c:1770
#19 0x0000aaaaab1e8720 in machine_run_board_init (machine=0xaaaaac0a8550) at hw/core/machine.c:1138
#20 0x0000aaaaaaf95394 in qemu_init (argc=5, argv=0xffffffffea58, envp=0xffffffffea88) at softmmu/vl.c:4348
#21 0x0000aaaaaada3f74 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at softmmu/main.c:48
This is because in frame #2, cpu->kvm_state is still NULL
(the vCPU is not yet realized).
KVM has a hard requirement of all cores supporting the same
feature set. We only need to check if the accelerator supports
a feature, not each vCPU individually.
Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
argument (already realized/valid at this point) instead of a
CPUState argument.
Reported-by: Haibo Xu <haibo.xu@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
target/arm/kvm_arm.h | 25 +++++++++++++------------
target/arm/cpu.c | 2 +-
target/arm/cpu64.c | 10 +++++-----
target/arm/kvm.c | 4 ++--
target/arm/kvm64.c | 14 +++++---------
5 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 48bf5e16d5..8209525f20 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -12,6 +12,7 @@
#define QEMU_KVM_ARM_H
#include "sysemu/kvm.h"
+#include "sysemu/accel.h"
#include "exec/memory.h"
#include "qemu/error-report.h"
@@ -269,29 +270,29 @@ void kvm_arm_add_vcpu_properties(Object *obj);
/**
* kvm_arm_aarch32_supported:
- * @cs: CPUState
+ * @as: AccelState
*
- * Returns: true if the KVM VCPU can enable AArch32 mode
+ * Returns: true if the KVM accelerator can enable AArch32 mode
* and false otherwise.
*/
-bool kvm_arm_aarch32_supported(CPUState *cs);
+bool kvm_arm_aarch32_supported(AccelState *as);
/**
* kvm_arm_pmu_supported:
- * @cs: CPUState
+ * @as: AccelState
*
- * Returns: true if the KVM VCPU can enable its PMU
+ * Returns: true if the KVM accelerator can enable its PMU
* and false otherwise.
*/
-bool kvm_arm_pmu_supported(CPUState *cs);
+bool kvm_arm_pmu_supported(AccelState *as);
/**
* kvm_arm_sve_supported:
- * @cs: CPUState
+ * @as: AccelState
*
- * Returns true if the KVM VCPU can enable SVE and false otherwise.
+ * Returns true if the KVM accelerator can enable SVE and false otherwise.
*/
-bool kvm_arm_sve_supported(CPUState *cs);
+bool kvm_arm_sve_supported(AccelState *as);
/**
* kvm_arm_get_max_vm_ipa_size:
@@ -359,17 +360,17 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
-static inline bool kvm_arm_aarch32_supported(CPUState *cs)
+static inline bool kvm_arm_aarch32_supported(AccelState *as)
{
return false;
}
-static inline bool kvm_arm_pmu_supported(CPUState *cs)
+static inline bool kvm_arm_pmu_supported(AccelState *as)
{
return false;
}
-static inline bool kvm_arm_sve_supported(CPUState *cs)
+static inline bool kvm_arm_sve_supported(AccelState *as)
{
return false;
}
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5b7a36b5d7..29b314427c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1108,7 +1108,7 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
ARMCPU *cpu = ARM_CPU(obj);
if (value) {
- if (kvm_enabled() && !kvm_arm_pmu_supported(CPU(cpu))) {
+ if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
error_setg(errp, "'pmu' feature not supported by KVM on this host");
return;
}
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 778cecc2e6..13835768ab 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -266,7 +266,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
/* Collect the set of vector lengths supported by KVM. */
bitmap_zero(kvm_supported, ARM_MAX_VQ);
- if (kvm_enabled() && kvm_arm_sve_supported(CPU(cpu))) {
+ if (kvm_enabled() && kvm_arm_sve_supported(current_accel())) {
kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
} else if (kvm_enabled()) {
assert(!cpu_isar_feature(aa64_sve, cpu));
@@ -473,7 +473,7 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
return;
}
- if (kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
+ if (kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
error_setg(errp, "cannot set sve-max-vq");
error_append_hint(errp, "SVE not supported by KVM on this host\n");
return;
@@ -519,7 +519,7 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, const char *name,
return;
}
- if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
+ if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
error_setg(errp, "cannot enable %s", name);
error_append_hint(errp, "SVE not supported by KVM on this host\n");
return;
@@ -556,7 +556,7 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
return;
}
- if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
+ if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
error_setg(errp, "'sve' feature not supported by KVM on this host");
return;
}
@@ -751,7 +751,7 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
* uniform execution state like do_interrupt.
*/
if (value == false) {
- if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
+ if (!kvm_enabled() || !kvm_arm_aarch32_supported(current_accel())) {
error_setg(errp, "'aarch64' feature cannot be disabled "
"unless KVM is enabled and 32-bit EL1 "
"is supported");
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index eef3bbd1cc..2247a96757 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -208,9 +208,9 @@ void kvm_arm_add_vcpu_properties(Object *obj)
}
}
-bool kvm_arm_pmu_supported(CPUState *cpu)
+bool kvm_arm_pmu_supported(AccelState *as)
{
- return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3);
+ return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_PMU_V3);
}
int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index f09ed9f4df..ae4e37ce78 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -652,18 +652,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
return true;
}
-bool kvm_arm_aarch32_supported(CPUState *cpu)
+bool kvm_arm_aarch32_supported(AccelState *as)
{
- KVMState *s = KVM_STATE(current_accel());
-
- return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
+ return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_EL1_32BIT);
}
-bool kvm_arm_sve_supported(CPUState *cpu)
+bool kvm_arm_sve_supported(AccelState *as)
{
- KVMState *s = KVM_STATE(current_accel());
-
- return kvm_check_extension(s, KVM_CAP_ARM_SVE);
+ return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_SVE);
}
QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
@@ -798,7 +794,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
env->features &= ~(1ULL << ARM_FEATURE_PMU);
}
if (cpu_isar_feature(aa64_sve, cpu)) {
- assert(kvm_arm_sve_supported(cs));
+ assert(kvm_arm_sve_supported(ACCEL(cs->kvm_state)));
cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-17 13:08 [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU) Philippe Mathieu-Daudé
@ 2020-06-17 13:29 ` no-reply
2020-06-17 15:23 ` Andrew Jones
2020-06-18 9:22 ` Andrew Jones
2 siblings, 0 replies; 9+ messages in thread
From: no-reply @ 2020-06-17 13:29 UTC (permalink / raw)
To: philmd
Cc: peter.maydell, drjones, kvm, qemu-devel, qemu-arm, haibo.xu,
pbonzini, philmd
Patchew URL: https://patchew.org/QEMU/20200617130800.26355-1-philmd@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC qga/guest-agent-command-state.o
CC qga/main.o
CC qga/commands-posix.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
CC qga/channel-posix.o
CC qga/qapi-generated/qga-qapi-types.o
CC qga/qapi-generated/qga-qapi-visit.o
---
GEN docs/interop/qemu-ga-ref.html
GEN docs/interop/qemu-ga-ref.txt
GEN docs/interop/qemu-ga-ref.7
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-keymap
LINK ivshmem-client
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK ivshmem-server
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-nbd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-storage-daemon
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/multiboot.o
AS pc-bios/optionrom/linuxboot.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-img
CC pc-bios/optionrom/linuxboot_dma.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/kvmvapic.o
LINK qemu-io
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-edid
AS pc-bios/optionrom/pvh.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK fsdev/virtfs-proxy-helper
CC pc-bios/optionrom/pvh_main.o
LINK scsi/qemu-pr-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
BUILD pc-bios/optionrom/multiboot.img
BUILD pc-bios/optionrom/linuxboot.img
LINK qemu-bridge-helper
---
BUILD pc-bios/optionrom/linuxboot.raw
BUILD pc-bios/optionrom/linuxboot_dma.img
LINK virtiofsd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
SIGN pc-bios/optionrom/multiboot.bin
SIGN pc-bios/optionrom/linuxboot.bin
BUILD pc-bios/optionrom/linuxboot_dma.raw
---
BUILD pc-bios/optionrom/pvh.img
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
BUILD pc-bios/optionrom/kvmvapic.img
BUILD pc-bios/optionrom/kvmvapic.raw
SIGN pc-bios/optionrom/kvmvapic.bin
LINK qemu-ga
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN x86_64-softmmu/config-target.h
GEN x86_64-softmmu/hmp-commands.h
GEN x86_64-softmmu/hmp-commands-info.h
---
CC x86_64-softmmu/hw/virtio/virtio.o
CC x86_64-softmmu/hw/virtio/vhost.o
CC x86_64-softmmu/hw/virtio/vhost-backend.o
/tmp/qemu-test/src/fpu/softfloat.c:3365:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3423:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~ ( ( (uint64_t) ( absZ1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3483:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~(((uint64_t)(absZ1<<1) == 0) & roundNearestEven);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3606:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3760:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x200 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3987:21: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4003:22: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig0 &= ~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4273:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig1 &= ~ ( ( zSig2 + zSig2 == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
8 errors generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: fpu/softfloat.o] Error 1
make[1]: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/migration/ram.c:919:45: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
xbzrle_counters.encoding_rate = UINT64_MAX;
~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make: *** [Makefile:527: x86_64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=adc833a3dd9f4aaa9d9d130b75d0b794', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-hngiejzd/src/docker-src.2020-06-17-09.25.40.24507:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=adc833a3dd9f4aaa9d9d130b75d0b794
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-hngiejzd/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 4m1.214s
user 0m8.553s
The full log is available at
http://patchew.org/logs/20200617130800.26355-1-philmd@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-17 13:08 [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU) Philippe Mathieu-Daudé
2020-06-17 13:29 ` no-reply
@ 2020-06-17 15:23 ` Andrew Jones
2020-06-17 17:37 ` Paolo Bonzini
2020-06-18 9:22 ` Andrew Jones
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-06-17 15:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, kvm, qemu-devel, qemu-arm, Haibo Xu, Paolo Bonzini
On Wed, Jun 17, 2020 at 03:08:00PM +0200, Philippe Mathieu-Daudé wrote:
> Since commit d70c996df23f, when enabling the PMU we get:
>
> $ qemu-system-aarch64 -cpu host,pmu=on -M virt,accel=kvm,gic-version=3
> Segmentation fault (core dumped)
>
> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
> 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
> 2588 ret = ioctl(s->fd, type, arg);
> (gdb) bt
> #0 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
> #1 0x0000aaaaaae31568 in kvm_check_extension (s=0x0, extension=126) at accel/kvm/kvm-all.c:916
> #2 0x0000aaaaaafce254 in kvm_arm_pmu_supported (cpu=0xaaaaac214ab0) at target/arm/kvm.c:213
> #3 0x0000aaaaaafc0f94 in arm_set_pmu (obj=0xaaaaac214ab0, value=true, errp=0xffffffffe438) at target/arm/cpu.c:1111
> #4 0x0000aaaaab5533ac in property_set_bool (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", opaque=0xaaaaac222730, errp=0xffffffffe438) at qom/object.c:2170
> #5 0x0000aaaaab5512f0 in object_property_set (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1328
> #6 0x0000aaaaab551e10 in object_property_parse (obj=0xaaaaac214ab0, string=0xaaaaac11b4c0 "on", name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1561
> #7 0x0000aaaaab54ee8c in object_apply_global_props (obj=0xaaaaac214ab0, props=0xaaaaac018e20, errp=0xaaaaabd6fd88 <error_fatal>) at qom/object.c:407
> #8 0x0000aaaaab1dd5a4 in qdev_prop_set_globals (dev=0xaaaaac214ab0) at hw/core/qdev-properties.c:1218
> #9 0x0000aaaaab1d9fac in device_post_init (obj=0xaaaaac214ab0) at hw/core/qdev.c:1050
> ...
> #15 0x0000aaaaab54f310 in object_initialize_with_type (obj=0xaaaaac214ab0, size=52208, type=0xaaaaabe237f0) at qom/object.c:512
> #16 0x0000aaaaab54fa24 in object_new_with_type (type=0xaaaaabe237f0) at qom/object.c:687
> #17 0x0000aaaaab54fa80 in object_new (typename=0xaaaaabe23970 "host-arm-cpu") at qom/object.c:702
> #18 0x0000aaaaaaf04a74 in machvirt_init (machine=0xaaaaac0a8550) at hw/arm/virt.c:1770
> #19 0x0000aaaaab1e8720 in machine_run_board_init (machine=0xaaaaac0a8550) at hw/core/machine.c:1138
> #20 0x0000aaaaaaf95394 in qemu_init (argc=5, argv=0xffffffffea58, envp=0xffffffffea88) at softmmu/vl.c:4348
> #21 0x0000aaaaaada3f74 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at softmmu/main.c:48
>
> This is because in frame #2, cpu->kvm_state is still NULL
> (the vCPU is not yet realized).
>
> KVM has a hard requirement of all cores supporting the same
> feature set. We only need to check if the accelerator supports
> a feature, not each vCPU individually.
>
> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
> argument (already realized/valid at this point) instead of a
> CPUState argument.
I'd rather not do that. IMO, a CPU feature test should operate on CPU,
not an "accelerator". How that test is implemented is another story.
If the CPUState isn't interesting, but it points to something that is,
or there's another function that uses globals to get the job done, then
fine, but the callers of a CPU feature test shouldn't need to know that.
I think we should just revert d70c996df23f and then apply the same
change to kvm_arm_pmu_supported() that other similar functions got
with 4f7f589381d5.
Thanks,
drew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-17 15:23 ` Andrew Jones
@ 2020-06-17 17:37 ` Paolo Bonzini
2020-06-18 8:57 ` Andrew Jones
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-06-17 17:37 UTC (permalink / raw)
To: Andrew Jones, Philippe Mathieu-Daudé
Cc: Haibo Xu, Peter Maydell, qemu-arm, qemu-devel, kvm
On 17/06/20 17:23, Andrew Jones wrote:
>>
>> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
>> argument (already realized/valid at this point) instead of a
>> CPUState argument.
> I'd rather not do that. IMO, a CPU feature test should operate on CPU,
> not an "accelerator".
If it's a test that the feature is enabled (e.g. via -cpu) then I agree.
For something that ends up as a KVM_CHECK_EXTENSION or KVM_ENABLE_CAP on
the KVM fd, however, I think passing an AccelState is better.
kvm_arm_pmu_supported case is clearly the latter, even the error message
hints at that:
+ if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
error_setg(errp, "'pmu' feature not supported by KVM on this host");
return;
}
but the same is true of kvm_arm_aarch32_supported and kvm_arm_sve_supported.
Applying the change to kvm_arm_pmu_supported as you suggest below would be
a bit of a bandaid because it would not have consistent prototypes. Sp
for Philippe's patch
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks,
Paolo
> How that test is implemented is another story.
> If the CPUState isn't interesting, but it points to something that is,
> or there's another function that uses globals to get the job done, then
> fine, but the callers of a CPU feature test shouldn't need to know that.
>
> I think we should just revert d70c996df23f and then apply the same
> change to kvm_arm_pmu_supported() that other similar functions got
> with 4f7f589381d5.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-17 17:37 ` Paolo Bonzini
@ 2020-06-18 8:57 ` Andrew Jones
2020-06-18 10:59 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-06-18 8:57 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, kvm, qemu-devel, qemu-arm, Haibo Xu,
Philippe Mathieu-Daudé
On Wed, Jun 17, 2020 at 07:37:42PM +0200, Paolo Bonzini wrote:
> On 17/06/20 17:23, Andrew Jones wrote:
> >>
> >> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
> >> argument (already realized/valid at this point) instead of a
> >> CPUState argument.
> > I'd rather not do that. IMO, a CPU feature test should operate on CPU,
> > not an "accelerator".
>
> If it's a test that the feature is enabled (e.g. via -cpu) then I agree.
> For something that ends up as a KVM_CHECK_EXTENSION or KVM_ENABLE_CAP on
> the KVM fd, however, I think passing an AccelState is better.
I can live with that justification as long as we don't support
heterogeneous VCPU configurations. And, if that ever happens, then I
guess we'll be reworking a lot more than just the interface of these
cpu feature probes.
Thanks,
drew
> kvm_arm_pmu_supported case is clearly the latter, even the error message
> hints at that:
>
> + if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
> error_setg(errp, "'pmu' feature not supported by KVM on this host");
> return;
> }
>
> but the same is true of kvm_arm_aarch32_supported and kvm_arm_sve_supported.
>
> Applying the change to kvm_arm_pmu_supported as you suggest below would be
> a bit of a bandaid because it would not have consistent prototypes. Sp
> for Philippe's patch
>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Thanks,
>
> Paolo
>
> > How that test is implemented is another story.
> > If the CPUState isn't interesting, but it points to something that is,
> > or there's another function that uses globals to get the job done, then
> > fine, but the callers of a CPU feature test shouldn't need to know that.
> >
> > I think we should just revert d70c996df23f and then apply the same
> > change to kvm_arm_pmu_supported() that other similar functions got
> > with 4f7f589381d5.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-17 13:08 [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU) Philippe Mathieu-Daudé
2020-06-17 13:29 ` no-reply
2020-06-17 15:23 ` Andrew Jones
@ 2020-06-18 9:22 ` Andrew Jones
2020-06-18 10:17 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-06-18 9:22 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, kvm, qemu-devel, qemu-arm, Haibo Xu, Paolo Bonzini
On Wed, Jun 17, 2020 at 03:08:00PM +0200, Philippe Mathieu-Daudé wrote:
> Since commit d70c996df23f, when enabling the PMU we get:
>
> $ qemu-system-aarch64 -cpu host,pmu=on -M virt,accel=kvm,gic-version=3
> Segmentation fault (core dumped)
>
> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
> 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
> 2588 ret = ioctl(s->fd, type, arg);
> (gdb) bt
> #0 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
> #1 0x0000aaaaaae31568 in kvm_check_extension (s=0x0, extension=126) at accel/kvm/kvm-all.c:916
> #2 0x0000aaaaaafce254 in kvm_arm_pmu_supported (cpu=0xaaaaac214ab0) at target/arm/kvm.c:213
> #3 0x0000aaaaaafc0f94 in arm_set_pmu (obj=0xaaaaac214ab0, value=true, errp=0xffffffffe438) at target/arm/cpu.c:1111
> #4 0x0000aaaaab5533ac in property_set_bool (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", opaque=0xaaaaac222730, errp=0xffffffffe438) at qom/object.c:2170
> #5 0x0000aaaaab5512f0 in object_property_set (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1328
> #6 0x0000aaaaab551e10 in object_property_parse (obj=0xaaaaac214ab0, string=0xaaaaac11b4c0 "on", name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1561
> #7 0x0000aaaaab54ee8c in object_apply_global_props (obj=0xaaaaac214ab0, props=0xaaaaac018e20, errp=0xaaaaabd6fd88 <error_fatal>) at qom/object.c:407
> #8 0x0000aaaaab1dd5a4 in qdev_prop_set_globals (dev=0xaaaaac214ab0) at hw/core/qdev-properties.c:1218
> #9 0x0000aaaaab1d9fac in device_post_init (obj=0xaaaaac214ab0) at hw/core/qdev.c:1050
> ...
> #15 0x0000aaaaab54f310 in object_initialize_with_type (obj=0xaaaaac214ab0, size=52208, type=0xaaaaabe237f0) at qom/object.c:512
> #16 0x0000aaaaab54fa24 in object_new_with_type (type=0xaaaaabe237f0) at qom/object.c:687
> #17 0x0000aaaaab54fa80 in object_new (typename=0xaaaaabe23970 "host-arm-cpu") at qom/object.c:702
> #18 0x0000aaaaaaf04a74 in machvirt_init (machine=0xaaaaac0a8550) at hw/arm/virt.c:1770
> #19 0x0000aaaaab1e8720 in machine_run_board_init (machine=0xaaaaac0a8550) at hw/core/machine.c:1138
> #20 0x0000aaaaaaf95394 in qemu_init (argc=5, argv=0xffffffffea58, envp=0xffffffffea88) at softmmu/vl.c:4348
> #21 0x0000aaaaaada3f74 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at softmmu/main.c:48
>
> This is because in frame #2, cpu->kvm_state is still NULL
> (the vCPU is not yet realized).
>
> KVM has a hard requirement of all cores supporting the same
> feature set. We only need to check if the accelerator supports
> a feature, not each vCPU individually.
>
> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
> argument (already realized/valid at this point) instead of a
> CPUState argument.
>
> Reported-by: Haibo Xu <haibo.xu@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> target/arm/kvm_arm.h | 25 +++++++++++++------------
> target/arm/cpu.c | 2 +-
> target/arm/cpu64.c | 10 +++++-----
> target/arm/kvm.c | 4 ++--
> target/arm/kvm64.c | 14 +++++---------
> 5 files changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 48bf5e16d5..8209525f20 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -12,6 +12,7 @@
> #define QEMU_KVM_ARM_H
>
> #include "sysemu/kvm.h"
> +#include "sysemu/accel.h"
> #include "exec/memory.h"
> #include "qemu/error-report.h"
>
> @@ -269,29 +270,29 @@ void kvm_arm_add_vcpu_properties(Object *obj);
>
> /**
> * kvm_arm_aarch32_supported:
> - * @cs: CPUState
> + * @as: AccelState
> *
> - * Returns: true if the KVM VCPU can enable AArch32 mode
> + * Returns: true if the KVM accelerator can enable AArch32 mode
> * and false otherwise.
> */
> -bool kvm_arm_aarch32_supported(CPUState *cs);
> +bool kvm_arm_aarch32_supported(AccelState *as);
>
> /**
> * kvm_arm_pmu_supported:
> - * @cs: CPUState
> + * @as: AccelState
> *
> - * Returns: true if the KVM VCPU can enable its PMU
> + * Returns: true if the KVM accelerator can enable its PMU
> * and false otherwise.
> */
> -bool kvm_arm_pmu_supported(CPUState *cs);
> +bool kvm_arm_pmu_supported(AccelState *as);
>
> /**
> * kvm_arm_sve_supported:
> - * @cs: CPUState
> + * @as: AccelState
> *
> - * Returns true if the KVM VCPU can enable SVE and false otherwise.
> + * Returns true if the KVM accelerator can enable SVE and false otherwise.
> */
> -bool kvm_arm_sve_supported(CPUState *cs);
> +bool kvm_arm_sve_supported(AccelState *as);
>
> /**
> * kvm_arm_get_max_vm_ipa_size:
> @@ -359,17 +360,17 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>
> static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
>
> -static inline bool kvm_arm_aarch32_supported(CPUState *cs)
> +static inline bool kvm_arm_aarch32_supported(AccelState *as)
> {
> return false;
> }
>
> -static inline bool kvm_arm_pmu_supported(CPUState *cs)
> +static inline bool kvm_arm_pmu_supported(AccelState *as)
> {
> return false;
> }
>
> -static inline bool kvm_arm_sve_supported(CPUState *cs)
> +static inline bool kvm_arm_sve_supported(AccelState *as)
> {
> return false;
> }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5b7a36b5d7..29b314427c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1108,7 +1108,7 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
> ARMCPU *cpu = ARM_CPU(obj);
>
> if (value) {
> - if (kvm_enabled() && !kvm_arm_pmu_supported(CPU(cpu))) {
> + if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
> error_setg(errp, "'pmu' feature not supported by KVM on this host");
> return;
> }
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 778cecc2e6..13835768ab 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -266,7 +266,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>
> /* Collect the set of vector lengths supported by KVM. */
> bitmap_zero(kvm_supported, ARM_MAX_VQ);
> - if (kvm_enabled() && kvm_arm_sve_supported(CPU(cpu))) {
> + if (kvm_enabled() && kvm_arm_sve_supported(current_accel())) {
> kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
> } else if (kvm_enabled()) {
> assert(!cpu_isar_feature(aa64_sve, cpu));
> @@ -473,7 +473,7 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - if (kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> + if (kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
> error_setg(errp, "cannot set sve-max-vq");
> error_append_hint(errp, "SVE not supported by KVM on this host\n");
> return;
> @@ -519,7 +519,7 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> + if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
> error_setg(errp, "cannot enable %s", name);
> error_append_hint(errp, "SVE not supported by KVM on this host\n");
> return;
> @@ -556,7 +556,7 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> return;
> }
>
> - if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
> + if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
> error_setg(errp, "'sve' feature not supported by KVM on this host");
> return;
> }
> @@ -751,7 +751,7 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
> * uniform execution state like do_interrupt.
> */
> if (value == false) {
> - if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(current_accel())) {
> error_setg(errp, "'aarch64' feature cannot be disabled "
> "unless KVM is enabled and 32-bit EL1 "
> "is supported");
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index eef3bbd1cc..2247a96757 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -208,9 +208,9 @@ void kvm_arm_add_vcpu_properties(Object *obj)
> }
> }
>
> -bool kvm_arm_pmu_supported(CPUState *cpu)
> +bool kvm_arm_pmu_supported(AccelState *as)
> {
> - return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3);
> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_PMU_V3);
> }
>
> int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index f09ed9f4df..ae4e37ce78 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -652,18 +652,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> return true;
> }
>
> -bool kvm_arm_aarch32_supported(CPUState *cpu)
> +bool kvm_arm_aarch32_supported(AccelState *as)
> {
> - KVMState *s = KVM_STATE(current_accel());
> -
> - return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_EL1_32BIT);
> }
>
> -bool kvm_arm_sve_supported(CPUState *cpu)
> +bool kvm_arm_sve_supported(AccelState *as)
> {
> - KVMState *s = KVM_STATE(current_accel());
> -
> - return kvm_check_extension(s, KVM_CAP_ARM_SVE);
> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_SVE);
> }
>
> QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
> @@ -798,7 +794,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> env->features &= ~(1ULL << ARM_FEATURE_PMU);
> }
> if (cpu_isar_feature(aa64_sve, cpu)) {
> - assert(kvm_arm_sve_supported(cs));
> + assert(kvm_arm_sve_supported(ACCEL(cs->kvm_state)));
Might as well use current_accel() here too, right?
> cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> }
>
> --
> 2.21.3
>
>
At all callsites we pass current_accel() to the kvm_arm_<feat>_supported()
functions. Is there any reason not to drop their input parameter and just
use current_accel() internally?
Thanks,
drew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-18 9:22 ` Andrew Jones
@ 2020-06-18 10:17 ` Philippe Mathieu-Daudé
2020-06-18 10:59 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-06-18 10:17 UTC (permalink / raw)
To: Andrew Jones
Cc: Peter Maydell, kvm, qemu-devel, qemu-arm, Haibo Xu, Paolo Bonzini
On 6/18/20 11:22 AM, Andrew Jones wrote:
> On Wed, Jun 17, 2020 at 03:08:00PM +0200, Philippe Mathieu-Daudé wrote:
>> Since commit d70c996df23f, when enabling the PMU we get:
>>
>> $ qemu-system-aarch64 -cpu host,pmu=on -M virt,accel=kvm,gic-version=3
>> Segmentation fault (core dumped)
>>
>> Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
>> 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
>> 2588 ret = ioctl(s->fd, type, arg);
>> (gdb) bt
>> #0 0x0000aaaaaae356d0 in kvm_ioctl (s=0x0, type=44547) at accel/kvm/kvm-all.c:2588
>> #1 0x0000aaaaaae31568 in kvm_check_extension (s=0x0, extension=126) at accel/kvm/kvm-all.c:916
>> #2 0x0000aaaaaafce254 in kvm_arm_pmu_supported (cpu=0xaaaaac214ab0) at target/arm/kvm.c:213
>> #3 0x0000aaaaaafc0f94 in arm_set_pmu (obj=0xaaaaac214ab0, value=true, errp=0xffffffffe438) at target/arm/cpu.c:1111
>> #4 0x0000aaaaab5533ac in property_set_bool (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", opaque=0xaaaaac222730, errp=0xffffffffe438) at qom/object.c:2170
>> #5 0x0000aaaaab5512f0 in object_property_set (obj=0xaaaaac214ab0, v=0xaaaaac223a80, name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1328
>> #6 0x0000aaaaab551e10 in object_property_parse (obj=0xaaaaac214ab0, string=0xaaaaac11b4c0 "on", name=0xaaaaac11a970 "pmu", errp=0xffffffffe438) at qom/object.c:1561
>> #7 0x0000aaaaab54ee8c in object_apply_global_props (obj=0xaaaaac214ab0, props=0xaaaaac018e20, errp=0xaaaaabd6fd88 <error_fatal>) at qom/object.c:407
>> #8 0x0000aaaaab1dd5a4 in qdev_prop_set_globals (dev=0xaaaaac214ab0) at hw/core/qdev-properties.c:1218
>> #9 0x0000aaaaab1d9fac in device_post_init (obj=0xaaaaac214ab0) at hw/core/qdev.c:1050
>> ...
>> #15 0x0000aaaaab54f310 in object_initialize_with_type (obj=0xaaaaac214ab0, size=52208, type=0xaaaaabe237f0) at qom/object.c:512
>> #16 0x0000aaaaab54fa24 in object_new_with_type (type=0xaaaaabe237f0) at qom/object.c:687
>> #17 0x0000aaaaab54fa80 in object_new (typename=0xaaaaabe23970 "host-arm-cpu") at qom/object.c:702
>> #18 0x0000aaaaaaf04a74 in machvirt_init (machine=0xaaaaac0a8550) at hw/arm/virt.c:1770
>> #19 0x0000aaaaab1e8720 in machine_run_board_init (machine=0xaaaaac0a8550) at hw/core/machine.c:1138
>> #20 0x0000aaaaaaf95394 in qemu_init (argc=5, argv=0xffffffffea58, envp=0xffffffffea88) at softmmu/vl.c:4348
>> #21 0x0000aaaaaada3f74 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at softmmu/main.c:48
>>
>> This is because in frame #2, cpu->kvm_state is still NULL
>> (the vCPU is not yet realized).
>>
>> KVM has a hard requirement of all cores supporting the same
>> feature set. We only need to check if the accelerator supports
>> a feature, not each vCPU individually.
>>
>> Fix by kvm_arm_<FEATURE>_supported() functions take a AccelState
>> argument (already realized/valid at this point) instead of a
>> CPUState argument.
>>
>> Reported-by: Haibo Xu <haibo.xu@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> target/arm/kvm_arm.h | 25 +++++++++++++------------
>> target/arm/cpu.c | 2 +-
>> target/arm/cpu64.c | 10 +++++-----
>> target/arm/kvm.c | 4 ++--
>> target/arm/kvm64.c | 14 +++++---------
>> 5 files changed, 26 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>> index 48bf5e16d5..8209525f20 100644
>> --- a/target/arm/kvm_arm.h
>> +++ b/target/arm/kvm_arm.h
>> @@ -12,6 +12,7 @@
>> #define QEMU_KVM_ARM_H
>>
>> #include "sysemu/kvm.h"
>> +#include "sysemu/accel.h"
>> #include "exec/memory.h"
>> #include "qemu/error-report.h"
>>
>> @@ -269,29 +270,29 @@ void kvm_arm_add_vcpu_properties(Object *obj);
>>
>> /**
>> * kvm_arm_aarch32_supported:
>> - * @cs: CPUState
>> + * @as: AccelState
>> *
>> - * Returns: true if the KVM VCPU can enable AArch32 mode
>> + * Returns: true if the KVM accelerator can enable AArch32 mode
>> * and false otherwise.
>> */
>> -bool kvm_arm_aarch32_supported(CPUState *cs);
>> +bool kvm_arm_aarch32_supported(AccelState *as);
>>
>> /**
>> * kvm_arm_pmu_supported:
>> - * @cs: CPUState
>> + * @as: AccelState
>> *
>> - * Returns: true if the KVM VCPU can enable its PMU
>> + * Returns: true if the KVM accelerator can enable its PMU
>> * and false otherwise.
>> */
>> -bool kvm_arm_pmu_supported(CPUState *cs);
>> +bool kvm_arm_pmu_supported(AccelState *as);
>>
>> /**
>> * kvm_arm_sve_supported:
>> - * @cs: CPUState
>> + * @as: AccelState
>> *
>> - * Returns true if the KVM VCPU can enable SVE and false otherwise.
>> + * Returns true if the KVM accelerator can enable SVE and false otherwise.
>> */
>> -bool kvm_arm_sve_supported(CPUState *cs);
>> +bool kvm_arm_sve_supported(AccelState *as);
>>
>> /**
>> * kvm_arm_get_max_vm_ipa_size:
>> @@ -359,17 +360,17 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>>
>> static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
>>
>> -static inline bool kvm_arm_aarch32_supported(CPUState *cs)
>> +static inline bool kvm_arm_aarch32_supported(AccelState *as)
>> {
>> return false;
>> }
>>
>> -static inline bool kvm_arm_pmu_supported(CPUState *cs)
>> +static inline bool kvm_arm_pmu_supported(AccelState *as)
>> {
>> return false;
>> }
>>
>> -static inline bool kvm_arm_sve_supported(CPUState *cs)
>> +static inline bool kvm_arm_sve_supported(AccelState *as)
>> {
>> return false;
>> }
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 5b7a36b5d7..29b314427c 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1108,7 +1108,7 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>> ARMCPU *cpu = ARM_CPU(obj);
>>
>> if (value) {
>> - if (kvm_enabled() && !kvm_arm_pmu_supported(CPU(cpu))) {
>> + if (kvm_enabled() && !kvm_arm_pmu_supported(current_accel())) {
>> error_setg(errp, "'pmu' feature not supported by KVM on this host");
>> return;
>> }
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index 778cecc2e6..13835768ab 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -266,7 +266,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
>>
>> /* Collect the set of vector lengths supported by KVM. */
>> bitmap_zero(kvm_supported, ARM_MAX_VQ);
>> - if (kvm_enabled() && kvm_arm_sve_supported(CPU(cpu))) {
>> + if (kvm_enabled() && kvm_arm_sve_supported(current_accel())) {
>> kvm_arm_sve_get_vls(CPU(cpu), kvm_supported);
>> } else if (kvm_enabled()) {
>> assert(!cpu_isar_feature(aa64_sve, cpu));
>> @@ -473,7 +473,7 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>> return;
>> }
>>
>> - if (kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
>> + if (kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
>> error_setg(errp, "cannot set sve-max-vq");
>> error_append_hint(errp, "SVE not supported by KVM on this host\n");
>> return;
>> @@ -519,7 +519,7 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, const char *name,
>> return;
>> }
>>
>> - if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
>> + if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
>> error_setg(errp, "cannot enable %s", name);
>> error_append_hint(errp, "SVE not supported by KVM on this host\n");
>> return;
>> @@ -556,7 +556,7 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
>> return;
>> }
>>
>> - if (value && kvm_enabled() && !kvm_arm_sve_supported(CPU(cpu))) {
>> + if (value && kvm_enabled() && !kvm_arm_sve_supported(current_accel())) {
>> error_setg(errp, "'sve' feature not supported by KVM on this host");
>> return;
>> }
>> @@ -751,7 +751,7 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
>> * uniform execution state like do_interrupt.
>> */
>> if (value == false) {
>> - if (!kvm_enabled() || !kvm_arm_aarch32_supported(CPU(cpu))) {
>> + if (!kvm_enabled() || !kvm_arm_aarch32_supported(current_accel())) {
>> error_setg(errp, "'aarch64' feature cannot be disabled "
>> "unless KVM is enabled and 32-bit EL1 "
>> "is supported");
>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>> index eef3bbd1cc..2247a96757 100644
>> --- a/target/arm/kvm.c
>> +++ b/target/arm/kvm.c
>> @@ -208,9 +208,9 @@ void kvm_arm_add_vcpu_properties(Object *obj)
>> }
>> }
>>
>> -bool kvm_arm_pmu_supported(CPUState *cpu)
>> +bool kvm_arm_pmu_supported(AccelState *as)
>> {
>> - return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3);
>> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_PMU_V3);
>> }
>>
>> int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index f09ed9f4df..ae4e37ce78 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -652,18 +652,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>> return true;
>> }
>>
>> -bool kvm_arm_aarch32_supported(CPUState *cpu)
>> +bool kvm_arm_aarch32_supported(AccelState *as)
>> {
>> - KVMState *s = KVM_STATE(current_accel());
>> -
>> - return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
>> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_EL1_32BIT);
>> }
>>
>> -bool kvm_arm_sve_supported(CPUState *cpu)
>> +bool kvm_arm_sve_supported(AccelState *as)
>> {
>> - KVMState *s = KVM_STATE(current_accel());
>> -
>> - return kvm_check_extension(s, KVM_CAP_ARM_SVE);
>> + return kvm_check_extension(KVM_STATE(as), KVM_CAP_ARM_SVE);
>> }
>>
>> QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
>> @@ -798,7 +794,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> env->features &= ~(1ULL << ARM_FEATURE_PMU);
>> }
>> if (cpu_isar_feature(aa64_sve, cpu)) {
>> - assert(kvm_arm_sve_supported(cs));
>> + assert(kvm_arm_sve_supported(ACCEL(cs->kvm_state)));
>
> Might as well use current_accel() here too, right?
I was not sure, I used kvm_state since at this point we are
sure it is initialized.
>
>> cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>> }
>>
>> --
>> 2.21.3
>>
>>
>
> At all callsites we pass current_accel() to the kvm_arm_<feat>_supported()
> functions. Is there any reason not to drop their input parameter and just
> use current_accel() internally?
Clever idea :)
>
> Thanks,
> drew
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-18 10:17 ` Philippe Mathieu-Daudé
@ 2020-06-18 10:59 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2020-06-18 10:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Andrew Jones
Cc: Haibo Xu, Peter Maydell, qemu-arm, qemu-devel, kvm
On 18/06/20 12:17, Philippe Mathieu-Daudé wrote:
>>> cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
>>> }
>>>
>>> --
>>> 2.21.3
>>>
>>>
>> At all callsites we pass current_accel() to the kvm_arm_<feat>_supported()
>> functions. Is there any reason not to drop their input parameter and just
>> use current_accel() internally?
> Clever idea :)
Or just the kvm_state global.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU)
2020-06-18 8:57 ` Andrew Jones
@ 2020-06-18 10:59 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2020-06-18 10:59 UTC (permalink / raw)
To: Andrew Jones
Cc: Peter Maydell, kvm, qemu-devel, qemu-arm, Haibo Xu,
Philippe Mathieu-Daudé
On 18/06/20 10:57, Andrew Jones wrote:
>> If it's a test that the feature is enabled (e.g. via -cpu) then I agree.
>> For something that ends up as a KVM_CHECK_EXTENSION or KVM_ENABLE_CAP on
>> the KVM fd, however, I think passing an AccelState is better.
> I can live with that justification as long as we don't support
> heterogeneous VCPU configurations. And, if that ever happens, then I
> guess we'll be reworking a lot more than just the interface of these
> cpu feature probes.
Yes, and anyway configuring "what is allowed" would be separate from
checking "what is supported".
Thanks,
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-18 11:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 13:08 [PATCH] target/arm/kvm: Check supported feature per accelerator (not per vCPU) Philippe Mathieu-Daudé
2020-06-17 13:29 ` no-reply
2020-06-17 15:23 ` Andrew Jones
2020-06-17 17:37 ` Paolo Bonzini
2020-06-18 8:57 ` Andrew Jones
2020-06-18 10:59 ` Paolo Bonzini
2020-06-18 9:22 ` Andrew Jones
2020-06-18 10:17 ` Philippe Mathieu-Daudé
2020-06-18 10:59 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).