Messages in this thread | | | Date | Fri, 4 Nov 2022 08:59:05 +0100 | Subject | Re: [PATCH 00/44] KVM: Rework kvm_init() and hardware enabling | From | Paolo Bonzini <> |
| |
On 11/4/22 08:17, Isaku Yamahata wrote: > On Wed, Nov 02, 2022 at 11:18:27PM +0000, > Sean Christopherson <seanjc@google.com> wrote: > >> Non-x86 folks, please test on hardware when possible. I made a _lot_ of >> mistakes when moving code around. Thankfully, x86 was the trickiest code >> to deal with, and I'm fairly confident that I found all the bugs I >> introduced via testing. But the number of mistakes I made and found on >> x86 makes me more than a bit worried that I screwed something up in other >> arch code. >> >> This is a continuation of Chao's series to do x86 CPU compatibility checks >> during virtualization hardware enabling[1], and of Isaku's series to try >> and clean up the hardware enabling paths so that x86 (Intel specifically) >> can temporarily enable hardware during module initialization without >> causing undue pain for other architectures[2]. It also includes one patch >> from another mini-series from Isaku that provides the less controversial >> patches[3]. >> >> The main theme of this series is to kill off kvm_arch_init(), >> kvm_arch_hardware_(un)setup(), and kvm_arch_check_processor_compat(), which >> all originated in x86 code from way back when, and needlessly complicate >> both common KVM code and architecture code. E.g. many architectures don't >> mark functions/data as __init/__ro_after_init purely because kvm_init() >> isn't marked __init to support x86's separate vendor modules. >> >> The idea/hope is that with those hooks gone (moved to arch code), it will >> be easier for x86 (and other architectures) to modify their module init >> sequences as needed without having to fight common KVM code. E.g. I'm >> hoping that ARM can build on this to simplify its hardware enabling logic, >> especially the pKVM side of things. >> >> There are bug fixes throughout this series. They are more scattered than >> I would usually prefer, but getting the sequencing correct was a gigantic >> pain for many of the x86 fixes due to needing to fix common code in order >> for the x86 fix to have any meaning. And while the bugs are often fatal, >> they aren't all that interesting for most users as they either require a >> malicious admin or broken hardware, i.e. aren't likely to be encountered >> by the vast majority of KVM users. So unless someone _really_ wants a >> particular fix isolated for backporting, I'm not planning on shuffling >> patches. >> >> Tested on x86. Lightly tested on arm64. Compile tested only on all other >> architectures. > > Thanks for the patch series. I the rebased TDX KVM patch series and it worked. > Since cpu offline needs to be rejected in some cases(To keep at least one cpu > on a package), arch hook for cpu offline is needed. > I can keep it in TDX KVM patch series.
Yes, this patch looks good.
Paolo
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h > index 23c0f4bc63f1..ef7bcb845d42 100644 > --- a/arch/x86/include/asm/kvm-x86-ops.h > +++ b/arch/x86/include/asm/kvm-x86-ops.h > @@ -17,6 +17,7 @@ BUILD_BUG_ON(1) > KVM_X86_OP(hardware_enable) > KVM_X86_OP(hardware_disable) > KVM_X86_OP(hardware_unsetup) > +KVM_X86_OP_OPTIONAL_RET0(offline_cpu) > KVM_X86_OP(has_emulated_msr) > KVM_X86_OP(vcpu_after_set_cpuid) > KVM_X86_OP(is_vm_type_supported) > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 496c7c6eaff9..c420409aa96f 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1468,6 +1468,7 @@ struct kvm_x86_ops { > int (*hardware_enable)(void); > void (*hardware_disable)(void); > void (*hardware_unsetup)(void); > + int (*offline_cpu)(void); > bool (*has_emulated_msr)(struct kvm *kvm, u32 index); > void (*vcpu_after_set_cpuid)(struct kvm_vcpu *vcpu); > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2ed5a017f7bc..17c5d6a76c93 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -12039,6 +12039,11 @@ void kvm_arch_hardware_disable(void) > drop_user_return_notifiers(); > } > > +int kvm_arch_offline_cpu(unsigned int cpu) > +{ > + return static_call(kvm_x86_offline_cpu)(); > +} > + > bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu) > { > return vcpu->kvm->arch.bsp_vcpu_id == vcpu->vcpu_id; > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 620489b9aa93..4df79443fd11 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1460,6 +1460,7 @@ static inline void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu) {} > int kvm_arch_hardware_enable(void); > void kvm_arch_hardware_disable(void); > #endif > +int kvm_arch_offline_cpu(unsigned int cpu); > int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu); > bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu); > int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index f6b6dcedaa0a..f770fdc662d0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -5396,16 +5396,24 @@ static void hardware_disable_nolock(void *junk) > __this_cpu_write(hardware_enabled, false); > } > > +__weak int kvm_arch_offline_cpu(unsigned int cpu) > +{ > + return 0; > +} > + > static int kvm_offline_cpu(unsigned int cpu) > { > + int r = 0; > + > mutex_lock(&kvm_lock); > - if (kvm_usage_count) { > + r = kvm_arch_offline_cpu(cpu); > + if (!r && kvm_usage_count) { > preempt_disable(); > hardware_disable_nolock(NULL); > preempt_enable(); > } > mutex_unlock(&kvm_lock); > - return 0; > + return r; > } > > static void hardware_disable_all_nolock(void) >
| |