Re: [PATCH 1/1] KVM: x86: Print names of apicv inhibit reasons in traces

[Alejandro Jimenez]

On 3/8/24 11:06, Alejandro Jimenez wrote:
> On 2/14/24 17:35, Alejandro Jimenez wrote:
> > Use the tracing infrastructure helper __print_flags() for printing flag
> > bitfields, to enhance the trace output by displaying a string describing
> > each of the inhibit reasons set.
> >
> > The kvm_apicv_inhibit_changed tracepoint currently shows the raw bitmap
> > value, requiring the user to consult the source file where the inhbit
> > reasons are defined to decode the trace output.
> >
> > Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@xxxxxxxxxx>
> >
> > ---
> > checkpatch reports an error:
> > ERROR: Macros with complex values should be enclosed in parentheses
> >
> > but that seems common for other patches that also use a macro to define an array
> > of struct trace_print_flags used by __print_flags().
> >
> > I did not include an example of the new traces in the commit message since they
> > are longer than 80 columns, but perhaps that is desirable. e.g.:
> >
> > qemu-system-x86-6961    [055] .....  1779.344065: kvm_apicv_inhibit_changed: set reason=2, inhibits=0x4 ABSENT
> > qemu-system-x86-6961    [055] .....  1779.356710: kvm_apicv_inhibit_changed: cleared reason=2, inhibits=0x0
> >
> > qemu-system-x86-9912    [137] ..... 57106.196107: kvm_apicv_inhibit_changed: set reason=8, inhibits=0x300 IRQWIN|PIT_REINJ
> > qemu-system-x86-9912    [137] ..... 57106.196115: kvm_apicv_inhibit_changed: cleared reason=8, inhibits=0x200 PIT_REINJ
> > ---
> >   arch/x86/kvm/trace.h | 28 ++++++++++++++++++++++++++--
> >   1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> > index b82e6ed4f024..8469e59dfce2 100644
> > --- a/arch/x86/kvm/trace.h
> > +++ b/arch/x86/kvm/trace.h
> > @@ -1372,6 +1372,27 @@ TRACE_EVENT(kvm_hv_stimer_cleanup,
> >             __entry->vcpu_id, __entry->timer_index)
> >   );
> > +/*
> > + * The inhibit flags in this flag array must be kept in sync with the
> > + * kvm_apicv_inhibit enum members in <asm/kvm_host.h>.
> > + */
> > +#define APICV_INHIBIT_FLAGS \
> > +    { BIT(APICV_INHIBIT_REASON_DISABLE),         "DISABLED" }, \
> > +    { BIT(APICV_INHIBIT_REASON_HYPERV),         "HYPERV" }, \
> > +    { BIT(APICV_INHIBIT_REASON_ABSENT),         "ABSENT" }, \
> > +    { BIT(APICV_INHIBIT_REASON_BLOCKIRQ),         "BLOCKIRQ" }, \
> > +    { BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED), "PHYS_ID_ALIASED" }, \
> > +    { BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED),     "APIC_ID_MOD" }, \
> > +    { BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED),     "APIC_BASE_MOD" }, \
> > +    { BIT(APICV_INHIBIT_REASON_NESTED),         "NESTED" }, \
> > +    { BIT(APICV_INHIBIT_REASON_IRQWIN),         "IRQWIN" }, \
> > +    { BIT(APICV_INHIBIT_REASON_PIT_REINJ),         "PIT_REINJ" }, \
> > +    { BIT(APICV_INHIBIT_REASON_SEV),         "SEV" }, \
> > +    { BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED),     "LOG_ID_ALIASED" } \
> > +
> > +#define show_inhibit_reasons(inhibits) \
> > +    __print_flags(inhibits, "|", APICV_INHIBIT_FLAGS)
> > +
> >   TRACE_EVENT(kvm_apicv_inhibit_changed,
> >           TP_PROTO(int reason, bool set, unsigned long inhibits),
> >           TP_ARGS(reason, set, inhibits),
> > @@ -1388,9 +1409,12 @@ TRACE_EVENT(kvm_apicv_inhibit_changed,
> >           __entry->inhibits = inhibits;
> >       ),
> > -    TP_printk("%s reason=%u, inhibits=0x%lx",
> > +    TP_printk("%s reason=%u, inhibits=0x%lx%s%s",
> >             __entry->set ? "set" : "cleared",
> > -          __entry->reason, __entry->inhibits)
> > +          __entry->reason, __entry->inhibits,
> > +          __entry->inhibits ? " " : "",
> > +          __entry->inhibits ?
> > +          show_inhibit_reasons(__entry->inhibits) : "")
> >   );
> >   TRACE_EVENT(kvm_apicv_accept_irq,
> >
> > base-commit: 7455665a3521aa7b56245c0a2810f748adc5fdd4
> ping..

ping...

This is a trivial but useful usability improvement, and I don't expect it to be controversial (other than perhaps formatting of the string), but I'd appreciate any comments.

Alejandro