Re: [PATCH 1/2] KVM: x86: introduce definitions to support static calls for kvm_x86_ops

[Jason Baron <jbaron@akamai.com>]

On 1/13/21 11:16 AM, Jason Baron wrote:
> 
> 
> On 1/13/21 7:53 AM, Paolo Bonzini wrote:
>> On 11/01/21 17:57, Jason Baron wrote:
>>> +#define DEFINE_KVM_OPS_STATIC_CALL(func)    \
>>> +    DEFINE_STATIC_CALL_NULL(kvm_x86_##func,    \
>>> +                *(((struct kvm_x86_ops *)0)->func))
>>> +#define DEFINE_KVM_OPS_STATIC_CALLS() \
>>> +    FOREACH_KVM_X86_OPS(DEFINE_KVM_OPS_STATIC_CALL)
>>
>> Something wrong here?
> 
> Hmmm...not sure what you are getting at here.
> 
>>
>>> +#define DECLARE_KVM_OPS_STATIC_CALL(func)    \
>>> +    DECLARE_STATIC_CALL(kvm_x86_##func,    \
>>> +                *(((struct kvm_x86_ops *)0)->func))
>>> +#define DECLARE_KVM_OPS_STATIC_CALLS()        \
>>> +    FOREACH_KVM_X86_OPS(DECLARE_KVM_OPS_STATIC_CALL)
>>> +
>>> +#define KVM_OPS_STATIC_CALL_UPDATE(func)    \
>>> +    static_call_update(kvm_x86_##func, kvm_x86_ops.func)
>>> +#define KVM_OPS_STATIC_CALL_UPDATES()        \
>>> +    FOREACH_KVM_X86_OPS(KVM_OPS_STATIC_CALL_UPDATE)
>>> +
>>>   struct kvm_x86_ops {
>>>       int (*hardware_enable)(void);
>>>       void (*hardware_disable)(void);
>>> @@ -1326,6 +1385,12 @@ extern u64 __read_mostly host_efer;
>>>   extern bool __read_mostly allow_smaller_maxphyaddr;
>>>   extern struct kvm_x86_ops kvm_x86_ops;
>>>   +DECLARE_KVM_OPS_STATIC_CALLS();
>>> +static inline void kvm_ops_static_call_update(void)
>>> +{
>>> +    KVM_OPS_STATIC_CALL_UPDATES();
>>> +}
>>
>> This would become
>>
>> #define KVM_X86_OP(func) \
>>     DECLARE_STATIC_CALL(kvm_x86_##func,    \
>>                 *(((struct kvm_x86_ops *)0)->func));
>>
>> #include <asm/kvm-x86-ops.h>
>>
>> static inline void kvm_ops_static_call_update(void)
>> {
>> #define KVM_X86_OP(func) \
>>   static_call_update(kvm_x86_##func, kvm_x86_ops.func)
>> #include <asm/kvm-x86-ops.h>
>> }
>>
>> If you need to choose between DECLARE_STATIC_CALL_NULL and DECLARE_STATIC_CALL, you can have kvm-x86-ops.h use one of two macros KVM_X86_OP_NULL and
>> KVM_X86_OP.
>>
>> #define KVM_X86_OP(func) \
>>     DECLARE_STATIC_CALL(kvm_x86_##func,    \
>>                 *(((struct kvm_x86_ops *)0)->func));
>>
>> #define KVM_X86_OP_NULL(func) \
>>     DECLARE_STATIC_CALL_NULL(kvm_x86_##func,    \
>>                 *(((struct kvm_x86_ops *)0)->func));
>>
>> #include <asm/kvm-x86-ops.h>
>>
>> ...
>>
>> #define KVM_X86_OP(func) \
>>   static_call_update(kvm_x86_##func, kvm_x86_ops.func)
>> #define KVM_X86_OP_NULL(func) \
>>   static_call_update(kvm_x86_##func, kvm_x86_ops.func)
>> #include <asm/kvm-x86-ops.h>
>>
>> In that case vmx.c and svm.c could define KVM_X86_OP_NULL to an empty string and list the optional callbacks manually.
>>
> 
> Ok, yes, this all makes sense. So I looked at vmx/svm definitions
> and I see that there are 5 definitions that are common that
> don't use the vmx or svm prefix:
> 
> .update_exception_bitmap = update_exception_bitmap,
> .enable_nmi_window = enable_nmi_window,
> .enable_irq_window = enable_irq_window,
> .update_cr8_intercept = update_cr8_intercept,
> .enable_smi_window = enable_smi_window,
> 
> 8 are specific to vmx that don't use the vmx prefix:
> 
> .hardware_unsetup = hardware_unsetup,
> .hardware_enable = hardware_enable,
> .hardware_disable = hardware_disable,
> .cpu_has_accelerated_tpr = report_flexpriority,
> .dy_apicv_has_pending_interrupt = pi_has_pending_interrupt,
> .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
> .update_pi_irte = pi_update_irte,
> .complete_emulated_msr = kvm_complete_insn_gp,
> 
> and finally 7 specific to svm that don't use the svm prefix:
> 
> .get_cs_db_l_bits = kvm_get_cs_db_l_bits,
> .handle_exit = handle_exit,
> .skip_emulated_instruction = skip_emulated_instruction,
> .update_emulated_instruction = NULL,
> .sync_pir_to_irr = kvm_lapic_find_highest_irr,
> .apicv_post_state_restore = avic_post_state_restore,
> .request_immediate_exit = __kvm_request_immediate_exit,
> 
> 
> So we could set all of these to empty definitions and specifically
> add the callbacks manually as you suggested. Or we could have 4
> categories of macros:
> 
> 1) KVM_X86_OP()  - for the usual case where we have the 'svm' or
> 'vmx' prefix.
> 
> 2) KVM_X86_OP_NULL() - for the case where both svm and vmx want
> to over-ride.
> 
> For the remaining cases I was thinking of having the VMX code
> and svm code, add a define for 'VMX' or 'SVM' respectively and
> then we could mark the vmx specific ones by adding something like
> this to asm/kvm-x86-ops.h:
> 
> #ifndef VMX
>  #define KVM_X86_OP_UNLESS_VMX(func) \
> 	KVM_X86_OP(func)
> #else
>  #define KVM_X86_OP_UNLESS_VMX(func)
> #endif
> 
> and similarly for the svm specific ones:
> 
> #ifndef SVM
>  #define KVM_X86_OP_UNLESS_SVM(func) \
> 	KVM_X86_OP(func)
> #else
>  #define KVM_X86_OP_UNLESS_SVM(func)
> #endif
> 
> 
> So in vmx.c you would have:
> 
> #define X86_OP(func)     .func = vmx_##func,
> #define VMX 1
> #include "kvm-x86-ops.h"
> 
> Or we could just use the KVM_X86_OP_NULL() macro for anything
> that doesn't have a 'svm' or 'vmx' prefix as I think you were
> suggesting? In that case I think there would then be 13 manual
> additions for vmx and 12 for svm.
> 

Sorry this last bit is wrong. Using the KVM_X86_OP_NULL() for
all definitions that don't use 'svm' and 'vmx', would mean
manually defining all 20 in vmx and svm. Whereas if we we
mark some of them specific to vmx or svm, we can get away
with less - 13 manual for vmx and 12 manual for svm.

Thanks,

-Jason