Received-SPF: pass (google.com: domain of linux-kernel+bounces-168208-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 3 May 2024 13:59:36 -0700
In-Reply-To: <20240214223554.1033154-1-alejandro.j.jimenez@oracle.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240214223554.1033154-1-alejandro.j.jimenez@oracle.com>
Message-ID: <ZjVQOFLXWrZvoa-Y@google.com>
Subject: Re: [PATCH 1/1] KVM: x86: Print names of apicv inhibit reasons in traces
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
Content-Type: text/plain; charset="us-ascii"

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
--