| Message ID | ce483ce4a1920a3c1c4e5deea11648d75f2a7b80.1610379877.git.jbaron@akamai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use static_call for kvm_x86_ops | expand |
On Mon, Jan 11, 2021, Jason Baron wrote: > Use static calls to improve kvm_x86_ops performance. Introduce the > definitions that will be used by a subsequent patch to actualize the > savings. > > Note that all kvm_x86_ops are covered here except for 'pmu_ops' and > 'nested ops'. I think they can be covered by static calls in a simlilar > manner, but were omitted from this series to reduce scope and because > I don't think they have as large of a performance impact. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Signed-off-by: Jason Baron <jbaron@akamai.com> > --- > arch/x86/include/asm/kvm_host.h | 65 +++++++++++++++++++++++++++++++++++++++++ > arch/x86/kvm/x86.c | 5 ++++ > 2 files changed, 70 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 3ab7b46..e947522 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1087,6 +1087,65 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical) > return dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL; > } > > +/* > + * static calls cover all kvm_x86_ops except for functions under pmu_ops and > + * nested_ops. > + */ > +#define FOREACH_KVM_X86_OPS(F) \ > + F(hardware_enable); F(hardware_disable); F(hardware_unsetup); \ > + F(cpu_has_accelerated_tpr); F(has_emulated_msr); \ > + F(vcpu_after_set_cpuid); F(vm_init); F(vm_destroy); F(vcpu_create); \ > + F(vcpu_free); F(vcpu_reset); F(prepare_guest_switch); F(vcpu_load); \ > + F(vcpu_put); F(update_exception_bitmap); F(get_msr); F(set_msr); \ > + F(get_segment_base); F(get_segment); F(get_cpl); F(set_segment); \ > + F(get_cs_db_l_bits); F(set_cr0); F(is_valid_cr4); F(set_cr4); \ > + F(set_efer); F(get_idt); F(set_idt); F(get_gdt); F(set_gdt); \ > + F(sync_dirty_debug_regs); F(set_dr7); F(cache_reg); F(get_rflags); \ > + F(set_rflags); F(tlb_flush_all); F(tlb_flush_current); \ > + F(tlb_remote_flush); F(tlb_remote_flush_with_range); F(tlb_flush_gva); \ > + F(tlb_flush_guest); F(run); F(handle_exit); \ > + F(skip_emulated_instruction); F(update_emulated_instruction); \ > + F(set_interrupt_shadow); F(get_interrupt_shadow); F(patch_hypercall); \ > + F(set_irq); F(set_nmi); F(queue_exception); F(cancel_injection); \ > + F(interrupt_allowed); F(nmi_allowed); F(get_nmi_mask); F(set_nmi_mask);\ > + F(enable_nmi_window); F(enable_irq_window); F(update_cr8_intercept); \ > + F(check_apicv_inhibit_reasons); F(pre_update_apicv_exec_ctrl); \ > + F(refresh_apicv_exec_ctrl); F(hwapic_irr_update); F(hwapic_isr_update);\ > + F(guest_apic_has_interrupt); F(load_eoi_exitmap); \ > + F(set_virtual_apic_mode); F(set_apic_access_page_addr); \ > + F(deliver_posted_interrupt); F(sync_pir_to_irr); F(set_tss_addr); \ > + F(set_identity_map_addr); F(get_mt_mask); F(load_mmu_pgd); \ > + F(has_wbinvd_exit); F(write_l1_tsc_offset); F(get_exit_info); \ > + F(check_intercept); F(handle_exit_irqoff); F(request_immediate_exit); \ > + F(sched_in); F(slot_enable_log_dirty); F(slot_disable_log_dirty); \ > + F(flush_log_dirty); F(enable_log_dirty_pt_masked); \ > + F(cpu_dirty_log_size); F(pre_block); F(post_block); F(vcpu_blocking); \ > + F(vcpu_unblocking); F(update_pi_irte); F(apicv_post_state_restore); \ > + F(dy_apicv_has_pending_interrupt); F(set_hv_timer); F(cancel_hv_timer);\ > + F(setup_mce); F(smi_allowed); F(pre_enter_smm); F(pre_leave_smm); \ > + F(enable_smi_window); F(mem_enc_op); F(mem_enc_reg_region); \ > + F(mem_enc_unreg_region); F(get_msr_feature); \ > + F(can_emulate_instruction); F(apic_init_signal_blocked); \ > + F(enable_direct_tlbflush); F(migrate_timers); F(msr_filter_changed); \ > + F(complete_emulated_msr) What about adding a dedicated .h file for this beast? Then it won't be so painful to do one function per line. As is, updates to kvm_x86_ops will be messy. And add yet another macro layer (or maybe just tweak this one?) so that the caller controls the line ending? I suppose you could also just use a comma, but that's a bit dirty... That would also allow using this to declare vmx_x86_ops and svm_x86_ops, which would need a comma insteat of a semi-colon. There have a been a few attempts to add a bit of automation to {vmx,svm}_x86_ops, this seems like it would be good motivation to go in a different direction and declare/define all ops, e.g. the VMX/SVM code could simply do something like: #define DECLARE_VMX_X86_OP(func) \ .func = vmx_##func static struct kvm_x86_ops vmx_x86_ops __initdata = { .vm_size = sizeof(struct kvm_vmx), .vm_init = vmx_vm_init, .pmu_ops = &intel_pmu_ops, .nested_ops = &vmx_nested_ops, FOREACH_KVM_X86_OPS(DECLARE_VMX_X86_OP) };
On 1/12/21 6:04 PM, Sean Christopherson wrote: > On Mon, Jan 11, 2021, Jason Baron wrote: >> Use static calls to improve kvm_x86_ops performance. Introduce the >> definitions that will be used by a subsequent patch to actualize the >> savings. >> >> Note that all kvm_x86_ops are covered here except for 'pmu_ops' and >> 'nested ops'. I think they can be covered by static calls in a simlilar >> manner, but were omitted from this series to reduce scope and because >> I don't think they have as large of a performance impact. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Borislav Petkov <bp@alien8.de> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Andrea Arcangeli <aarcange@redhat.com> >> Signed-off-by: Jason Baron <jbaron@akamai.com> >> --- >> arch/x86/include/asm/kvm_host.h | 65 +++++++++++++++++++++++++++++++++++++++++ >> arch/x86/kvm/x86.c | 5 ++++ >> 2 files changed, 70 insertions(+) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index 3ab7b46..e947522 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -1087,6 +1087,65 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical) >> return dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL; >> } >> >> +/* >> + * static calls cover all kvm_x86_ops except for functions under pmu_ops and >> + * nested_ops. >> + */ >> +#define FOREACH_KVM_X86_OPS(F) \ >> + F(hardware_enable); F(hardware_disable); F(hardware_unsetup); \ >> + F(cpu_has_accelerated_tpr); F(has_emulated_msr); \ >> + F(vcpu_after_set_cpuid); F(vm_init); F(vm_destroy); F(vcpu_create); \ >> + F(vcpu_free); F(vcpu_reset); F(prepare_guest_switch); F(vcpu_load); \ >> + F(vcpu_put); F(update_exception_bitmap); F(get_msr); F(set_msr); \ >> + F(get_segment_base); F(get_segment); F(get_cpl); F(set_segment); \ >> + F(get_cs_db_l_bits); F(set_cr0); F(is_valid_cr4); F(set_cr4); \ >> + F(set_efer); F(get_idt); F(set_idt); F(get_gdt); F(set_gdt); \ >> + F(sync_dirty_debug_regs); F(set_dr7); F(cache_reg); F(get_rflags); \ >> + F(set_rflags); F(tlb_flush_all); F(tlb_flush_current); \ >> + F(tlb_remote_flush); F(tlb_remote_flush_with_range); F(tlb_flush_gva); \ >> + F(tlb_flush_guest); F(run); F(handle_exit); \ >> + F(skip_emulated_instruction); F(update_emulated_instruction); \ >> + F(set_interrupt_shadow); F(get_interrupt_shadow); F(patch_hypercall); \ >> + F(set_irq); F(set_nmi); F(queue_exception); F(cancel_injection); \ >> + F(interrupt_allowed); F(nmi_allowed); F(get_nmi_mask); F(set_nmi_mask);\ >> + F(enable_nmi_window); F(enable_irq_window); F(update_cr8_intercept); \ >> + F(check_apicv_inhibit_reasons); F(pre_update_apicv_exec_ctrl); \ >> + F(refresh_apicv_exec_ctrl); F(hwapic_irr_update); F(hwapic_isr_update);\ >> + F(guest_apic_has_interrupt); F(load_eoi_exitmap); \ >> + F(set_virtual_apic_mode); F(set_apic_access_page_addr); \ >> + F(deliver_posted_interrupt); F(sync_pir_to_irr); F(set_tss_addr); \ >> + F(set_identity_map_addr); F(get_mt_mask); F(load_mmu_pgd); \ >> + F(has_wbinvd_exit); F(write_l1_tsc_offset); F(get_exit_info); \ >> + F(check_intercept); F(handle_exit_irqoff); F(request_immediate_exit); \ >> + F(sched_in); F(slot_enable_log_dirty); F(slot_disable_log_dirty); \ >> + F(flush_log_dirty); F(enable_log_dirty_pt_masked); \ >> + F(cpu_dirty_log_size); F(pre_block); F(post_block); F(vcpu_blocking); \ >> + F(vcpu_unblocking); F(update_pi_irte); F(apicv_post_state_restore); \ >> + F(dy_apicv_has_pending_interrupt); F(set_hv_timer); F(cancel_hv_timer);\ >> + F(setup_mce); F(smi_allowed); F(pre_enter_smm); F(pre_leave_smm); \ >> + F(enable_smi_window); F(mem_enc_op); F(mem_enc_reg_region); \ >> + F(mem_enc_unreg_region); F(get_msr_feature); \ >> + F(can_emulate_instruction); F(apic_init_signal_blocked); \ >> + F(enable_direct_tlbflush); F(migrate_timers); F(msr_filter_changed); \ >> + F(complete_emulated_msr) > What about adding a dedicated .h file for this beast? Then it won't be so > painful to do one function per line. As is, updates to kvm_x86_ops will be > messy. Hi, I'm fine moving it to a new header. I had put it right above 'struct kvm_x86_ops' in arch/x86/include/asm/kvm_host.h, such that it would be updated along with kvm_x86_ops changes. But I can put a big comment there to update the new header. > > And add yet another macro layer (or maybe just tweak this one?) so that the > caller controls the line ending? I suppose you could also just use a comma, but > that's a bit dirty... > > That would also allow using this to declare vmx_x86_ops and svm_x86_ops, which > would need a comma insteat of a semi-colon. There have a been a few attempts to > add a bit of automation to {vmx,svm}_x86_ops, this seems like it would be good > motivation to go in a different direction and declare/define all ops, e.g. the > VMX/SVM code could simply do something like: > > #define DECLARE_VMX_X86_OP(func) \ > .func = vmx_##func > > static struct kvm_x86_ops vmx_x86_ops __initdata = { > .vm_size = sizeof(struct kvm_vmx), > .vm_init = vmx_vm_init, > > .pmu_ops = &intel_pmu_ops, > .nested_ops = &vmx_nested_ops, > > FOREACH_KVM_X86_OPS(DECLARE_VMX_X86_OP) > }; > Looking at the vmx definitions I see quite a few that don't match that naming. For example: hardware_unsetup, hardware_enable, hardware_disable, report_flexpriority, update_exception_bitmap, enable_nmi_window, enable_irq_window, update_cr8_intercept, pi_has_pending_interrupt, cpu_has_vmx_wbinvd_exit, pi_update_irte, kvm_complete_insn_gp, So I'm not sure if we want to extend these macros to vmx/svm. Thanks, -Jason
On 13/01/21 05:12, Jason Baron wrote: >> > Looking at the vmx definitions I see quite a few that don't > match that naming. For example: > > hardware_unsetup, > hardware_enable, > hardware_disable, > report_flexpriority, > update_exception_bitmap, > enable_nmi_window, > enable_irq_window, > update_cr8_intercept, > pi_has_pending_interrupt, > cpu_has_vmx_wbinvd_exit, > pi_update_irte, > kvm_complete_insn_gp, > > So I'm not sure if we want to extend these macros to > vmx/svm. Don't do it yourself, but once you introduce the new header it becomes a no-brainer to switch the declarations to use it. So let's plan the new header to make that switch easy. Using trailing commas unconditionally would be okay, i.e. #define X86_OP(func) .func = vmx_##func, #include "kvm-x86-ops.h" and leave out the terminator/delimiter in kvm-x86-ops.h. This is similar to how we use vmx/vmcs_shadow_fields.h: #define SHADOW_FIELD_RO(x, y) { x, offsetof(struct vmcs12, y) }, #include "vmcs_shadow_fields.h" #define SHADOW_FIELD_RW(x, y) case x: #include "vmcs_shadow_fields.h" Thanks, Paolo
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? > +#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. Paolo
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. Thanks, -Jason
On Wed, Jan 13, 2021, Paolo Bonzini wrote: > On 13/01/21 05:12, Jason Baron wrote: > > > > > Looking at the vmx definitions I see quite a few that don't > > match that naming. For example: > > > > hardware_unsetup, > > hardware_enable, > > hardware_disable, > > report_flexpriority, > > update_exception_bitmap, > > enable_nmi_window, > > enable_irq_window, > > update_cr8_intercept, > > pi_has_pending_interrupt, > > cpu_has_vmx_wbinvd_exit, > > pi_update_irte, > > kvm_complete_insn_gp, > > > > So I'm not sure if we want to extend these macros to > > vmx/svm. > > Don't do it yourself, but once you introduce the new header it becomes a > no-brainer to switch the declarations to use it. So let's plan the new > header to make that switch easy. Ya, sorry if I didn't make it clear that the vmx/svm conversion is firmly out of scope for this series.
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
On 13/01/21 17:16, 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. I just misread define vs. declare. > 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? > > 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 Yes, that's the simplest thing to do. Then we clean them up as we rename the functions. If you want, go ahead and rename the five easy ones yourself. Paolo > .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,
On Wed, Jan 13, 2021, Jason Baron wrote: > > On 1/13/21 7:53 AM, Paolo Bonzini wrote: > > #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: We'll rename the helpers when doing the conversion, i.e. you can safely assume that all VMX/SVM functions will use the pattern {vmx,svm}_##func. I did all the renaming a few months back, but it got tossed away when svm.c was broken up. > .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,
On Wed, Jan 13, 2021, Paolo Bonzini wrote: > 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, \ Gah, DECLARE_STATIC_CALL_NULL doesn't exist, though it's referenced in a comment. I assume these should be s/DECLARE/DEFINE? I haven't fully grokked the static call code yet... > *(((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. > > Paolo >
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 3ab7b46..e947522 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1087,6 +1087,65 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical) return dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL; } +/* + * static calls cover all kvm_x86_ops except for functions under pmu_ops and + * nested_ops. + */ +#define FOREACH_KVM_X86_OPS(F) \ + F(hardware_enable); F(hardware_disable); F(hardware_unsetup); \ + F(cpu_has_accelerated_tpr); F(has_emulated_msr); \ + F(vcpu_after_set_cpuid); F(vm_init); F(vm_destroy); F(vcpu_create); \ + F(vcpu_free); F(vcpu_reset); F(prepare_guest_switch); F(vcpu_load); \ + F(vcpu_put); F(update_exception_bitmap); F(get_msr); F(set_msr); \ + F(get_segment_base); F(get_segment); F(get_cpl); F(set_segment); \ + F(get_cs_db_l_bits); F(set_cr0); F(is_valid_cr4); F(set_cr4); \ + F(set_efer); F(get_idt); F(set_idt); F(get_gdt); F(set_gdt); \ + F(sync_dirty_debug_regs); F(set_dr7); F(cache_reg); F(get_rflags); \ + F(set_rflags); F(tlb_flush_all); F(tlb_flush_current); \ + F(tlb_remote_flush); F(tlb_remote_flush_with_range); F(tlb_flush_gva); \ + F(tlb_flush_guest); F(run); F(handle_exit); \ + F(skip_emulated_instruction); F(update_emulated_instruction); \ + F(set_interrupt_shadow); F(get_interrupt_shadow); F(patch_hypercall); \ + F(set_irq); F(set_nmi); F(queue_exception); F(cancel_injection); \ + F(interrupt_allowed); F(nmi_allowed); F(get_nmi_mask); F(set_nmi_mask);\ + F(enable_nmi_window); F(enable_irq_window); F(update_cr8_intercept); \ + F(check_apicv_inhibit_reasons); F(pre_update_apicv_exec_ctrl); \ + F(refresh_apicv_exec_ctrl); F(hwapic_irr_update); F(hwapic_isr_update);\ + F(guest_apic_has_interrupt); F(load_eoi_exitmap); \ + F(set_virtual_apic_mode); F(set_apic_access_page_addr); \ + F(deliver_posted_interrupt); F(sync_pir_to_irr); F(set_tss_addr); \ + F(set_identity_map_addr); F(get_mt_mask); F(load_mmu_pgd); \ + F(has_wbinvd_exit); F(write_l1_tsc_offset); F(get_exit_info); \ + F(check_intercept); F(handle_exit_irqoff); F(request_immediate_exit); \ + F(sched_in); F(slot_enable_log_dirty); F(slot_disable_log_dirty); \ + F(flush_log_dirty); F(enable_log_dirty_pt_masked); \ + F(cpu_dirty_log_size); F(pre_block); F(post_block); F(vcpu_blocking); \ + F(vcpu_unblocking); F(update_pi_irte); F(apicv_post_state_restore); \ + F(dy_apicv_has_pending_interrupt); F(set_hv_timer); F(cancel_hv_timer);\ + F(setup_mce); F(smi_allowed); F(pre_enter_smm); F(pre_leave_smm); \ + F(enable_smi_window); F(mem_enc_op); F(mem_enc_reg_region); \ + F(mem_enc_unreg_region); F(get_msr_feature); \ + F(can_emulate_instruction); F(apic_init_signal_blocked); \ + F(enable_direct_tlbflush); F(migrate_timers); F(msr_filter_changed); \ + F(complete_emulated_msr) + +#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) + +#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(); +} + #define __KVM_HAVE_ARCH_VM_ALLOC static inline struct kvm *kvm_arch_alloc_vm(void) { diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3f7c1fc..6ae32ab 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -113,6 +113,11 @@ static int sync_regs(struct kvm_vcpu *vcpu); struct kvm_x86_ops kvm_x86_ops __read_mostly; EXPORT_SYMBOL_GPL(kvm_x86_ops); +DEFINE_KVM_OPS_STATIC_CALLS(); +EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits); +EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg); +EXPORT_STATIC_CALL_GPL(kvm_x86_tlb_flush_current); + static bool __read_mostly ignore_msrs = 0; module_param(ignore_msrs, bool, S_IRUGO | S_IWUSR);
Use static calls to improve kvm_x86_ops performance. Introduce the definitions that will be used by a subsequent patch to actualize the savings. Note that all kvm_x86_ops are covered here except for 'pmu_ops' and 'nested ops'. I think they can be covered by static calls in a simlilar manner, but were omitted from this series to reduce scope and because I don't think they have as large of a performance impact. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Jason Baron <jbaron@akamai.com> --- arch/x86/include/asm/kvm_host.h | 65 +++++++++++++++++++++++++++++++++++++++++ arch/x86/kvm/x86.c | 5 ++++ 2 files changed, 70 insertions(+)