On Tue, Apr 22, 2025, Zack Rusin wrote:
> Centralize KVM's VMware specific code and introduce CONFIG_KVM_VMWARE to
> isolate all of it.
I think it makes sense to split this into two patches: one to move code around,
and then one to add CONFIG_KVM_VMWARE. And move _all_ of the code at once, e.g.
enable_vmware_backdoor should be moved to vmware.c along with all the other code
shuffling, not as part of "Allow enabling of the vmware backdoor via a cap".
> Code used to support VMware backdoor has been scattered around the KVM
> codebase making it difficult to reason about, maintain it and change
> it. Introduce CONFIG_KVM_VMWARE which, much like CONFIG_KVM_XEN and
> CONFIG_KVM_VMWARE for Xen and Hyper-V, abstracts away VMware specific
> parts.
>
> In general CONFIG_KVM_VMWARE should be set to y and to preserve the
> current behavior it defaults to Y.
>
> Signed-off-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> Cc: Doug Covelli <doug.covelli@xxxxxxxxxxxx>
> Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: kvm@xxxxxxxxxxxxxxx
> ---
> MAINTAINERS | 9 +++
> arch/x86/kvm/Kconfig | 13 ++++
> arch/x86/kvm/emulate.c | 11 ++--
> arch/x86/kvm/kvm_vmware.h | 127 ++++++++++++++++++++++++++++++++++++++
My vote is to drop the "kvm" from the file name. We have kvm_onhyperv.{c,h} to
identify the case where KVM is running as a Hyper-V guest, but for the case where
KVM is emulating Hyper-V, we use arch/x86/kvm/hyperv.{c,h}.
> arch/x86/kvm/pmu.c | 39 +-----------
> arch/x86/kvm/pmu.h | 4 --
> arch/x86/kvm/svm/svm.c | 7 ++-
> arch/x86/kvm/vmx/vmx.c | 5 +-
> arch/x86/kvm/x86.c | 34 +---------
> arch/x86/kvm/x86.h | 2 -
> 10 files changed, 166 insertions(+), 85 deletions(-)
> create mode 100644 arch/x86/kvm/kvm_vmware.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 00e94bec401e..9e38103ac2bb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13051,6 +13051,15 @@ F: arch/x86/kvm/svm/hyperv.*
> F: arch/x86/kvm/svm/svm_onhyperv.*
> F: arch/x86/kvm/vmx/hyperv.*
>
> +KVM X86 VMware (KVM/VMware)
> +M: Zack Rusin <zack.rusin@xxxxxxxxxxxx>
> +M: Doug Covelli <doug.covelli@xxxxxxxxxxxx>
> +M: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> +L: kvm@xxxxxxxxxxxxxxx
> +S: Supported
> +T: git git://git.kernel.org/pub/scm/virt/kvm/kvm.git
> +F: arch/x86/kvm/kvm_vmware.*
> +
> KVM X86 Xen (KVM/Xen)
> M: David Woodhouse <dwmw2@xxxxxxxxxxxxx>
> M: Paul Durrant <paul@xxxxxxx>
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index ea2c4f21c1ca..9e3be87fc82b 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -178,6 +178,19 @@ config KVM_HYPERV
>
> If unsure, say "Y".
>
> +config KVM_VMWARE
> + bool "Features needed for VMware guests support"
> + depends on KVM
Make this depend on KVM_x86. See:
https://lore.kernel.org/all/20250723104714.1674617-3-tabba@xxxxxxxxxx
> + default y
> + help
> + Provides KVM support for hosting VMware guests. Adds support for
> + VMware legacy backdoor interface: VMware tools and various userspace
> + utilities running in VMware guests sometimes utilize specially
> + formatted IN, OUT and RDPMC instructions which need to be
> + intercepted.
> +
> + If unsure, say "Y".
> +
> config KVM_XEN
> bool "Support for Xen hypercall interface"
> depends on KVM
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 60986f67c35a..b42988ce8043 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -26,6 +26,7 @@
> #include <asm/debugreg.h>
> #include <asm/nospec-branch.h>
> #include <asm/ibt.h>
> +#include "kvm_vmware.h"
Please sort includes as best as possible. KVM's loose rule is to organize by
linux => asm => local, and sort alphabetically within each section, e.g.
#include <linux/aaaa.h>
#include <linux/blah.h>
#include <asm/aaaa.h>
#include <asm/blah.h>
#include "aaaa.h"
#include "blah.h"
> @@ -2565,8 +2563,8 @@ static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt,
> * VMware allows access to these ports even if denied
> * by TSS I/O permission bitmap. Mimic behavior.
> */
> - if (enable_vmware_backdoor &&
> - ((port == VMWARE_PORT_VMPORT) || (port == VMWARE_PORT_VMRPC)))
> + if (kvm_vmware_backdoor_enabled(ctxt->vcpu) &&
Maybe kvm_is_vmware_backdoor_enabled()? To make it super clear it's a predicate.
Regarding namespacing, I think for the "is" predicates, the code reads better if
it's kvm_is_vmware_xxx versus kvm_vware_is_xxx. E.g. is the VMware backdoor
enabled vs. VMware is the backdoor enabled. Either way is fine for me if someone
has a strong preference though.
> + kvm_vmware_io_port_allowed(port))
Please separate the addition of helpers from the code movement. That way the
code movement patch can be acked/reviewed super easily, and then we can focus on
the helpers (and it also makes it much easier to review the helpers changes).
E.g.
patch 1: move code to vmware.{c,h}
patch 2: introduce wrappers and bury variables/#defines in vmware.c
patch 3: introduce CONFIG_KVM_VMWARE to disasble VMware emulation
I mention that here, because kvm_vmware_io_port_allowed() doesn't seem like the
right name. kvm_is_vmware_io_port() seems more appropriate.
Oh, and also relevant. For this and kvm_vmware_is_backdoor_pmc(), put the
kvm_vmware_backdoor_enabled() check inside kvm_is_vmware_io_port() and
kvm_is_vmware_backdoor_pmc().
