| Message ID | cover.1663869838.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: hardware enable/disable reorganize | expand |
On Thu, Sep 22, 2022, isaku.yamahata@intel.com wrote: > From: Isaku Yamahata <isaku.yamahata@intel.com> > > This patch series is to implement the suggestion by Sean Christopherson [1] > to reorganize enable/disable cpu virtualization feature by replacing > the arch-generic current enable/disable logic with PM related hooks. And > convert kvm/x86 to use new hooks. Thanks for putting this together, actually seeing code is super helpful! Unfortunately, after seeing the code, I think my suggestion was a bad one. At the end of this series, there's a rather gross amount of duplicate code between x86 and common KVM, and no clear line of sight to improving things. Even if we move ARM, s390, and PPC away from the generic hooks, MIPS and RISC-V still need the generic implementation, i.e. we'll still have duplicate code. Rather than force arch code to implement most/all power management hooks, I think we can achieve a similar outcome (let ARM do its own thing, turn s390 and PPC into nops) by wrapping the hardware enable/disable (and thus PM code) in a Kconfig, e.g. KVM_GENERIC_HARDWARE_ENABLING. I'll throw together a rough prototype tomorrow (got partway through and then got distracted by other stuff) and hopefully post an RFC series. Thanks again!
On Thu, Oct 13, 2022, Sean Christopherson wrote: > On Thu, Sep 22, 2022, isaku.yamahata@intel.com wrote: > > From: Isaku Yamahata <isaku.yamahata@intel.com> > > > > This patch series is to implement the suggestion by Sean Christopherson [1] > > to reorganize enable/disable cpu virtualization feature by replacing > > the arch-generic current enable/disable logic with PM related hooks. And > > convert kvm/x86 to use new hooks. > > Thanks for putting this together, actually seeing code is super helpful! > > Unfortunately, after seeing the code, I think my suggestion was a bad one. At > the end of this series, there's a rather gross amount of duplicate code between > x86 and common KVM, and no clear line of sight to improving things. > > Even if we move ARM, s390, and PPC away from the generic hooks, MIPS and RISC-V > still need the generic implementation, i.e. we'll still have duplicate code. > > Rather than force arch code to implement most/all power management hooks, I think > we can achieve a similar outcome (let ARM do its own thing, turn s390 and PPC into > nops) by wrapping the hardware enable/disable (and thus PM code) in a Kconfig, > e.g. KVM_GENERIC_HARDWARE_ENABLING. > > I'll throw together a rough prototype tomorrow (got partway through and then got > distracted by other stuff) and hopefully post an RFC series. Good news and bad news. Bad news: The KVM_GENERIC_HARDWARE_ENABLING idea is a bit of a bust. It works, but it's little more than a nice-to-have for s390 and PPC. Good news (from a certain point of view): The reason that the "generic h/w enabling" doesn't help much is because after sorting out the myriad issues in KVM initialization, which is even more of a bug-ridden mess than I initially thought, kvm_init() no longer has _any_ arch hooks. All the compat checks and whatnot are handled in x86, so tweaking those for TDX should be easier, or at the very least, we should have more options. Sorting everything out is proving to be a nightmare, but I think I have the initial coding done. Testing will be fun. It'll likely be next week before I can post anything.
On 10/14/22 06:04, Sean Christopherson wrote: > Good news (from a certain point of view): > The reason that the "generic h/w enabling" doesn't help much is because after sorting > out the myriad issues in KVM initialization, which is even more of a bug-ridden mess > than I initially thought, kvm_init() no longer has_any_ arch hooks. All the compat > checks and whatnot are handled in x86, so tweaking those for TDX should be easier, > or at the very least, we should have more options. > > Sorting everything out is proving to be a nightmare, but I think I have the initial > coding done. Testing will be fun. It'll likely be next week before I can post > anything. Hi Sean, is this the same series that you mentioned a couple hours ago in reply to Isaku? Thanks, Paolo
On Wed, Nov 02, 2022, Paolo Bonzini wrote: > On 10/14/22 06:04, Sean Christopherson wrote: > > Good news (from a certain point of view): > > The reason that the "generic h/w enabling" doesn't help much is because after sorting > > out the myriad issues in KVM initialization, which is even more of a bug-ridden mess > > than I initially thought, kvm_init() no longer has_any_ arch hooks. All the compat > > checks and whatnot are handled in x86, so tweaking those for TDX should be easier, > > or at the very least, we should have more options. > > > > Sorting everything out is proving to be a nightmare, but I think I have the initial > > coding done. Testing will be fun. It'll likely be next week before I can post > > anything. > > Hi Sean, > > is this the same series that you mentioned a couple hours ago in reply to > Isaku? Yep.
From: Isaku Yamahata <isaku.yamahata@intel.com> I've updated the patch series based on Chao's review and eliminated kvm_arch_pre_hardware_unsetup() and compatibility check on resume. Those two patch are compile only tested. - "KVM: arm64: Simplify the CPUHP logic" - "RFC: KVM: powerpc: Move processor compatibility check to hardware setup" - "RFC: KVM: Remove cpus_hardware_enabled and related sanity check" Changes from v4: - reorder/reorganize the patch. Especially introduced one patch for each PM events. reboot, suspend, resume, and cpu online/offline. - eliminated kvm_arch_pre_hardware_unsetup(). - removed compatibility check on resume - "KVM: arm64: Simplify the CPUHP logic": fixed up signed-off-by and the link. - "RFC: KVM: powerpc: Move processor compatibility check to hardware setup" move the comment in the code to the commit message. This patch series is to implement the suggestion by Sean Christopherson [1] to reorganize enable/disable cpu virtualization feature by replacing the arch-generic current enable/disable logic with PM related hooks. And convert kvm/x86 to use new hooks. - Untable x86 hardware enable logic, snapshot MSRs for user return notifier, enabling cpu virtualization on cpu online and platform resume. and real enabling of CPU virtualization feature - Introduce hooks related to PM. - Convert kvm/x86 code to user those hooks. - Split out hardware enabling/disabling logic into a separate file. Compile it for non-x86 code. Once conversion of other KVM archs is done, this file can be dropped. - Delete kvm_arch_check_processor_compat() - Delete cpus_hardware_enabled. 29/30 and 30/30 [1] https://lore.kernel.org/kvm/YvU+6fdkHaqQiKxp@google.com/ [2] https://lore.kernel.org/all/20220216031528.92558-1-chao.gao@intel.com/ Changes from v3: - Updated "KVM: arm64: Simplify the CPUHP logic". - add preempt_disable/enable() around hardware_enable/disable() to keep the assumption of the arch callback. - Eliminated arch compat check callback, kvm_arch_check_processor_compat(). Changes from v2: - Replace the first patch("KVM: x86: Drop kvm_user_return_msr_cpu_online()") with Sean's implementation - Included all patches of "Improve KVM's interaction with CPU hotplug" [2] Until v2, Tried to cherry-pick the least patches of it. It turned out that all the patches are desirable. Changes from v1: - Add a patch "KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section" to make online/offline callback to run thread context to use mutex instead of spin lock - fixes pointed by Chao Gao Chao Gao (4): KVM: x86: Move check_processor_compatibility from init ops to runtime ops Partially revert "KVM: Pass kvm_init()'s opaque param to additional arch funcs" KVM: Rename and move CPUHP_AP_KVM_STARTING to ONLINE section KVM: Do compatibility checks on hotplugged CPUs Isaku Yamahata (24): KVM: x86: Use this_cpu_ptr() instead of per_cpu_ptr(smp_processor_id()) KVM: Provide more information in kernel log if hardware enabling fails KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock KVM: Add arch hooks when VM is added/deleted KVM: Add arch hook for reboot event KVM: Add arch hook for suspend KVM: Add arch hook for resume event KVM: Add arch hook for cpu online event KVM: Add arch hook for cpu offline event KVM: Remove on_each_cpu(hardware_disable_nolock) in kvm_exit() KVM: Move out KVM arch PM hooks and hardware enable/disable logic KVM: kvm_arch.c: Remove _nolock post fix KVM: kvm_arch.c: Remove a global variable, hardware_enable_failed KVM: Introduce an arch wrapper to check all processor compatibility KVM: x86: Duplicate arch callbacks related to pm events and compat check KVM: x86: Move TSC fixup logic to KVM arch resume callback KVM: Eliminate kvm_arch_post_init_vm() KVM: Add config to not compile kvm_arch.c KVM: x86: Delete kvm_arch_hardware_enable/disable() KVM: x86: Make x86 processor compat check callback empty RFC: KVM: powerpc: Move processor compatibility check to hardware setup KVM: Eliminate kvm_arch_check_processor_compat() RFC: KVM: x86: Remove cpus_hardware_enabled and related sanity check RFC: KVM: Remove cpus_hardware_enabled and related sanity check Marc Zyngier (1): KVM: arm64: Simplify the CPUHP logic Sean Christopherson (1): KVM: x86: Drop kvm_user_return_msr_cpu_online() Documentation/virt/kvm/locking.rst | 14 +- arch/arm64/kvm/arch_timer.c | 27 ++-- arch/arm64/kvm/arm.c | 18 ++- arch/arm64/kvm/vgic/vgic-init.c | 19 +-- arch/mips/kvm/mips.c | 5 - arch/powerpc/kvm/powerpc.c | 5 - arch/riscv/kvm/main.c | 5 - arch/s390/kvm/kvm-s390.c | 5 - arch/x86/include/asm/kvm-x86-ops.h | 1 + arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/Kconfig | 1 + arch/x86/kvm/svm/svm.c | 4 +- arch/x86/kvm/vmx/vmx.c | 24 ++-- arch/x86/kvm/x86.c | 201 ++++++++++++++++++++++++---- include/kvm/arm_arch_timer.h | 4 + include/kvm/arm_vgic.h | 4 + include/linux/cpuhotplug.h | 5 +- include/linux/kvm_host.h | 13 +- virt/kvm/Kconfig | 3 + virt/kvm/Makefile.kvm | 3 + virt/kvm/kvm_arch.c | 132 +++++++++++++++++++ virt/kvm/kvm_main.c | 205 +++++++++-------------------- 22 files changed, 443 insertions(+), 257 deletions(-) create mode 100644 virt/kvm/kvm_arch.c base-commit: 521a547ced6477c54b4b0cc206000406c221b4d6