On 1/28/22 16:42, Sean Christopherson wrote:
On Fri, Jan 28, 2022, Paolo Bonzini wrote:
On 1/28/22 01:51, Sean Christopherson wrote:
Drop KVM_X86_OP_NULL, which is superfluous and confusing. The macro is
just a "pass-through" to KVM_X86_OP; it was added with the intent of
actually using it in the future, but that obviously never happened. The
name is confusing because its intended use was to provide a way for
vendor implementations to specify a NULL pointer, and even if it were
used, wouldn't necessarily be synonymous with declaring a kvm_x86_op as
DEFINE_STATIC_CALL_NULL.
Lastly, actually using KVM_X86_OP_NULL as intended isn't a maintanable
approach, e.g. bleeds vendor details into common x86 code, and would
either be prone to bit rot or would require modifying common x86 code
when modifying a vendor implementation.
I have some patches that redefine KVM_X86_OP_NULL as "must be used with
static_call_cond". That's a more interesting definition, as it can be used
to WARN if KVM_X86_OP is used with a NULL function pointer.
I'm skeptical that will actually work well and be maintainble. E.g. sync_pir_to_ir()
must be explicitly check for NULL in apic_has_interrupt_for_ppr(), forcing that path
to do static_call_cond() will be odd. Ditto for ops that are wired up to ioctl()s,
e.g. the confidential VM stuff, and for ops that are guarded by other stuff, e.g. the
hypervisor timer.
Actually, it won't just be odd, it will be impossible to disallow NULL a pointer
for KVM_X86_OP and require static_call_cond() for KVM_X86_OP_NULL. static_call_cond()
forces the return to "void", so any path that returns a value needs to be manually
guarded and can't use static_call_cond(), e.g.
You're right and I should have looked up the series instead of going by
memory. What I did was mostly WARNing on KVM_X86_OP that sets NULL, as
non-NULL ops are the common case. I also added KVM_X86_OP_RET0 to
remove some checks on kvm_x86_ops for ops that return a value.
All in all I totally agree with patches 2-11 and will apply them (patch
2 to 5.17 even, as a prerequisite to fix the AVIC race). Several of
patches 13-21 are also mostly useful as it clarifies the code, and the
others I guess are okay in the context of a coherent series though
probably they would have been rejected as one-offs. However, patches 12
and 22 are unnecessary uses of the C preprocessor in my opinion.
Paolo
