All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Yang Weijiang <weijiang.yang@intel.com>
Cc: pbonzini@redhat.com, jmattson@google.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, like.xu.linux@gmail.com,
	kan.liang@linux.intel.com, wei.w.wang@intel.com
Subject: Re: [PATCH v2 00/15] Introduce Architectural LBR for vPMU
Date: Fri, 27 Jan 2023 22:46:18 +0000	[thread overview]
Message-ID: <Y9RUOvJ5dkCU9J8C@google.com> (raw)
In-Reply-To: <20221125040604.5051-1-weijiang.yang@intel.com>

On Thu, Nov 24, 2022, Yang Weijiang wrote:
> Intel CPU model-specific LBR(Legacy LBR) has evolved to Architectural
> LBR(Arch LBR [0]), it's the replacement of legacy LBR on new platforms.
> The native support patches were merged into 5.9 kernel tree, and this
> patch series is to enable Arch LBR in vPMU so that guest can benefit
> from the feature.
> 
> The main advantages of Arch LBR are [1]:
> - Faster context switching due to XSAVES support and faster reset of
>   LBR MSRs via the new DEPTH MSR
> - Faster LBR read for a non-PEBS event due to XSAVES support, which
>   lowers the overhead of the NMI handler.
> - Linux kernel can support the LBR features without knowing the model
>   number of the current CPU.
> 
> From end user's point of view, the usage of Arch LBR is the same as
> the Legacy LBR that has been merged in the mainline.
> 
> Note, in this series, there's one restriction for guest Arch LBR, i.e.,
> guest can only set its LBR record depth the same as host's. This is due
> to the special behavior of MSR_ARCH_LBR_DEPTH: 
> 1) On write to the MSR, it'll reset all Arch LBR recording MSRs to 0s.
> 2) XRSTORS resets all record MSRs to 0s if the saved depth mismatches
> MSR_ARCH_LBR_DEPTH.
> Enforcing the restriction keeps KVM Arch LBR vPMU working flow simple
> and straightforward.
> 
> Paolo refactored the old series and the resulting patches became the
> base of this new series, therefore he's the author of some patches.

To be very blunt, this series is a mess.  I don't want to point fingers as there
is plenty of blame to go around.  The existing LBR support is a confusing mess,
vPMU as a whole has been neglected for too long, review feedback has been relatively
non-existent, and I'm sure some of the mess is due to Paolo trying to hastily fix
things up back when this was temporarily queued.

However, for arch LBR support to be merged, things need to change.

First and foremost, the existing LBR support needs to be documented.  Someone,
I don't care who, needs to provide a detailed writeup of the contract between KVM
and perf.  Specifically, I want to know:

  1. When exactly is perf allowed to take control of LBR MRS.  Task switch?  IRQ?
     NMI?

  2. What is the expected behavior when perf is using LBRs?  Is the guest supposed
     to be traced?

  3. Why does KVM snapshot DEBUGCTL with IRQs enabled, but disables IRQs when
     accessing LBR MSRs?

It doesn't have to be polished, e.g. I'll happily wordsmith things into proper
documentation, but I want to have a very clear understanding of how LBR support
is _intended_ to function and how it all _actually_ functions without having to
make guesses.

And depending on the answers, I want to revisit KVM's LBR implementation before
tackling arch LBRs.  Letting perf usurp LBRs while KVM has the vCPU loaded is
frankly ridiculous.  Just have perf set a flag telling KVM that it needs to take
control of LBRs and have KVM service the flag as a request or something.  Stealing
the LBRs back in IRQ context adds a stupid amount of complexity without much value,
e.g. waiting a few branches for KVM to get to a safe place isn't going to meaningfully
change the traces.  If that can't actually happen, then why on earth does KVM need
to disable IRQs to read MSRs?

And AFAICT, since KVM unconditionally loads the guest's DEBUGCTL, whether or not
guest branches show up in the LBRs when the host is tracing is completely up to
the whims of the guest.  If that's correct, then again, what's the point of the
dance between KVM and perf?

Beyond the "how does this work" issues, there needs to be tests.  At the absolute
minimum, there needs to be selftests showing that this stuff actually works, that
save/restore (migration) works, that the MSRs can/can't be accessed when guest
CPUID is (in)correctly configured, etc. And I would really, really like to have
tests that force contention between host and guests, e.g. to make sure that KVM
isn't leaking host state or outright exploding, but I can understand that those
types of tests would be very difficult to write.

I've pushed a heavily reworked, but definitely broken, version to

  git@github.com:sean-jc/linux.git x86/arch_lbrs

It compiles, but it's otherwise untested and there are known gaps.  E.g. I omitted
toggling load+clear of ARCH_LBR_CTL because I couldn't figure out the intended
behavior.

  parent reply	other threads:[~2023-01-27 22:46 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-25  4:05 [PATCH v2 00/15] Introduce Architectural LBR for vPMU Yang Weijiang
2022-11-25  4:05 ` [PATCH v2 01/15] perf/x86/lbr: Simplify the exposure check for the LBR_INFO registers Yang Weijiang
2022-12-22 10:57   ` Like Xu
2022-12-22 13:29     ` Peter Zijlstra
2022-12-22 17:41     ` Sean Christopherson
2022-12-23  2:12       ` Like Xu
2022-12-27 11:58   ` [tip: perf/core] " tip-bot2 for Like Xu
2022-11-25  4:05 ` [PATCH v2 02/15] KVM: x86: Report XSS as an MSR to be saved if there are supported features Yang Weijiang
2022-11-25  4:05 ` [PATCH v2 03/15] KVM: x86: Refresh CPUID on writes to MSR_IA32_XSS Yang Weijiang
2023-01-26 19:50   ` Sean Christopherson
2023-01-30  6:33     ` Yang, Weijiang
2022-11-25  4:05 ` [PATCH v2 04/15] KVM: PMU: disable LBR handling if architectural LBR is available Yang Weijiang
2023-01-27 20:10   ` Sean Christopherson
2023-01-30  8:10     ` Yang, Weijiang
2022-11-25  4:05 ` [PATCH v2 05/15] KVM: vmx/pmu: Emulate MSR_ARCH_LBR_DEPTH for guest Arch LBR Yang Weijiang
2022-12-22 11:00   ` Like Xu
2022-12-25  4:30     ` Yang, Weijiang
2022-12-22 11:15   ` Like Xu
2023-01-27 20:25   ` Sean Christopherson
2023-01-30 11:46     ` Yang, Weijiang
2022-11-25  4:05 ` [PATCH v2 06/15] KVM: vmx/pmu: Emulate MSR_ARCH_LBR_CTL " Yang Weijiang
2022-12-22 11:09   ` Like Xu
2022-12-25  4:27     ` Yang, Weijiang
2022-12-22 11:19   ` Like Xu
2022-12-25  4:16     ` Yang, Weijiang
2022-12-22 11:24   ` Like Xu
2022-12-25  4:08     ` Yang, Weijiang
2023-01-27 21:42   ` Sean Christopherson
2022-11-25  4:05 ` [PATCH v2 07/15] KVM: VMX: Support passthrough of architectural LBRs Yang Weijiang
2022-11-25  4:05 ` [PATCH v2 08/15] KVM: x86: Add Arch LBR MSRs to msrs_to_save_all list Yang Weijiang
2023-01-27 21:43   ` Sean Christopherson
2023-01-30 12:27     ` Yang, Weijiang
2022-11-25  4:05 ` [PATCH v2 09/15] KVM: x86: Refine the matching and clearing logic for supported_xss Yang Weijiang
2023-01-27 21:46   ` Sean Christopherson
2023-01-30 12:37     ` Yang, Weijiang
2022-11-25  4:05 ` [PATCH v2 10/15] KVM: x86/vmx: Check Arch LBR config when return perf capabilities Yang Weijiang
2022-12-22 11:06   ` Like Xu
2022-12-25  4:28     ` Yang, Weijiang
2023-01-27 22:04   ` Sean Christopherson
2022-11-25  4:06 ` [PATCH v2 11/15] KVM: x86: Add XSAVE Support for Architectural LBR Yang Weijiang
2023-01-27 22:07   ` Sean Christopherson
2023-01-30 13:13     ` Yang, Weijiang
2022-11-25  4:06 ` [PATCH v2 12/15] KVM: x86/vmx: Disable Arch LBREn bit in #DB and warm reset Yang Weijiang
2022-12-22 11:22   ` Like Xu
2022-12-25  4:12     ` Yang, Weijiang
2023-01-27 22:09   ` Sean Christopherson
2023-01-30 13:09     ` Yang, Weijiang
2022-11-25  4:06 ` [PATCH v2 13/15] KVM: x86/vmx: Save/Restore guest Arch LBR Ctrl msr at SMM entry/exit Yang Weijiang
2023-01-27 22:11   ` Sean Christopherson
2023-01-30 12:50     ` Yang, Weijiang
2022-11-25  4:06 ` [PATCH v2 14/15] KVM: x86: Add Arch LBR data MSR access interface Yang Weijiang
2023-01-27 22:13   ` Sean Christopherson
2023-01-30 12:46     ` Yang, Weijiang
2023-01-30 17:30       ` Sean Christopherson
2023-01-31 13:14         ` Yang, Weijiang
2023-01-31 16:05           ` Sean Christopherson
2022-11-25  4:06 ` [PATCH v2 15/15] KVM: x86/cpuid: Advertise Arch LBR feature in CPUID Yang Weijiang
2022-12-22 11:03   ` Like Xu
2022-12-25  4:31     ` Yang, Weijiang
2023-01-27 22:15   ` Sean Christopherson
2023-01-12  1:57 ` [PATCH v2 00/15] Introduce Architectural LBR for vPMU Yang, Weijiang
2023-01-27 22:46 ` Sean Christopherson [this message]
2023-01-30 13:38   ` Yang, Weijiang
2023-06-05  9:50   ` Like Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y9RUOvJ5dkCU9J8C@google.com \
    --to=seanjc@google.com \
    --cc=jmattson@google.com \
    --cc=kan.liang@linux.intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=like.xu.linux@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=wei.w.wang@intel.com \
    --cc=weijiang.yang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.