Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early

[Akihiko Odaki]

On 2024/07/19 21:21, Cornelia Huck wrote:
> On Fri, Jul 19 2024, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> > On 2024/07/18 21:07, Peter Maydell wrote:
> > > On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > > > kvm_arm_get_host_cpu_features() used to add the PMU feature
> > > > unconditionally, and kvm_arch_init_vcpu() removed it when it is actually
> > > > not available. Conditionally add the PMU feature in
> > > > kvm_arm_get_host_cpu_features() to save code.
> > > >
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > ---
> > > >    target/arm/kvm.c | 7 +------
> > > >    1 file changed, 1 insertion(+), 6 deletions(-)
> > > >
> > > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > > > index 70f79eda33cd..849e2e21b304 100644
> > > > --- a/target/arm/kvm.c
> > > > +++ b/target/arm/kvm.c
> > > > @@ -280,6 +280,7 @@ static bool 
> > > > kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > > >        if (kvm_arm_pmu_supported()) {
> > > >            init.features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> > > >            pmu_supported = true;
> > > > +        features |= 1ULL << ARM_FEATURE_PMU;
> > > >        }
> > > >
> > > >        if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
> > > > @@ -448,7 +449,6 @@ static bool 
> > > > kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > > >        features |= 1ULL << ARM_FEATURE_V8;
> > > >        features |= 1ULL << ARM_FEATURE_NEON;
> > > >        features |= 1ULL << ARM_FEATURE_AARCH64;
> > > > -    features |= 1ULL << ARM_FEATURE_PMU;
> > > >        features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> > > >
> > > >        ahcf->features = features;
> > > > @@ -1888,13 +1888,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > > >        if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
> > > >            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
> > > >        }
> > > > -    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
> > > > -        cpu->has_pmu = false;
> > > > -    }
> > > >        if (cpu->has_pmu) {
> > > >            cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
> > > > -    } else {
> > > > -        env->features &= ~(1ULL << ARM_FEATURE_PMU);
> > > >        }
> > > >        if (cpu_isar_feature(aa64_sve, cpu)) {
> > > >            assert(kvm_arm_sve_supported());
> > >
> > > Not every KVM CPU is necessarily the "host" CPU type.
> > > The "cortex-a57" and "cortex-a53" CPU types will work if you
> > > happen to be on a host of that CPU type, and they don't go
> > > through kvm_arm_get_host_cpu_features().
> >
> > kvm_arm_vcpu_init() will emit an error in such a situation and I think
> > it's better than silently removing a feature that the requested CPU type
> > has. A user can still disable the feature if desired.
>
> OTOH, if we fail for the named cpu models if the kernel does not provide
> the cap, but silently disable for the host cpu model in that case, that
> also seems inconsistent. I'd rather keep it as it is now.

There are two perspectives of consistency:
1) The initial value of pmu
2) The behavior with the pmu value

This change introduces inconsistency for 1); the host cpu model will 
have pmu=off by default and the other cpu models will keep default 
pmu=on value on a system that does not support PMU. It still keeps 
consistency for 2); it fails if the user sets pmu=on for any cpu model 
on such a system.

We should align 1) for better consistency, but I don't think such a 
change would be useful. It is likely that something is wrong with the 
system when the system reports a cpu model but it doesn't support its 
feature. I think that is the reason why we assert 
kvm_arm_sve_supported() for SVE; however I don't think such an assertion 
would help either because kvm_arm_vcpu_init() will fail anyway.

Regards,
Akihiko Odaki