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 ==
PERF_EVENT_STATE_ACTIVE' or something.
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) &&
+ (vmcs_read64(GUEST_IA32_DEBUGCTL) & DEBUGCTLMSR_LBR))
+ 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
that?
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.
Thanks,
Like Xu
