| Message ID | 20191101085140.5205-10-peter.maydell@linaro.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | target-arm queue | expand |
On Fri, 1 Nov 2019 at 08:51, Peter Maydell <peter.maydell@linaro.org> wrote: > > From: Andrew Jones <drjones@redhat.com> > > Allow cpu 'host' to enable SVE when it's available, unless the > user chooses to disable it with the added 'sve=off' cpu property. > Also give the user the ability to select vector lengths with the > sve<N> properties. We don't adopt 'max' cpu's other sve property, > sve-max-vq, because that property is difficult to use with KVM. > That property assumes all vector lengths in the range from 1 up > to and including the specified maximum length are supported, but > there may be optional lengths not supported by the host in that > range. With KVM one must be more specific when enabling vector > lengths. Hi; I've been looking at the '-cpu max' vs '-cpu host' code as part of trying to sort out the 'hvf' accelerator doing oddly different things with them. I noticed an oddity introduced in this patch. In the commit message you say that we deliberately leave the 'sve-max-vq' property out of the new aarch64_add_sve_properties(), because it is difficult to use with KVM. But in the code for handling '-cpu max' in aarch64_max_initfn(): > @@ -602,17 +617,11 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name, > static void aarch64_max_initfn(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > - uint32_t vq; > - uint64_t t; > > if (kvm_enabled()) { > kvm_arm_set_cpu_features_from_host(cpu); > - if (kvm_arm_sve_supported(CPU(cpu))) { > - t = cpu->isar.id_aa64pfr0; > - t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); > - cpu->isar.id_aa64pfr0 = t; > - } because this 'if' doesn't exit the function early... > } else { > + uint64_t t; > uint32_t u; > aarch64_a57_initfn(obj); > > @@ -712,17 +721,9 @@ static void aarch64_max_initfn(Object *obj) > #endif > } ...all this code at the tail of the function runs for both KVM and TCG, and it still sets the sve-max-vq property, even for using '-cpu max' with KVM. > - object_property_add(obj, "sve", "bool", cpu_arm_get_sve, > - cpu_arm_set_sve, NULL, NULL, &error_fatal); > + aarch64_add_sve_properties(obj); > object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, > cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal); > - > - for (vq = 1; vq <= ARM_MAX_VQ; ++vq) { > - char name[8]; > - sprintf(name, "sve%d", vq * 128); > - object_property_add(obj, name, "bool", cpu_arm_get_sve_vq, > - cpu_arm_set_sve_vq, NULL, NULL, &error_fatal); > - } > } Was this intentional? I'd like to fix up the weird divergence between -cpu host and -cpu max, either by moving sve-max-vq into aarch64_add_sve_properties() so it's present on both, or by changing the aarch64_max_initfn() so it only adds the property when using TCG. (I think also this code may get the '-cpu max,aarch64=off' case wrong, as it doesn't guard the calls to add the sve and pauth properties with the "if aarch64" feature check.) thanks -- PMM
On Thu, Feb 03, 2022 at 04:46:21PM +0000, Peter Maydell wrote: > On Fri, 1 Nov 2019 at 08:51, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > From: Andrew Jones <drjones@redhat.com> > > > > Allow cpu 'host' to enable SVE when it's available, unless the > > user chooses to disable it with the added 'sve=off' cpu property. > > Also give the user the ability to select vector lengths with the > > sve<N> properties. We don't adopt 'max' cpu's other sve property, > > sve-max-vq, because that property is difficult to use with KVM. > > That property assumes all vector lengths in the range from 1 up > > to and including the specified maximum length are supported, but > > there may be optional lengths not supported by the host in that > > range. With KVM one must be more specific when enabling vector > > lengths. > > Hi; I've been looking at the '-cpu max' vs '-cpu host' code > as part of trying to sort out the 'hvf' accelerator doing > oddly different things with them. I noticed an oddity > introduced in this patch. In the commit message you say that > we deliberately leave the 'sve-max-vq' property out of the > new aarch64_add_sve_properties(), because it is difficult to > use with KVM. But in the code for handling '-cpu max' in > aarch64_max_initfn(): > > > @@ -602,17 +617,11 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name, > > static void aarch64_max_initfn(Object *obj) > > { > > ARMCPU *cpu = ARM_CPU(obj); > > - uint32_t vq; > > - uint64_t t; > > > > if (kvm_enabled()) { > > kvm_arm_set_cpu_features_from_host(cpu); > > - if (kvm_arm_sve_supported(CPU(cpu))) { > > - t = cpu->isar.id_aa64pfr0; > > - t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); > > - cpu->isar.id_aa64pfr0 = t; > > - } > > because this 'if' doesn't exit the function early... > > > } else { > > + uint64_t t; > > uint32_t u; > > aarch64_a57_initfn(obj); > > > > @@ -712,17 +721,9 @@ static void aarch64_max_initfn(Object *obj) > > #endif > > } > > ...all this code at the tail of the function runs for both KVM > and TCG, and it still sets the sve-max-vq property, even for > using '-cpu max' with KVM. > > > - object_property_add(obj, "sve", "bool", cpu_arm_get_sve, > > - cpu_arm_set_sve, NULL, NULL, &error_fatal); > > + aarch64_add_sve_properties(obj); > > object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, > > cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal); > > - > > - for (vq = 1; vq <= ARM_MAX_VQ; ++vq) { > > - char name[8]; > > - sprintf(name, "sve%d", vq * 128); > > - object_property_add(obj, name, "bool", cpu_arm_get_sve_vq, > > - cpu_arm_set_sve_vq, NULL, NULL, &error_fatal); > > - } > > } > > Was this intentional? No, darn. I don't know how many times I rebased that series and was always careful to ensure sve-max-vq was left in the non-kvm part of the above condition. I guess the final rebase finally got me... > > I'd like to fix up the weird divergence between -cpu host and > -cpu max, either by moving sve-max-vq into aarch64_add_sve_properties() > so it's present on both, or by changing the aarch64_max_initfn() so > it only adds the property when using TCG. The later, please. sve-max-vq won't work for any of the machines that support SVE that I know of, so I think it's a bad idea for KVM. > > (I think also this code may get the '-cpu max,aarch64=off' case wrong, > as it doesn't guard the calls to add the sve and pauth properties > with the "if aarch64" feature check.) Yes, but these property dependencies may need to be checked at property finalize time. That means that the properties may get added, but then they will error out if the user tried to enable them. Otherwise, they'll be disabled and the QMP query will inform the user that they cannot be enabled. Thanks, drew
On Thu, 3 Feb 2022 at 17:36, Andrew Jones <drjones@redhat.com> wrote: > > On Thu, Feb 03, 2022 at 04:46:21PM +0000, Peter Maydell wrote: > > Was this intentional? > > No, darn. I don't know how many times I rebased that series and was always > careful to ensure sve-max-vq was left in the non-kvm part of the above > condition. I guess the final rebase finally got me... > > > > > I'd like to fix up the weird divergence between -cpu host and > > -cpu max, either by moving sve-max-vq into aarch64_add_sve_properties() > > so it's present on both, or by changing the aarch64_max_initfn() so > > it only adds the property when using TCG. > > The later, please. sve-max-vq won't work for any of the machines that > support SVE that I know of, so I think it's a bad idea for KVM. > > > > > (I think also this code may get the '-cpu max,aarch64=off' case wrong, > > as it doesn't guard the calls to add the sve and pauth properties > > with the "if aarch64" feature check.) > > Yes, but these property dependencies may need to be checked at property > finalize time. That means that the properties may get added, but then > they will error out if the user tried to enable them. Otherwise, they'll > be disabled and the QMP query will inform the user that they cannot be > enabled. Does 'max' need to do anything different from what we're doing already in arm_host_initfn() for 'host' ? (My proposal for fixing this stuff is basically to make aarch64_max_initfn() start with "if (kvm or hvf) { call arm_host_initfn(); return }".) thanks -- PMM
On Thu, Feb 03, 2022 at 05:40:20PM +0000, Peter Maydell wrote: > On Thu, 3 Feb 2022 at 17:36, Andrew Jones <drjones@redhat.com> wrote: > > > > On Thu, Feb 03, 2022 at 04:46:21PM +0000, Peter Maydell wrote: > > > Was this intentional? > > > > No, darn. I don't know how many times I rebased that series and was always > > careful to ensure sve-max-vq was left in the non-kvm part of the above > > condition. I guess the final rebase finally got me... > > > > > > > > I'd like to fix up the weird divergence between -cpu host and > > > -cpu max, either by moving sve-max-vq into aarch64_add_sve_properties() > > > so it's present on both, or by changing the aarch64_max_initfn() so > > > it only adds the property when using TCG. > > > > The later, please. sve-max-vq won't work for any of the machines that > > support SVE that I know of, so I think it's a bad idea for KVM. > > > > > > > > (I think also this code may get the '-cpu max,aarch64=off' case wrong, > > > as it doesn't guard the calls to add the sve and pauth properties > > > with the "if aarch64" feature check.) > > > > Yes, but these property dependencies may need to be checked at property > > finalize time. That means that the properties may get added, but then > > they will error out if the user tried to enable them. Otherwise, they'll > > be disabled and the QMP query will inform the user that they cannot be > > enabled. > > Does 'max' need to do anything different from what we're doing > already in arm_host_initfn() for 'host' ? > I don't think so. I think we want max+kvm == host and max+kvm to support a strict subset of the properties that max+tcg supports. We could try to provide all of max+tcg's properties to max+kvm, and then let them error out when they bump into KVM limitations, but when we know they will likely never work (e.g. sve-max-vq), then I think I still prefer just removing them. > (My proposal for > fixing this stuff is basically to make aarch64_max_initfn() > start with "if (kvm or hvf) { call arm_host_initfn(); return }".) That sounds good to me. We'll get max+kvm == host that way for sure, and I hope that host supports a strict subset of max's properties already. Thanks, drew
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index a044d6028b6..e1a66a2d1cc 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -977,11 +977,13 @@ int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg); void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq); void aarch64_sve_change_el(CPUARMState *env, int old_el, int new_el, bool el0_a64); +void aarch64_add_sve_properties(Object *obj); #else static inline void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq) { } static inline void aarch64_sve_change_el(CPUARMState *env, int o, int n, bool a) { } +static inline void aarch64_add_sve_properties(Object *obj) { } #endif #if !defined(CONFIG_TCG) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 17d1f2b2894..7a4ac9339bf 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2670,6 +2670,9 @@ static void arm_host_initfn(Object *obj) ARMCPU *cpu = ARM_CPU(obj); kvm_arm_set_cpu_features_from_host(cpu); + if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { + aarch64_add_sve_properties(obj); + } arm_cpu_post_init(obj); } diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index c161a146ff0..68baf0482ff 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -594,6 +594,21 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name, cpu->isar.id_aa64pfr0 = t; } +void aarch64_add_sve_properties(Object *obj) +{ + uint32_t vq; + + object_property_add(obj, "sve", "bool", cpu_arm_get_sve, + cpu_arm_set_sve, NULL, NULL, &error_fatal); + + for (vq = 1; vq <= ARM_MAX_VQ; ++vq) { + char name[8]; + sprintf(name, "sve%d", vq * 128); + object_property_add(obj, name, "bool", cpu_arm_get_sve_vq, + cpu_arm_set_sve_vq, NULL, NULL, &error_fatal); + } +} + /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host); * otherwise, a CPU with as many features enabled as our emulation supports. * The version of '-cpu max' for qemu-system-arm is defined in cpu.c; @@ -602,17 +617,11 @@ static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name, static void aarch64_max_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); - uint32_t vq; - uint64_t t; if (kvm_enabled()) { kvm_arm_set_cpu_features_from_host(cpu); - if (kvm_arm_sve_supported(CPU(cpu))) { - t = cpu->isar.id_aa64pfr0; - t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); - cpu->isar.id_aa64pfr0 = t; - } } else { + uint64_t t; uint32_t u; aarch64_a57_initfn(obj); @@ -712,17 +721,9 @@ static void aarch64_max_initfn(Object *obj) #endif } - object_property_add(obj, "sve", "bool", cpu_arm_get_sve, - cpu_arm_set_sve, NULL, NULL, &error_fatal); + aarch64_add_sve_properties(obj); object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, cpu_max_set_sve_max_vq, NULL, NULL, &error_fatal); - - for (vq = 1; vq <= ARM_MAX_VQ; ++vq) { - char name[8]; - sprintf(name, "sve%d", vq * 128); - object_property_add(obj, name, "bool", cpu_arm_get_sve_vq, - cpu_arm_set_sve_vq, NULL, NULL, &error_fatal); - } } struct ARMCPUInfo { diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index c93bbee425a..876184b8fe4 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -488,7 +488,9 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) * and then query that CPU for the relevant ID registers. */ int fdarray[3]; + bool sve_supported; uint64_t features = 0; + uint64_t t; int err; /* Old kernels may not know about the PREFERRED_TARGET ioctl: however @@ -578,13 +580,23 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) ARM64_SYS_REG(3, 0, 0, 3, 2)); } + sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0; + kvm_arm_destroy_scratch_host_vcpu(fdarray); if (err < 0) { return false; } - /* We can assume any KVM supporting CPU is at least a v8 + /* Add feature bits that can't appear until after VCPU init. */ + if (sve_supported) { + t = ahcf->isar.id_aa64pfr0; + t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1); + ahcf->isar.id_aa64pfr0 = t; + } + + /* + * We can assume any KVM supporting CPU is at least a v8 * with VFPv4+Neon; this in turn implies most of the other * feature bits. */ diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c index 40d8395b7f7..b132ed09806 100644 --- a/tests/arm-cpu-features.c +++ b/tests/arm-cpu-features.c @@ -458,8 +458,8 @@ static void test_query_cpu_model_expansion_kvm(const void *data) "We cannot guarantee the CPU type 'cortex-a15' works " "with KVM on this host", NULL); - assert_has_feature(qts, "max", "sve"); - resp = do_query_no_props(qts, "max"); + assert_has_feature(qts, "host", "sve"); + resp = do_query_no_props(qts, "host"); kvm_supports_sve = resp_get_feature(resp, "sve"); vls = resp_get_sve_vls(resp); qobject_unref(resp); @@ -470,7 +470,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) sprintf(max_name, "sve%d", max_vq * 128); /* Enabling a supported length is of course fine. */ - assert_sve_vls(qts, "max", vls, "{ %s: true }", max_name); + assert_sve_vls(qts, "host", vls, "{ %s: true }", max_name); /* Get the next supported length smaller than max-vq. */ vq = 64 - __builtin_clzll(vls & ~BIT_ULL(max_vq - 1)); @@ -479,7 +479,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) * We have at least one length smaller than max-vq, * so we can disable max-vq. */ - assert_sve_vls(qts, "max", (vls & ~BIT_ULL(max_vq - 1)), + assert_sve_vls(qts, "host", (vls & ~BIT_ULL(max_vq - 1)), "{ %s: false }", max_name); /* @@ -489,7 +489,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) */ sprintf(name, "sve%d", vq * 128); error = g_strdup_printf("cannot disable %s", name); - assert_error(qts, "max", error, + assert_error(qts, "host", error, "{ %s: true, %s: false }", max_name, name); g_free(error); @@ -502,7 +502,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) vq = __builtin_ffsll(vls); sprintf(name, "sve%d", vq * 128); error = g_strdup_printf("cannot disable %s", name); - assert_error(qts, "max", error, "{ %s: false }", name); + assert_error(qts, "host", error, "{ %s: false }", name); g_free(error); /* Get an unsupported length. */ @@ -514,7 +514,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) if (vq <= SVE_MAX_VQ) { sprintf(name, "sve%d", vq * 128); error = g_strdup_printf("cannot enable %s", name); - assert_error(qts, "max", error, "{ %s: true }", name); + assert_error(qts, "host", error, "{ %s: true }", name); g_free(error); } } else { @@ -523,8 +523,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data) } else { assert_has_not_feature(qts, "host", "aarch64"); assert_has_not_feature(qts, "host", "pmu"); - - assert_has_not_feature(qts, "max", "sve"); + assert_has_not_feature(qts, "host", "sve"); } qtest_quit(qts); diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst index bed218d4461..1b367e22e16 100644 --- a/docs/arm-cpu-features.rst +++ b/docs/arm-cpu-features.rst @@ -272,31 +272,36 @@ SVE CPU Property Examples $ qemu-system-aarch64 -M virt -cpu max - 3) Only enable the 128-bit vector length:: + 3) When KVM is enabled, implicitly enable all host CPU supported vector + lengths with the `host` CPU type:: + + $ qemu-system-aarch64 -M virt,accel=kvm -cpu host + + 4) Only enable the 128-bit vector length:: $ qemu-system-aarch64 -M virt -cpu max,sve128=on - 4) Disable the 512-bit vector length and all larger vector lengths, + 5) Disable the 512-bit vector length and all larger vector lengths, since 512 is a power-of-two. This results in all the smaller, uninitialized lengths (128, 256, and 384) defaulting to enabled:: $ qemu-system-aarch64 -M virt -cpu max,sve512=off - 5) Enable the 128-bit, 256-bit, and 512-bit vector lengths:: + 6) Enable the 128-bit, 256-bit, and 512-bit vector lengths:: $ qemu-system-aarch64 -M virt -cpu max,sve128=on,sve256=on,sve512=on - 6) The same as (5), but since the 128-bit and 256-bit vector + 7) The same as (6), but since the 128-bit and 256-bit vector lengths are required for the 512-bit vector length to be enabled, then allow them to be auto-enabled:: $ qemu-system-aarch64 -M virt -cpu max,sve512=on - 7) Do the same as (6), but by first disabling SVE and then re-enabling it:: + 8) Do the same as (7), but by first disabling SVE and then re-enabling it:: $ qemu-system-aarch64 -M virt -cpu max,sve=off,sve512=on,sve=on - 8) Force errors regarding the last vector length:: + 9) Force errors regarding the last vector length:: $ qemu-system-aarch64 -M virt -cpu max,sve128=off $ qemu-system-aarch64 -M virt -cpu max,sve=off,sve128=off,sve=on @@ -308,5 +313,5 @@ The examples in "SVE CPU Property Examples" exhibit many ways to select vector lengths which developers may find useful in order to avoid overly verbose command lines. However, the recommended way to select vector lengths is to explicitly enable each desired length. Therefore only -example's (1), (3), and (5) exhibit recommended uses of the properties. +example's (1), (4), and (6) exhibit recommended uses of the properties.