linux-kernel - Re: [PATCH v2 2/2] KVM: x86/pmu: Add PEBS support for SPR and future non-hybird models

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y0mLi5KRnzSiAXPk@google.com>
Date:   Fri, 14 Oct 2022 16:17:15 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Jim Mattson <jmattson@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v2 2/2] KVM: x86/pmu: Add PEBS support for SPR and future
 non-hybird models

On Fri, Oct 14, 2022, Like Xu wrote:
> On 12/10/2022 3:09 am, Sean Christopherson wrote:
> > > @@ -181,11 +200,11 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
> > >   		 * the accuracy of the PEBS profiling result, because the "event IP"
> > >   		 * in the PEBS record is calibrated on the guest side.
> > >   		 *
> > > -		 * On Icelake everything is fine. Other hardware (GLC+, TNT+) that
> > > +		 * On Icelake everything is fine. Other hardware (TNT+) that
> > >   		 * could possibly care here is unsupported and needs changes.
> > 
> > This part of the comment is still somewhat stale, and for me at least it's not at
> > all helpful.
> 
> SPR has GLC core, and this patch adds this support and removes the
> unsupported statements.

It's stale in the sense that it says "Icelake is fine", which implies that SPR is
NOT fine.

> Not sure how else I may help you.

Write comments that don't require the reader to have in-depth knowledge of the PMU's
magic numbers.  Explain what "unsupported" means.  Explain what changes might need
to be made.  Use search-friendly words, e.g. I assume TNT means Tremont, but Googling
for "Intel TNT" doesn't yield anything useful.

The above comment essentially says "Icelake is good, Tremont is broken", but that
doesn't help the reader understand what this code actually does.

> > >   		attr.precise_ip = 1;
> > > -		if (x86_match_cpu(vmx_icl_pebs_cpu) && pmc->idx == 32)
> > > +		if (need_max_precise(pmc))
> > >   			attr.precise_ip = 3;
> > 
> > What about writing this as:
> > 
> > 		attr.precise_ip = pmc_get_pebs_precision(pmc);
> > 
> > (or whatever name is appropriate for "pebs_precision").
> 
> The comment says, "the difference in the software precision levels of guest and
> host PEBS events will not affect the accuracy of the PEBS profiling result".
> 
> > 
> > Then in the helper, add comments to explaint the magic numbers and the interaction
> > with PDIST and PDIR.  Bonus points if #defines for the the magic numbers can be
> > added somewher
> 
> KVM just uses "precision_ip = 3" to request hw pdit/pdir counters from host perf,
> It only works at 3. For 1 or 2, there is no difference.

What only works at 3?  Are 0..3 architectural PMU values, or are they arbitrary
numbers perf uses?

> > 
> > 				 *  0 - SAMPLE_IP can have arbitrary skid
> > 				 *  1 - SAMPLE_IP must have constant skid
> > 				 *  2 - SAMPLE_IP requested to have 0 skid
> > 				 *  3 - SAMPLE_IP must have 0 skid
> > 
> > static u64 pmc_get_pebs_precision(struct kvm_pmc *pmc)
> > {
> > 	/* Comment that explains why PDIST/PDIR require 0 skid? */
> > 	if ((pmc->idx == 0 && x86_match_cpu(vmx_pebs_pdist_cpu)) ||
> > 	    (pmc->idx == 32 && x86_match_cpu(vmx_pebs_pdir_cpu)))
> > 		return 3;
> > 
> > 	/* Comment about constant skid? */
> > 	return 1;
> > }
> , therefore 0 or constant skid makes no difference.

Then why '1'?  

> > > diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> > > index c5e5dfef69c7..4dc4bbe18821 100644
> > > --- a/arch/x86/kvm/vmx/capabilities.h
> > > +++ b/arch/x86/kvm/vmx/capabilities.h
> > > @@ -398,7 +398,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) &&
> > 
> > This belongs in a separate patch, and it should be ordered before patch 1 so that
> > there's no window where KVM can see pebs_ept==1 on a hybrid CPU.
> > 
> > Actually, shouldn't everything in this patch land before core enabling?
> 
> Perf core enabling about SPR is already upstreamed.

I'm referring to patch 1, "perf/x86/intel: Expose EPT-friendly PEBS for SPR and
future models".  Before that patch, x86_pmu.pebs_ept will be set only for Icelake,
i.e. PEBS for KVM is effecitively not enabled in the core perf code until patch 1.
KVM should get all its support lined up, and then enable PEBS support for SPR and
beyond in core perf.  E.g. if someone applied only patch 1, they would be able to
use KVM PEBS on any CPU with PEBS v5, even if it were a hybrid CPU.