On Mon, 10 Aug 2020 at 19:16, Andrew Jones <drjo...@redhat.com> wrote:
>
> On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote:
> > Turn on the spe cpu property by default when working with host
> > cpu type in KVM mode, i.e. we can now do '-cpu host' to add the
> > vSPE, and '-cpu host,spe=off' to remove it.
>
> -cpu max with KVM should also enable it by default
>
Ok, will fix it!
> >
> > Signed-off-by: Haibo Xu <haibo...@linaro.org>
> > ---
> > target/arm/cpu.c | 4 ++++
> > target/arm/kvm64.c | 9 +++++++++
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 67ab0089fd..42fa99953c 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev,
Error **errp)
> > cpu->pmceid1 = 0;
> > }
> >
> > + if (!cpu->has_spe || !kvm_enabled()) {
> > + unset_feature(env, ARM_FEATURE_SPE);
> > + }
>
> I don't think this should be necessary.
>
Yes, I have tried to remove this check, and the vSPE can still work
correctly.
But I don't know whether there are some corner cases that trigger an error.
The similar logic is added in commit 929e754d5a to enable vPMU support.
> > +
> > if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > /* Disable the hypervisor feature bits in the processor feature
> > * registers if we don't have EL2. These are id_pfr1[15:12] and
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index be045ccc5f..4ea58afc1d 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -679,6 +679,7 @@ bool
kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > features |= 1ULL << ARM_FEATURE_AARCH64;
> > features |= 1ULL << ARM_FEATURE_PMU;
> > features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> > + features |= 1ULL << ARM_FEATURE_SPE;
>
> No, SPE is not a feature we assume is present in v8.0 CPUs.
>
Yes, SPE is an optional feature for v8.2. How about changing to the
following logic:
spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1)
> 0;
if (spe_supported) {
features |= 1ULL << ARM_FEATURE_SPE;
}
> >
> > ahcf->features = features;
> >
> > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > } else {
> > env->features &= ~(1ULL << ARM_FEATURE_PMU);
> > }
> > + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
> > + cpu->has_spe = false;
> > + }
> > + if (cpu->has_spe) {
> > + cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
> > + } else {
> > + env->features &= ~(1ULL << ARM_FEATURE_SPE);
> > + }
>
> The PMU code above this isn't a good pattern to copy. The SVE code below
> is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.
> It'd be nice to cleanup the PMU code (with a separate patch) and then add
> SPE in a better way.
>
I noticed that Peter had sent out a mail
<https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk
about the feature-identification strategy.
So shall we adapt it to the vPMU and vSPE feature?
> > if (cpu_isar_feature(aa64_sve, cpu)) {
> > assert(kvm_arm_sve_supported());
> > cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > --
> > 2.17.1
> >
>
> Thanks,
> drew
>
