From: Sean Christopherson <seanjc@google.com>
To: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com,
linux-kernel@vger.kernel.org, joao.m.martins@oracle.com,
boris.ostrovsky@oracle.com, suravee.suthikulpanit@amd.com,
mlevitsk@redhat.com
Subject: Re: [PATCH 1/1] KVM: x86: Print names of apicv inhibit reasons in traces
Date: Fri, 3 May 2024 13:59:36 -0700 [thread overview]
Message-ID: <ZjVQOFLXWrZvoa-Y@google.com> (raw)
In-Reply-To: <20240214223554.1033154-1-alejandro.j.jimenez@oracle.com>
On Wed, Feb 14, 2024, 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@oracle.com>
>
> ---
> checkpatch reports an error:
> ERROR: Macros with complex values should be enclosed in parentheses
Yeah, checkpatch is a tool/guide, there are situations where it's best to use
common sense and ignore checkpatch.
> +/*
> + * The inhibit flags in this flag array must be kept in sync with the
> + * kvm_apicv_inhibit enum members in <asm/kvm_host.h>.
Comments are nice, code is better. And in this case, it's quite easy to assert
that all inhibits have an entry via BUILD_BUG_ON(), e.g. by defining a throwaway
array and asserting that ARRAY_SIZE == NR_APICV_INHIBIT_REASONS.
It's not so easy to add such an assert for the VMX exit reason because the numbers
aren't arbitrary and there are gaps, but for APICv inhibits, the numbers truly
don't matter.
> + */
> +#define APICV_INHIBIT_FLAGS \
> + { BIT(APICV_INHIBIT_REASON_DISABLE), "DISABLED" }, \
To reduce copy+paste and especially to minimize the number of typos, use another
macro, e.g.
#define __APICV_INHIBIT_REASON(reason) \
{ BIT(APICV_INHIBIT_REASON_##reason), #reason }
If the DISABLE vs. DISABLED annoys you enough, feel free to tack on add a patch
to rename the enum itself.
> + { 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) \
s/show/print, no reason to introduce yet more terminology. And it probably makes
sense to use a fully verbose name, e.g. kvm_print_apicv_inhibit_reasons().
> + __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) : "")
Use the inner macro to print the _entire_ sequence. The fmt is obviously going
to be coupled with the macro regardless, and so having the macro handle everything
makes it a bit more obvious that the entirety of 0x%lx%s%s of being taken care of.
All in all, sans the potential s/DISABLE/DISABLED rename, this? Compile tested only.
---
arch/x86/include/asm/kvm_host.h | 19 +++++++++++++++++++
arch/x86/kvm/trace.h | 9 +++++++--
arch/x86/kvm/x86.c | 4 ++++
3 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1d13e3cd1dc5..8d9d9a0e9685 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1273,8 +1273,27 @@ enum kvm_apicv_inhibit {
* mapping between logical ID and vCPU.
*/
APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED,
+
+ NR_APICV_INHIBIT_REASONS,
};
+#define __APICV_INHIBIT_REASON(reason) \
+ { BIT(APICV_INHIBIT_REASON_##reason), #reason }
+
+#define APICV_INHIBIT_REASONS \
+ __APICV_INHIBIT_REASON(DISABLE), \
+ __APICV_INHIBIT_REASON(HYPERV), \
+ __APICV_INHIBIT_REASON(ABSENT), \
+ __APICV_INHIBIT_REASON(BLOCKIRQ), \
+ __APICV_INHIBIT_REASON(PHYSICAL_ID_ALIASED), \
+ __APICV_INHIBIT_REASON(APIC_ID_MODIFIED), \
+ __APICV_INHIBIT_REASON(APIC_BASE_MODIFIED), \
+ __APICV_INHIBIT_REASON(NESTED), \
+ __APICV_INHIBIT_REASON(IRQWIN), \
+ __APICV_INHIBIT_REASON(PIT_REINJ), \
+ __APICV_INHIBIT_REASON(SEV), \
+ __APICV_INHIBIT_REASON(LOGICAL_ID_ALIASED)
+
struct kvm_arch {
unsigned long n_used_mmu_pages;
unsigned long n_requested_mmu_pages;
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 9d0b02ef307e..f23fb9a6776e 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1375,6 +1375,10 @@ TRACE_EVENT(kvm_hv_stimer_cleanup,
__entry->vcpu_id, __entry->timer_index)
);
+#define kvm_print_apicv_inhibit_reasons(inhibits) \
+ (inhibits), (inhibits) ? " " : "", \
+ (inhibits) ? __print_flags(inhibits, "|", APICV_INHIBIT_REASONS) : ""
+
TRACE_EVENT(kvm_apicv_inhibit_changed,
TP_PROTO(int reason, bool set, unsigned long inhibits),
TP_ARGS(reason, set, inhibits),
@@ -1391,9 +1395,10 @@ 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,
+ kvm_print_apicv_inhibit_reasons(__entry->inhibits))
);
TRACE_EVENT(kvm_apicv_accept_irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b389129d59a9..ea1ef901fb8c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10011,6 +10011,10 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_apicv_activated);
static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
enum kvm_apicv_inhibit reason, bool set)
{
+ const struct trace_print_flags apic_inhibits[] = { APICV_INHIBIT_REASONS };
+
+ BUILD_BUG_ON(ARRAY_SIZE(apic_inhibits) != NR_APICV_INHIBIT_REASONS);
+
if (set)
__set_bit(reason, inhibits);
else
base-commit: 04b1c6b4841dc319810db044e887219e05fae27f
--
prev parent reply other threads:[~2024-05-03 20:59 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 22:35 [PATCH 1/1] KVM: x86: Print names of apicv inhibit reasons in traces Alejandro Jimenez
2024-03-08 16:06 ` Alejandro Jimenez
2024-03-26 16:47 ` Alejandro Jimenez
2024-04-08 8:43 ` Vasant Hegde
2024-05-03 20:59 ` Sean Christopherson [this message]
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=ZjVQOFLXWrZvoa-Y@google.com \
--to=seanjc@google.com \
--cc=alejandro.j.jimenez@oracle.com \
--cc=boris.ostrovsky@oracle.com \
--cc=joao.m.martins@oracle.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=suravee.suthikulpanit@amd.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).