Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Andrew Jones]

On Tue, Aug 11, 2020 at 11:15:42AM +0800, Haibo Xu wrote:
> > > +    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.

I think the PMU logic needs a cleanup, rather than to be imitated.

> 
> > > +
> > >      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;
> }

Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
register bit instead like "sve_supported" does.

> 
> > >
> > >      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?

At least SPE. You'll have to double check that it makes sense to do for
PMU. But, if so, then it should be done with a separate series.

Thanks,
drew