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.
arch/x86/kvm/x86.c: In function ‘kvm_arch_vm_ioctl’:
arch/x86/kvm/x86.c:6450:19: error: void value not ignored as it ought to be
6450 | r = static_call_cond(kvm_x86_mem_enc_ioctl)(kvm, argp);
| ^
