On 11/20/2024 1:32 AM, Sean Christopherson wrote:
> On Thu, Aug 01, 2024, Mingwei Zhang wrote:
>> Introduce a vendor specific API to check if rdpmc passthrough allowed.
>> RDPMC passthrough requires guest VM have the full ownership of all
>> counters. These include general purpose counters and fixed counters and
>> some vendor specific MSRs such as PERF_METRICS. Since PERF_METRICS MSR is
>> Intel specific, putting the check into vendor specific code.
>>
>> Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
>> Tested-by: Yongwei Ma <yongwei.ma@xxxxxxxxx>
>> ---
>> arch/x86/include/asm/kvm-x86-pmu-ops.h | 1 +
>> arch/x86/kvm/pmu.c | 1 +
>> arch/x86/kvm/pmu.h | 1 +
>> arch/x86/kvm/svm/pmu.c | 6 ++++++
>> arch/x86/kvm/vmx/pmu_intel.c | 16 ++++++++++++++++
>> 5 files changed, 25 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/kvm-x86-pmu-ops.h b/arch/x86/include/asm/kvm-x86-pmu-ops.h
>> index f852b13aeefe..fd986d5146e4 100644
>> --- a/arch/x86/include/asm/kvm-x86-pmu-ops.h
>> +++ b/arch/x86/include/asm/kvm-x86-pmu-ops.h
>> @@ -20,6 +20,7 @@ KVM_X86_PMU_OP(get_msr)
>> KVM_X86_PMU_OP(set_msr)
>> KVM_X86_PMU_OP(refresh)
>> KVM_X86_PMU_OP(init)
>> +KVM_X86_PMU_OP(is_rdpmc_passthru_allowed)
>> KVM_X86_PMU_OP_OPTIONAL(reset)
>> KVM_X86_PMU_OP_OPTIONAL(deliver_pmi)
>> KVM_X86_PMU_OP_OPTIONAL(cleanup)
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index 19104e16a986..3afefe4cf6e2 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -102,6 +102,7 @@ bool kvm_pmu_check_rdpmc_passthrough(struct kvm_vcpu *vcpu)
>>
>> if (is_passthrough_pmu_enabled(vcpu) &&
>> !enable_vmware_backdoor &&
>> + static_call(kvm_x86_pmu_is_rdpmc_passthru_allowed)(vcpu) &&
> If the polarity is inverted, the callback can be OPTIONAL_RET0 on AMD. E.g.
>
> if (kvm_pmu_call(rdpmc_needs_intercept(vcpu)))
> return false;
>
>> +static bool intel_is_rdpmc_passthru_allowed(struct kvm_vcpu *vcpu)
>> +{
>> + /*
>> + * Per Intel SDM vol. 2 for RDPMC,
>
> Please don't reference specific sections in the comments. For changelogs it's
> ok, because changelogs are a snapshot in time. But comments are living things
> and will become stale in almost every case. And I don't see any reason to reference
> the SDM, just state the behavior; it's implied that that's the architectural
> behavior, otherwise KVM is buggy.
>
>> MSR_PERF_METRICS is accessible by
> This is technically wrong, the SDM states that the RDPMC behavior is implementation
> specific. That matters to some extent, because if it was _just_ one MSR and was
> guaranteed to always be that one MSR, it might be worth creating a virtualization
> hole.
>
> /*
> * Intercept RDPMC if the host supports PERF_METRICS, but the guest
> * does not, as RDPMC with type 0x2000 accesses implementation specific
> * metrics.
> */
>
>
> All that said, isn't this redundant with the number of fixed counters? I'm having
> a hell of a time finding anything concrete in the SDM, but IIUC fixed counter 3
> is tightly coupled to perf metrics. E.g. rather than add a vendor hook just for
> this, rely on the fixed counters and refuse to enable the mediated PMU if the
> underlying CPU model is nonsensical, i.e. perf metrics exists without ctr3.
>
> And I kinda think we have to go that route, because enabling RDPMC interception
> based on future features is doomed from the start. E.g. if this code had been
> written prior to PERF_METRICS, older KVMs would have zero clue that RDPMC needs
> to be intercepted on newer hardware.
Yeah, this sounds make sense. Fixed counter 3 and PERF_METRICS are always
coupled as a whole, and the previous code has already checked the fixed
counter bitmap. I think we can drop this patch and just add a comment to
explain the reason.
