Trace all APICv inhibit changes instead of just those that result in
APICv being (un)inhibited, and log the current state. Debugging why
APICv isn't working is frustrating as it's hard to see why APICv is still
inhibited, and logging only the first inhibition means unnecessary onion
peeling.
Opportunistically drop the export of the tracepoint, it is not and should
not be used by vendor code due to the need to serialize toggling via
apicv_update_lock.
Note, using the common flow means kvm_apicv_init() switched from atomic
to non-atomic bitwise operations. The VM is unreachable at init, so
non-atomic is perfectly ok.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/trace.h | 18 ++++++++++--------
arch/x86/kvm/x86.c | 29 +++++++++++++++++++----------
2 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 105037a251b5..e3a24b8f04be 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1339,23 +1339,25 @@ TRACE_EVENT(kvm_hv_stimer_cleanup,
__entry->vcpu_id, __entry->timer_index)
);
-TRACE_EVENT(kvm_apicv_update_request,
- TP_PROTO(int reason, bool activate),
- TP_ARGS(reason, activate),
+TRACE_EVENT(kvm_apicv_inhibit_changed,
+ TP_PROTO(int reason, bool set, unsigned long inhibits),
+ TP_ARGS(reason, set, inhibits),
TP_STRUCT__entry(
__field(int, reason)
- __field(bool, activate)
+ __field(bool, set)
+ __field(unsigned long, inhibits)
),
TP_fast_assign(
__entry->reason = reason;
- __entry->activate = activate;
+ __entry->set = set;
+ __entry->inhibits = inhibits;
),
- TP_printk("%s reason=%u",
- __entry->activate ? "activate" : "deactivate",
- __entry->reason)
+ TP_printk("%s reason=%u, inhibits=0x%lx",
+ __entry->set ? "set" : "cleared",
+ __entry->reason, __entry->inhibits)
);
TRACE_EVENT(kvm_apicv_accept_irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 965688aa6b45..7333322a22ff 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9053,15 +9053,29 @@ bool kvm_apicv_activated(struct kvm *kvm)
}
EXPORT_SYMBOL_GPL(kvm_apicv_activated);
+
+static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
+ enum kvm_apicv_inhibit reason, bool set)
+{
+ if (set)
+ __set_bit(reason, inhibits);
+ else
+ __clear_bit(reason, inhibits);
+
+ trace_kvm_apicv_inhibit_changed(reason, set, *inhibits);
+}
+
static void kvm_apicv_init(struct kvm *kvm)
{
+ unsigned long *inhibits = &kvm->arch.apicv_inhibit_reasons;
+
init_rwsem(&kvm->arch.apicv_update_lock);
- set_bit(APICV_INHIBIT_REASON_ABSENT,
- &kvm->arch.apicv_inhibit_reasons);
+ set_or_clear_apicv_inhibit(inhibits, APICV_INHIBIT_REASON_ABSENT, true);
+
if (!enable_apicv)
- set_bit(APICV_INHIBIT_REASON_DISABLE,
- &kvm->arch.apicv_inhibit_reasons);
+ set_or_clear_apicv_inhibit(inhibits,
+ APICV_INHIBIT_REASON_ABSENT, true);
}
static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
@@ -9747,13 +9761,9 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
old = new = kvm->arch.apicv_inhibit_reasons;
- if (set)
- __set_bit(reason, &new);
- else
- __clear_bit(reason, &new);
+ set_or_clear_apicv_inhibit(&new, reason, set);
if (!!old != !!new) {
- trace_kvm_apicv_update_request(reason, !set);
/*
* Kick all vCPUs before setting apicv_inhibit_reasons to avoid
* false positives in the sanity check WARN in svm_vcpu_run().
@@ -12939,7 +12949,6 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pi_irte_update);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_unaccelerated_access);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_incomplete_ipi);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_ga_log);
-EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_apicv_update_request);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_apicv_accept_irq);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_vmgexit_enter);
EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_vmgexit_exit);
--
2.35.1.723.g4982287a31-goog
On 3/15/22 15:48, Maxim Levitsky wrote:
>> Note that some calls may not toggle any bit. Do you want to log them?
>> I am afraid that a VM with many vCPUs may get a lot of traces that actually
>> doesn't change inhibits.
> I also think so.
Let's keep Sean's version for now, it may also be useful to see the
state changes for all vCPU threads (based on the pid field in the
trace). We can always change it later if it's too noisy.
Paolo
On Tue, 2022-03-15 at 22:42 +0800, Chao Gao wrote:
> On Fri, Mar 11, 2022 at 04:35:17AM +0000, Sean Christopherson wrote:
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9053,15 +9053,29 @@ bool kvm_apicv_activated(struct kvm *kvm)
> > }
> > EXPORT_SYMBOL_GPL(kvm_apicv_activated);
> >
> > +
>
> stray newline.
>
> > +static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
> > + enum kvm_apicv_inhibit reason, bool set)
> > +{
> > + if (set)
> > + __set_bit(reason, inhibits);
> > + else
> > + __clear_bit(reason, inhibits);
> > +
> > + trace_kvm_apicv_inhibit_changed(reason, set, *inhibits);
>
> Note that some calls may not toggle any bit. Do you want to log them?
> I am afraid that a VM with many vCPUs may get a lot of traces that actually
> doesn't change inhibits.
I also think so.
Best regards,
Maxim Levitsky
>
> Anyway, this series looks good to me.
>
On Fri, Mar 11, 2022 at 04:35:17AM +0000, Sean Christopherson wrote:
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -9053,15 +9053,29 @@ bool kvm_apicv_activated(struct kvm *kvm)
> }
> EXPORT_SYMBOL_GPL(kvm_apicv_activated);
>
>+
stray newline.
>+static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
>+ enum kvm_apicv_inhibit reason, bool set)
>+{
>+ if (set)
>+ __set_bit(reason, inhibits);
>+ else
>+ __clear_bit(reason, inhibits);
>+
>+ trace_kvm_apicv_inhibit_changed(reason, set, *inhibits);
Note that some calls may not toggle any bit. Do you want to log them?
I am afraid that a VM with many vCPUs may get a lot of traces that actually
doesn't change inhibits.
Anyway, this series looks good to me.