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

[Haibo Xu]

On Wed, 12 Aug 2020 at 00:50, Andrew Jones <drjones@redhat.com> wrote:
>
> 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.
>

Ok, will adapt the SPE support to this new feature-identification
strategy first, and
investigate whether it makes sense to do so for PMU later.

Thank you very much for helping review the patch series!

> Thanks,
> drew
>