Re: [PATCH 0/5] kvm: x86: better handling of NULL-able kvm_x86_ops

[Sean Christopherson]

On Wed, Feb 02, 2022, Paolo Bonzini wrote:
> This series is really two changes:
> 
> - patch 1 to 4 clean up NULLable kvm_x86_ops so that they are marked
>   in kvm-x86-ops.h and the non-NULLable ones WARN if used incorrectly.
>   As an additional outcome of the review, a few more uses of
>   static_call_cond are introduced.
> 
> - patch 5 allows to NULL a few kvm_x86_ops that return a value, by
>   using __static_call_ret0.
> 
> Paolo Bonzini (5):
>   KVM: x86: use static_call_cond for optional callbacks
>   KVM: x86: mark NULL-able kvm_x86_ops
>   KVM: x86: warn on incorrectly NULL static calls
>   KVM: x86: change hwapic_{irr,isr}_update to NULLable calls
>   KVM: x86: allow defining return-0 static calls

I belatedly remembered the other thing about "NULL" that I don't like:

        #define KVM_X86_OP(func)                                             \
                DEFINE_STATIC_CALL_NULL(kvm_x86_##func,                      \
                                        *(((struct kvm_x86_ops *)0)->func));
        #define KVM_X86_OP_NULL KVM_X86_OP

That's bound to be confusing for folks that aren't already familiar with the
code, especially if they don't have a good handle on static_call() magic.

Side topic, the above doesn't handle KVM_X86_OP_RET0, no idea how that doesn't
fail at link time.  The BUILD_BUG_ON(1) in kvm-x86-ops.h also needs to be updated,
and the comment too.

Anyways, back to NULL.  KVM_X86_OP_RET0 also doesn't capture that the hook can
be NULL in that case; if the reader is familiar with static_call() then they'll
understand the full meaning, but I doubt that covers the majority of readers.

TL;DR: what about using more verbose names KVM_X86_OP_OPTIONAL and
KVM_X86_OP_OPTIONAL_RET0[*]?  And also tweak kvm_ops_static_call_update()'s
defines so that KVM_X86_OP never routes through KVM_X86_OP_OPTIONAL (as syntactic
sugar to avoid confusion.

Other than that, I like the WARN on KVM_X86_OPS with a NULL implementation.

[*] The OP_OPTIONAL kills me, but I can't think of a better alternative.


E.g. sans the kvm-x86-ops.h changes...

---
 arch/x86/include/asm/kvm_host.h | 11 ++++++-----
 arch/x86/kvm/x86.c              | 11 +++++++++--
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e7e5bd9a984d..055b3a2419f7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1539,17 +1539,18 @@ extern struct kvm_x86_ops kvm_x86_ops;

 #define KVM_X86_OP(func) \
 	DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_NULL KVM_X86_OP
-#define KVM_X86_OP_RET0 KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
 #include <asm/kvm-x86-ops.h>

 static inline void kvm_ops_static_call_update(void)
 {
-#define KVM_X86_OP_NULL(func) \
+#define __KVM_X86_OP(func) \
 	static_call_update(kvm_x86_##func, kvm_x86_ops.func);
 #define KVM_X86_OP(func) \
-	WARN_ON(!kvm_x86_ops.func); KVM_X86_OP_NULL(func)
-#define KVM_X86_OP_RET0(func) \
+	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
+#define KVM_X86_OP_OPTIONAL __KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0(func) \
 	static_call_update(kvm_x86_##func, kvm_x86_ops.func ? : \
 			   (typeof(kvm_x86_ops.func)) __static_call_return0);
 #include <asm/kvm-x86-ops.h>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 657aa646871e..337e39dec3c4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -125,11 +125,18 @@ static void __get_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2);

 struct kvm_x86_ops kvm_x86_ops __read_mostly;

+/*
+ * All ops are filled by vendor code, thus the default function is NULL for
+ * both mandatory and optional hooks.  The exception are optional RET0 hooks,
+ * which obviously default to __static_call_return0.
+ */
 #define KVM_X86_OP(func)					     \
 	DEFINE_STATIC_CALL_NULL(kvm_x86_##func,			     \
 				*(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_NULL KVM_X86_OP
-#define KVM_X86_OP_RET0 KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0(func)				     \
+	DEFINE_STATIC_CALL_RET0(kvm_x86_##func,			     \
+				*(((struct kvm_x86_ops *)0)->func));
 #include <asm/kvm-x86-ops.h>
 EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
 EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);

base-commit: 347f6a965596211726c39eb6bc320e8375f80b52
--