Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RESEND v13 03/10] KVM: x86/pmu: Use IA32_PERF_CAPABILITIES to
 adjust features visibility
To:     Like Xu <like.xu@linux.intel.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>, ak@linux.intel.com,
        wei.w.wang@intel.com, kan.liang@intel.com, x86@kernel.org,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Joerg Roedel <joro@8bytes.org>,
        Jim Mattson <jmattson@google.com>,
        Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>
References: <20210108013704.134985-1-like.xu@linux.intel.com>
 <20210108013704.134985-4-like.xu@linux.intel.com>
 <a1291d0b-297c-9146-9689-f4a4129de3c6@redhat.com>
 <fd7df596-9715-e6a8-0040-18aecedb0fae@linux.intel.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <5da57047-db15-adb6-f3bc-6152753d0b49@redhat.com>
Date:   Wed, 27 Jan 2021 10:48:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <fd7df596-9715-e6a8-0040-18aecedb0fae@linux.intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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