Date:   Mon, 30 Jan 2023 17:40:05 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Like Xu <like.xu.linux@gmail.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Subject: Re: [PATCH v3 1/3] KVM: x86/pmu: Disable guest PEBS on hybird cpu
 due to heterogeneity
Message-ID: <Y9gA9aZNxYAZGgPh@google.com>
References: <20221109082802.27543-1-likexu@tencent.com>
 <20221109082802.27543-2-likexu@tencent.com>
 <Y8nknyxfKl4p/0GY@google.com>
 <9b422d58-72ab-051f-e317-02b4d8e7211d@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9b422d58-72ab-051f-e317-02b4d8e7211d@gmail.com>
Precedence: bulk

On Mon, Jan 30, 2023, Like Xu wrote:
> On 20/1/2023 8:47 am, Sean Christopherson wrote:
> > On Wed, Nov 09, 2022, Like Xu wrote:
> > > From: Like Xu <likexu@tencent.com>
> > > 
> > >  From vPMU enabling perspective, KVM does not have proper support for
> > > hybird x86 core. The reported perf_capabilities value (e.g. the format
> > > of pebs record) depends on the type of cpu the kvm-intel module is init.
> > > When a vcpu of one pebs format migrates to a vcpu of another pebs format,
> > > the incorrect parsing of pebs records by guest can make profiling data
> > > analysis extremely problematic.
> > > 
> > > The safe way to fix this is to disable this part of the support until the
> > > guest recognizes that it is running on the hybird cpu, which is appropriate
> > > at the moment given that x86 hybrid architectures are not heavily touted
> > > in the data center market.
> > > 
> > > Signed-off-by: Like Xu <likexu@tencent.com>
> > > ---
> > >   arch/x86/kvm/vmx/capabilities.h | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> > > index cd2ac9536c99..ea0498684048 100644
> > > --- a/arch/x86/kvm/vmx/capabilities.h
> > > +++ b/arch/x86/kvm/vmx/capabilities.h
> > > @@ -392,7 +392,9 @@ static inline bool vmx_pt_mode_is_host_guest(void)
> > >   static inline bool vmx_pebs_supported(void)
> > >   {
> > > -	return boot_cpu_has(X86_FEATURE_PEBS) && kvm_pmu_cap.pebs_ept;
> > > +	return boot_cpu_has(X86_FEATURE_PEBS) &&
> > > +	       !boot_cpu_has(X86_FEATURE_HYBRID_CPU) &&
> > > +	       kvm_pmu_cap.pebs_ept;
> > 
> > I assume the patch I just posted[*] to disable the vPMU entirely is sufficient, or
> 
> AFAI, some developers doing client-side virtualization on a hybrid cpu will
> specifically want vPMU,
> in which case it makes perfect sense for KVM to expose common pmu
> capabilities (not PEBS at the current) of big and little cores, such as the
> most basic performance counter.
> 
> > do we need this as well in order to hide X86_FEATURE_DS and X86_FEATURE_DTES64?
> 
> I think we still need this diff. Better to prioritize this minor feature a
> little bit for hungry users.

That wasn't my question.  My question was whether or not wholesale disabling vPMU
is sufficient to prevent issues with PEBS.  Unless we need this patch on top of
disabling the vPMU, my strong preference is to disable vPMU, or at the very least
make it off-by-default and require a explicit override.

I agree that there are users that want to enable vPMU for hybrid CPUs, but as
stated in the link below, that needs to be a dedicated enabling effort.  I don't
see any reason to exempt PEBS from that.  E.g. isn't PEBS usable if userspace pins
vCPUs to pCPUs and enumerates an accurate topology to the guest?

> > [*] https://lore.kernel.org/all/20230120004051.2043777-1-seanjc@google.com