Re: [PATCH] KVM: x86/svm: Add module param to control PMU virtualization

[Like Xu <like.xu.linux@gmail.com>]

On 15/1/2022 9:26 am, Jim Mattson wrote:
> On Thu, Nov 18, 2021 at 5:25 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 11/17/21 09:03, Like Xu wrote:
>>> From: Like Xu <likexu@tencent.com>
>>>
>>> For Intel, the guest PMU can be disabled via clearing the PMU CPUID.
>>> For AMD, all hw implementations support the base set of four
>>> performance counters, with current mainstream hardware indicating
>>> the presence of two additional counters via X86_FEATURE_PERFCTR_CORE.
>>>
>>> In the virtualized world, the AMD guest driver may detect
>>> the presence of at least one counter MSR. Most hypervisor
>>> vendors would introduce a module param (like lbrv for svm)
>>> to disable PMU for all guests.
>>>
>>> Another control proposal per-VM is to pass PMU disable information
>>> via MSR_IA32_PERF_CAPABILITIES or one bit in CPUID Fn4000_00[FF:00].
>>> Both of methods require some guest-side changes, so a module
>>> parameter may not be sufficiently granular, but practical enough.
>>>
>>> Signed-off-by: Like Xu <likexu@tencent.com>
>>> ---
>>>    arch/x86/kvm/cpuid.c   |  2 +-
>>>    arch/x86/kvm/svm/pmu.c |  4 ++++
>>>    arch/x86/kvm/svm/svm.c | 11 +++++++++++
>>>    arch/x86/kvm/svm/svm.h |  1 +
>>>    4 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 2d70edb0f323..647af2a184ad 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -487,7 +487,7 @@ void kvm_set_cpu_caps(void)
>>>                F(CR8_LEGACY) | F(ABM) | F(SSE4A) | F(MISALIGNSSE) |
>>>                F(3DNOWPREFETCH) | F(OSVW) | 0 /* IBS */ | F(XOP) |
>>>                0 /* SKINIT, WDT, LWP */ | F(FMA4) | F(TBM) |
>>> -             F(TOPOEXT) | F(PERFCTR_CORE)
>>> +             F(TOPOEXT) | 0 /* PERFCTR_CORE */
>>>        );
>>>
>>>        kvm_cpu_cap_mask(CPUID_8000_0001_EDX,
>>> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
>>> index fdf587f19c5f..a0bcf0144664 100644
>>> --- a/arch/x86/kvm/svm/pmu.c
>>> +++ b/arch/x86/kvm/svm/pmu.c
>>> @@ -16,6 +16,7 @@
>>>    #include "cpuid.h"
>>>    #include "lapic.h"
>>>    #include "pmu.h"
>>> +#include "svm.h"
>>>
>>>    enum pmu_type {
>>>        PMU_TYPE_COUNTER = 0,
>>> @@ -100,6 +101,9 @@ static inline struct kvm_pmc *get_gp_pmc_amd(struct kvm_pmu *pmu, u32 msr,
>>>    {
>>>        struct kvm_vcpu *vcpu = pmu_to_vcpu(pmu);
>>>
>>> +     if (!pmuv)
>>> +             return NULL;
>>> +
>>>        switch (msr) {
>>>        case MSR_F15H_PERF_CTL0:
>>>        case MSR_F15H_PERF_CTL1:
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 21bb81710e0f..062e48c191ee 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -190,6 +190,10 @@ module_param(vgif, int, 0444);
>>>    static int lbrv = true;
>>>    module_param(lbrv, int, 0444);
>>>
>>> +/* enable/disable PMU virtualization */
>>> +bool pmuv = true;
>>> +module_param(pmuv, bool, 0444);
>>> +
>>>    static int tsc_scaling = true;
>>>    module_param(tsc_scaling, int, 0444);
>>>
>>> @@ -952,6 +956,10 @@ static __init void svm_set_cpu_caps(void)
>>>            boot_cpu_has(X86_FEATURE_AMD_SSBD))
>>>                kvm_cpu_cap_set(X86_FEATURE_VIRT_SSBD);
>>>
>>> +     /* AMD PMU PERFCTR_CORE CPUID */
>>> +     if (pmuv && boot_cpu_has(X86_FEATURE_PERFCTR_CORE))
>>> +             kvm_cpu_cap_set(X86_FEATURE_PERFCTR_CORE);
>>> +
>>>        /* CPUID 0x8000001F (SME/SEV features) */
>>>        sev_set_cpu_caps();
>>>    }
>>> @@ -1085,6 +1093,9 @@ static __init int svm_hardware_setup(void)
>>>                        pr_info("LBR virtualization supported\n");
>>>        }
>>>
>>> +     if (!pmuv)
>>> +             pr_info("PMU virtualization is disabled\n");
>>> +
>>>        svm_set_cpu_caps();
>>>
>>>        /*
>>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>>> index 0d7bbe548ac3..08e1c19ffbdf 100644
>>> --- a/arch/x86/kvm/svm/svm.h
>>> +++ b/arch/x86/kvm/svm/svm.h
>>> @@ -32,6 +32,7 @@
>>>    extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
>>>    extern bool npt_enabled;
>>>    extern bool intercept_smi;
>>> +extern bool pmuv;
>>>
>>>    /*
>>>     * Clean bits in VMCB.
>>>
>>
>> Queued, thanks - just changed the parameter name to "pmu".
> 
> Whoops! The global 'pmu' is hidden by a local 'pmu' in get_gp_pmc_amd().

Indeed, I wonder if Poalo would like to take a look at this fix:
https://lore.kernel.org/kvm/20220113035324.59572-1-likexu@tencent.com/