Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to adjust features visibility

[Paolo Bonzini]

On 27/01/21 07:04, Like Xu wrote:
> On 2021/1/26 17:42, Paolo Bonzini wrote:
> > On 08/01/21 02:36, Like Xu wrote:
> > > @@ -401,6 +398,9 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
> > >          pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
> > >          pmu->fixed_counters[i].current_config = 0;
> > >      }
> > > +
> > > +    vcpu->arch.perf_capabilities = guest_cpuid_has(vcpu, 
> > > X86_FEATURE_PDCM) ?
> > > +        vmx_get_perf_capabilities() : 0;
> >
> > There is one thing I don't understand with this patch: intel_pmu_init 
> > is not called when CPUID is changed.  So I would have thought that 
> > anything that uses guest_cpuid_has must stay in intel_pmu_refresh.  As 
> > I understand it vcpu->arch.perf_capabilities is always set to 0 
> > (vmx_get_perf_capabilities is never called), and kvm_set_msr_common 
> > would fail to set any bit in the MSR.  What am I missing?
> >
> > In addition, the code of patch 4:
> >
> > +    if (!intel_pmu_lbr_is_enabled(vcpu)) {
> > +        vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
> > +        lbr_desc->records.nr = 0;
> > +    }
> >
> > is not okay after MSR changes.  The value written by the host must be 
> > either rejected (with "return 1") or applied unchanged.
> >
> > Fortunately I think this code is dead if you move the check in 
> > kvm_set_msr from patch 9 to patch 4.  However, in patch 9 
> > vmx_get_perf_capabilities() must only set the LBR format bits if 
> > intel_pmu_lbr_is_compatible(vcpu).
>
> Thanks for the guidance. How about handling it in this way:
>
> In the intel_pmu_init():
>
>      vcpu->arch.perf_capabilities = 0;
>      lbr_desc->records.nr = 0;
>
> In the intel_pmu_refresh():
>
>      if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM)) {
>          vcpu->arch.perf_capabilities = vmx_get_perf_capabilities();
>          if (!lbr_desc->records.nr)
>              vcpu->arch.perf_capabilities &= ~PMU_CAP_LBR_FMT;
>      }
>
> In the vmx_set_msr():
>
>      case MSR_IA32_PERF_CAPABILITIES:
>          // set up lbr_desc->records.nr
>          if (!intel_pmu_lbr_is_compatible(vcpu))

Maybe pass msr_info.data to intel_pmu_lbr_is_compatible?  Otherwise 
seems okay, thanks Like.

Paolo