Re: [PATCH v11 10/11] KVM: x86/pmu: Check guest LBR availability in case host reclaims them

From: Like Xu
Date: Wed May 27 2020 - 04:17:50 EST

Hi Peter,

On 2020/5/20 10:01, Xu, Like wrote:
On 2020/5/19 22:57, Peter Zijlstra wrote:
On Tue, May 19, 2020 at 09:10:58PM +0800, Xu, Like wrote:
On 2020/5/19 19:15, Peter Zijlstra wrote:
On Thu, May 14, 2020 at 04:30:53PM +0800, Like Xu wrote:

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index ea4faae56473..db185dca903d 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -646,6 +646,43 @@ static void intel_pmu_lbr_cleanup(struct kvm_vcpu *vcpu)
ÂÂÂÂÂÂÂÂÂÂ intel_pmu_free_lbr_event(vcpu);
ÂÂ }
+static bool intel_pmu_lbr_is_availabile(struct kvm_vcpu *vcpu)
+ÂÂÂ struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+ÂÂÂ if (!pmu->lbr_event)
+ÂÂÂÂÂÂÂ return false;
+ÂÂÂ if (event_is_oncpu(pmu->lbr_event)) {
+ÂÂÂÂÂÂÂ intel_pmu_intercept_lbr_msrs(vcpu, false);
+ÂÂÂ } else {
+ÂÂÂÂÂÂÂ intel_pmu_intercept_lbr_msrs(vcpu, true);
+ÂÂÂÂÂÂÂ return false;
+ÂÂÂ }
+ÂÂÂ return true;
This is unreadable gunk, what?
Abstractly, it is saying "KVM would passthrough the LBR satck MSRs if
event_is_oncpu() is true, otherwise cancel the passthrough state if any."

I'm using 'event->oncpu != -1' to represent the guest LBR event
is scheduled on rather than 'event->state == PERF_EVENT_STATE_ERROR'.

For intel_pmu_intercept_lbr_msrs(), false means to passthrough the LBR stack
MSRs to the vCPU, and true means to cancel the passthrough state and make
LBR MSR accesses trapped by the KVM.
To me it seems very weird to change state in a function that is supposed
to just query state.

'is_available' seems to suggest a simple: return 'lbr_event->state ==
This clarification led me to reconsider the use of a more readable name here.

Do you accept the check usage of "event->oncpu != -1" instead of
'event->state == PERF_EVENT_STATE_ERROR' before KVM do passthrough ?

+static void intel_pmu_availability_check(struct kvm_vcpu *vcpu)
+ÂÂÂ lockdep_assert_irqs_disabled();
+ÂÂÂ if (lbr_is_enabled(vcpu) && !intel_pmu_lbr_is_availabile(vcpu) &&
+ÂÂÂÂÂÂÂ pr_warn_ratelimited("kvm: vcpu-%d: LBR is temporarily unavailable.\n",
+ÂÂÂÂÂÂÂÂÂÂÂ vcpu->vcpu_id);
More unreadable nonsense; when the events go into ERROR state, it's a
permanent fail, they'll not come back.
It's not true. The guest LBR event with 'ERROR state' or 'oncpu != -1'
would be
lazy released and re-created in the next time the
intel_pmu_create_lbr_event() is
called and it's supposed to be re-scheduled and re-do availability_check()
as well.
Where? Also, wth would you need to destroy and re-create an event for
If the guest does not set the EN_LBR bit and did not touch any LBR-related registers
in the last time slice, KVM will destroy the guest LBR event in kvm_pmu_cleanup()
which is called once every time the vCPU thread is scheduled in.

The re-creation is not directly called after the destruction
but is triggered by the next guest access to the LBR-related registers if any.

From the time when the guest LBR event enters the "oncpu! = -1" state
to the next re-creation, the guest LBR is not available. After the re-creation,
the guest LBR is hopefully available and if it's true, the LBR will be passthrough
and used by the guest normally.

That's the reason for "LBR is temporarily unavailable"

Do you still have any concerns on this issue?

and please let me know if it doesn't make sense to you.

@@ -6696,8 +6696,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu)
ÂÂÂÂÂÂ pt_guest_enter(vmx);
-ÂÂÂ if (vcpu_to_pmu(vcpu)->version)
+ÂÂÂ if (vcpu_to_pmu(vcpu)->version) {
ÂÂÂÂÂÂÂÂÂÂ atomic_switch_perf_msrs(vmx);
+ÂÂÂÂÂÂÂ kvm_x86_ops.pmu_ops->availability_check(vcpu);
+ÂÂÂ }
AFAICT you just did a call out to the kvm_pmu crud in
atomic_switch_perf_msrs(), why do another call?
In fact, availability_check() is only called here for just one time.

The callchain looks like:
- vmx_vcpu_run()
ÂÂÂÂ - kvm_x86_ops.pmu_ops->availability_check();
ÂÂÂÂ ÂÂÂ - intel_pmu_availability_check()
ÂÂÂÂ ÂÂÂ ÂÂÂ - intel_pmu_lbr_is_availabile()
ÂÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ - event_is_oncpu() ...

What I'm saying is that you just did a pmu_ops indirect call in
atomic_switch_perf_msrs(), why add another?
Do you mean the indirect call:
- atomic_switch_perf_msrs()
ÂÂÂ - perf_guest_get_msrs()
ÂÂÂ ÂÂÂ - x86_pmu.guest_get_msrs()

The two pmu_ops are quite different:
- the first one in atomic_switch_perf_msrs() is defined in the host side;
- the second one for availability_check() is defined in the KVM side;

The availability_check() for guest LBR event and MSRs pass-through
operations are definitely KVM context specific.

Do you still have any concerns on this issue?

If you have more comments on the patchset, please let me know.

Like Xu