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;
MIME-Version: 1.0
References: <20211117080304.38989-1-likexu@tencent.com> <c840f1fe-5000-fb45-b5f6-eac15e205995@redhat.com>
In-Reply-To: <c840f1fe-5000-fb45-b5f6-eac15e205995@redhat.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Fri, 14 Jan 2022 17:26:19 -0800
Message-ID: <CALMp9eQCEFsQTbm7F9CqotirbP18OF_cQUySb7Q=dqiuiK1FMg@mail.gmail.com>
Subject: Re: [PATCH] KVM: x86/svm: Add module param to control PMU virtualization
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Like Xu <like.xu.linux@gmail.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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().