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