| Message ID | 20211227081515.2088920-2-chao.gao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve KVM's interaction with CPU hotplug | expand |
On Mon, Dec 27, 2021, Chao Gao wrote: > so that KVM can do compatibility checks on hotplugged CPUs. Drop __init > from check_processor_compatibility() and its callees. Losing the __init annotation on all these helpers makes me a bit sad, more from a documentation perspective than a "but we could shave a few bytes" perspective. More than once I've wondered why some bit of code isn't __init, only to realize its used for hotplug. What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y? At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out on !CONFIG_HOTPLUG_CPU. That also give us a bit of test coverage for bots that build with SMP=n. diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index a80e3b0c11a8..30bbcb4f4057 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11380,7 +11380,7 @@ void kvm_arch_hardware_unsetup(void) static_call(kvm_x86_hardware_unsetup)(); } -int kvm_arch_check_processor_compat(void) +int __init_or_hotplug kvm_arch_check_processor_compat(void) { struct cpuinfo_x86 *c = &cpu_data(smp_processor_id()); diff --git a/include/linux/init.h b/include/linux/init.h index d82b4b2e1d25..33788b3c180a 100644 --- a/include/linux/init.h +++ b/include/linux/init.h @@ -53,6 +53,12 @@ #define __exitdata __section(".exit.data") #define __exit_call __used __section(".exitcall.exit") +#ifdef CONFIG_HOTPLUG_CPU +#define __init_or_hotplug +#else +#define __init_or_hotplug __init +#endif /* CONFIG_HOTPLUG_CPU + /* * modpost check for section mismatches during the kernel build. * A section mismatch happens when there are references from a
On Mon, Jan 10, 2022 at 11:27:09PM +0000, Sean Christopherson wrote: >On Mon, Dec 27, 2021, Chao Gao wrote: >> so that KVM can do compatibility checks on hotplugged CPUs. Drop __init >> from check_processor_compatibility() and its callees. > >Losing the __init annotation on all these helpers makes me a bit sad, more from a >documentation perspective than a "but we could shave a few bytes" perspective. This makes sense. >More than once I've wondered why some bit of code isn't __init, only to realize >its used for hotplug. Same problem to some global data structures which can be __initdata if hotplug isn't supported. > >What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y? Personally __init_or_hotplug is a little long as an annotation. How about __hotplug? One concern is: is it acceptable to introduce a new annotation and use it in new code but not fix all places that should use it in existing code. I think the right process is 1. introduce a new annotation 2. fix existing code to use this annotation 3. add new code. There is no doubt that #2 would take great effort. I'm not sure if it is really worth it. >At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out >on !CONFIG_HOTPLUG_CPU. That also give us a bit of test coverage for bots that >build with SMP=n. Will do with your suggested-by. > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >index a80e3b0c11a8..30bbcb4f4057 100644 >--- a/arch/x86/kvm/x86.c >+++ b/arch/x86/kvm/x86.c >@@ -11380,7 +11380,7 @@ void kvm_arch_hardware_unsetup(void) > static_call(kvm_x86_hardware_unsetup)(); > } > >-int kvm_arch_check_processor_compat(void) >+int __init_or_hotplug kvm_arch_check_processor_compat(void) > { > struct cpuinfo_x86 *c = &cpu_data(smp_processor_id()); > >diff --git a/include/linux/init.h b/include/linux/init.h >index d82b4b2e1d25..33788b3c180a 100644 >--- a/include/linux/init.h >+++ b/include/linux/init.h >@@ -53,6 +53,12 @@ > #define __exitdata __section(".exit.data") > #define __exit_call __used __section(".exitcall.exit") > >+#ifdef CONFIG_HOTPLUG_CPU >+#define __init_or_hotplug >+#else >+#define __init_or_hotplug __init >+#endif /* CONFIG_HOTPLUG_CPU >+ > /* > * modpost check for section mismatches during the kernel build. > * A section mismatch happens when there are references from a >
On Tue, Jan 11, 2022, Chao Gao wrote: > On Mon, Jan 10, 2022 at 11:27:09PM +0000, Sean Christopherson wrote: > >On Mon, Dec 27, 2021, Chao Gao wrote: > >> so that KVM can do compatibility checks on hotplugged CPUs. Drop __init > >> from check_processor_compatibility() and its callees. > > > >Losing the __init annotation on all these helpers makes me a bit sad, more from a > >documentation perspective than a "but we could shave a few bytes" perspective. > > This makes sense. > > >More than once I've wondered why some bit of code isn't __init, only to realize > >its used for hotplug. > > Same problem to some global data structures which can be __initdata if hotplug > isn't supported. > > > > >What if we added an __init_or_hotplug annotation that is a nop if HOTPLUG_CPU=y? > > Personally __init_or_hotplug is a little long as an annotation. How about > __hotplug? > > One concern is: is it acceptable to introduce a new annotation and use it in > new code but not fix all places that should use it in existing code. > > I think the right process is > 1. introduce a new annotation > 2. fix existing code to use this annotation > 3. add new code. > > There is no doubt that #2 would take great effort. I'm not sure if it is really > worth it. > > >At a glance, KVM could use that if the guts of kvm_online_cpu() were #idef'd out > >on !CONFIG_HOTPLUG_CPU. That also give us a bit of test coverage for bots that > >build with SMP=n. > > Will do with your suggested-by. I don't think you should try to add a new annotation in this series. My question really was just that, a question to others if there would be value in adding an annotation to identify symbols that are !__init only because of hotplug. Such new functionality is certainly not required for fixing KVM's mishandling of hotplug, and trying to cram it in here will bloat and slow down this series.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 5d97f4adc1cb..0d5dc148090c 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1313,6 +1313,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical) struct kvm_x86_ops { const char *name; + int (*check_processor_compatibility)(void); int (*hardware_enable)(void); void (*hardware_disable)(void); void (*hardware_unsetup)(void); @@ -1513,7 +1514,6 @@ struct kvm_x86_nested_ops { struct kvm_x86_init_ops { int (*cpu_has_kvm_support)(void); int (*disabled_by_bios)(void); - int (*check_processor_compatibility)(void); int (*hardware_setup)(void); struct kvm_x86_ops *runtime_ops; diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 6cb38044a860..74c41a261633 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -3825,7 +3825,7 @@ svm_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall) hypercall[2] = 0xd9; } -static int __init svm_check_processor_compat(void) +static int svm_check_processor_compat(void) { return 0; } @@ -4374,6 +4374,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = { .name = "kvm_amd", .hardware_unsetup = svm_hardware_teardown, + .check_processor_compatibility = svm_check_processor_compat, .hardware_enable = svm_hardware_enable, .hardware_disable = svm_hardware_disable, .cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr, @@ -4743,7 +4744,6 @@ static struct kvm_x86_init_ops svm_init_ops __initdata = { .cpu_has_kvm_support = has_svm, .disabled_by_bios = is_disabled, .hardware_setup = svm_hardware_setup, - .check_processor_compatibility = svm_check_processor_compat, .runtime_ops = &svm_x86_ops, }; diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c index ba6f99f584ac..50f923e9917e 100644 --- a/arch/x86/kvm/vmx/evmcs.c +++ b/arch/x86/kvm/vmx/evmcs.c @@ -296,7 +296,7 @@ const struct evmcs_field vmcs_field_to_evmcs_1[] = { }; const unsigned int nr_evmcs_1_fields = ARRAY_SIZE(vmcs_field_to_evmcs_1); -__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf) +void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf) { vmcs_conf->pin_based_exec_ctrl &= ~EVMCS1_UNSUPPORTED_PINCTRL; vmcs_conf->cpu_based_2nd_exec_ctrl &= ~EVMCS1_UNSUPPORTED_2NDEXEC; diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h index 16731d2cf231..17a7c956396b 100644 --- a/arch/x86/kvm/vmx/evmcs.h +++ b/arch/x86/kvm/vmx/evmcs.h @@ -181,7 +181,7 @@ static inline void evmcs_load(u64 phys_addr) vp_ap->enlighten_vmentry = 1; } -__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf); +void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf); #else /* !IS_ENABLED(CONFIG_HYPERV) */ static __always_inline void evmcs_write64(unsigned long field, u64 value) {} static inline void evmcs_write32(unsigned long field, u32 value) {} diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index fe06b02994e6..de617a7bd966 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2373,8 +2373,8 @@ static bool cpu_has_sgx(void) return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); } -static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, - u32 msr, u32 *result) +static int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, + u32 msr, u32 *result) { u32 vmx_msr_low, vmx_msr_high; u32 ctl = ctl_min | ctl_opt; @@ -2392,8 +2392,8 @@ static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, return 0; } -static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, - struct vmx_capability *vmx_cap) +static int setup_vmcs_config(struct vmcs_config *vmcs_conf, + struct vmx_capability *vmx_cap) { u32 vmx_msr_low, vmx_msr_high; u32 min, opt, min2, opt2; @@ -6990,7 +6990,7 @@ static int vmx_vm_init(struct kvm *kvm) return 0; } -static int __init vmx_check_processor_compat(void) +static int vmx_check_processor_compat(void) { struct vmcs_config vmcs_conf; struct vmx_capability vmx_cap; @@ -7578,6 +7578,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = { .hardware_unsetup = hardware_unsetup, + .check_processor_compatibility = vmx_check_processor_compat, .hardware_enable = hardware_enable, .hardware_disable = hardware_disable, .cpu_has_accelerated_tpr = report_flexpriority, @@ -7911,7 +7912,6 @@ static __init int hardware_setup(void) static struct kvm_x86_init_ops vmx_init_ops __initdata = { .cpu_has_kvm_support = cpu_has_kvm_support, .disabled_by_bios = vmx_disabled_by_bios, - .check_processor_compatibility = vmx_check_processor_compat, .hardware_setup = hardware_setup, .runtime_ops = &vmx_x86_ops, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c194a8cbd25f..6411417b6871 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -11391,7 +11391,7 @@ int kvm_arch_check_processor_compat(void *opaque) __cr4_reserved_bits(cpu_has, &boot_cpu_data)) return -EIO; - return ops->check_processor_compatibility(); + return ops->runtime_ops->check_processor_compatibility(); } bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
so that KVM can do compatibility checks on hotplugged CPUs. Drop __init from check_processor_compatibility() and its callees. Signed-off-by: Chao Gao <chao.gao@intel.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/svm/svm.c | 4 ++-- arch/x86/kvm/vmx/evmcs.c | 2 +- arch/x86/kvm/vmx/evmcs.h | 2 +- arch/x86/kvm/vmx/vmx.c | 12 ++++++------ arch/x86/kvm/x86.c | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-)