Re: [PATCH v2 1/5] KVM: x86: Centralize KVM's VMware code

[Sean Christopherson <seanjc@google.com>]

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@broadcom.com>
> Cc: Doug Covelli <doug.covelli@broadcom.com>
> Cc: Sean Christopherson <seanjc@google.com>
> 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: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Zack Rusin <zack.rusin@broadcom.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: kvm@vger.kernel.org
> ---
>  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@broadcom.com>
> +M:	Doug Covelli <doug.covelli@broadcom.com>
> +M:	Paolo Bonzini <pbonzini@redhat.com>
> +L:	kvm@vger.kernel.org
> +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@infradead.org>
>  M:	Paul Durrant <paul@xen.org>
> 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@google.com

> +	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().