KVM: Improve VMware guest support

Centralize KVM's VMware specific code and introduce CONFIG_KVM_VMWARE to
isolate all of it.

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

+static inline bool kvm_vmware_is_backdoor_opcode(u8 opcode_len, u8
b)

arch/x86/kvm/kvm_vmware.h new file +33 -0

+{
+	switch (opcode_len) {
+	case 1:
+		switch (b) {
+		case 0xe4:	/* IN */
+		case 0xe5:
+		case 0xec:
+		case 0xed:
+		case 0xe6:	/* OUT */
+		case 0xe7:
+		case 0xee:
+		case 0xef:
+		case 0x6c:	/* INS */
+		case 0x6d:
+		case 0x6e:	/* OUTS */
+		case 0x6f:
+			return true;
+		}
+		break;
+	case 2:
+		switch (b) {
+		case 0x33:	/* RDPMC */
+			return true;
+		}
+		break;
+	}
+
+	return false;
+}
+
+#else /* !CONFIG_KVM_VMWARE */
+
+static inline bool kvm_vmware_backdoor_enabled(struct kvm_vcpu

*vcpu)

arch/x86/kvm/kvm_vmware.h new file +15 -0

+{
+	return false;
+}
+
+static inline bool kvm_vmware_is_backdoor_pmc(u32 pmc_idx)
+{
+	return false;
+}
+
+static inline bool kvm_vmware_io_port_allowed(u16 port)
+{
+	return false;
+}
+
+static inline int kvm_vmware_pmu_rdpmc(struct kvm_vcpu *vcpu, u32

idx, u64 *data)

arch/x86/kvm/kvm_vmware.h new file +5 -0

+{
+	return 0;
+}
+
+static inline bool kvm_vmware_is_backdoor_opcode(u8 opcode_len, u8

len)

Centralize KVM's VMware specific code and introduce CONFIG_KVM_VMWARE to isolate all of it.

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 <[email protected]>

Cc Doug Covelli <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc [email protected]

Cc "H. Peter Anvin" <[email protected]>

Cc Zack Rusin <[email protected]>

Cc [email protected]

Cc [email protected]

---
 MAINTAINERS               |   9 +++
 arch/x86/kvm/Kconfig      |  13 ++++
 arch/x86/kvm/emulate.c    |  11 ++--
 arch/x86/kvm/kvm_vmware.h | 127 ++++++++++++++++++++++++++++++++++++++

 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

MAINTAINERS +9 -0

index 00e94bec401e..9e38103ac2bb 100644
@@ -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 <[email protected]>
+M:	Doug Covelli <[email protected]>
+M:	Paolo Bonzini <[email protected]>
+L:	[email protected]
+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 <[email protected]>
 M:	Paul Durrant <[email protected]>

arch/x86/kvm/Kconfig +0 -0

index ea2c4f21c1ca..9e3be87fc82b 100644

arch/x86/kvm/Kconfig +10 -0

+	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

arch/x86/kvm/emulate.c +0 -0

index 60986f67c35a..b42988ce8043 100644

Quoted 13 lines · Sean Christopherson

arch/x86/kvm/emulate.c line 29

Quoted 9 lines

arch/x86/kvm/emulate.c +1 -0

index 60986f67c35a..b42988ce8043 100644
@@ -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.

Quoted 8 lines

#include <linux/aaaa.h>
#include <linux/blah.h>

#include <asm/aaaa.h>
#include <asm/blah.h>

#include "aaaa.h"
#include "blah.h"

Yea, that's what I do in my code but in this case I had no idea where to put it because none of the sections in that file are sorted, where would you like the include among:

```
#include <linux/kvm_host.h>
#include "kvm_cache_regs.h"
#include "kvm_emulate.h"
#include <linux/stringify.h>
#include <asm/debugreg.h>
#include <asm/nospec-branch.h>
#include <asm/ibt.h>

#include "x86.h"
#include "tss.h"
#include "mmu.h"
#include "pmu.h"
```

below kvm_emulate or would you like me to resort all the includes?

Quoted 15 lines

arch/x86/kvm/emulate.c static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt, · line 2566

Quoted 7 lines

arch/x86/kvm/emulate.c +1 -2

@@ -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.

Quoted 5 lines

arch/x86/kvm/emulate.c static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt, · line 2567

Quoted 1 line

arch/x86/kvm/emulate.c +1 -0

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

Sorry, I'm confused about this one. I find it a lot easier to review helpers if I know what code they're supposed to replace and that's harder to do if a change is just adding some code without any indication of where it's coming from but I'm happy to adjust this in whatever way is easiest for you.

arch/x86/kvm/Kconfig +0 -0

index 9e3be87fc82b..f817601924bd 100644
@@ -183,11 +183,13 @@ config KVM_VMWARE
 	depends on KVM
 	default y

include/uapi/linux/kvm.h +10 -0

index 793d0cf7ae3c..adf1a1449c06 100644
@@ -135,6 +135,27 @@ struct kvm_xen_exit {
 	} u;
 };
 
+struct kvm_vmware_exit {
+#define KVM_EXIT_VMWARE_HCALL          1
+	__u32 type;
+	__u32 pad1;
+	union {
+		struct {
+			__u32 longmode;
+			__u32 cpl;
+			__u64 rax, rbx, rcx, rdx, rsi, rdi, rbp;
+			__u64 result;

Allow handling VMware backdoors by the L0 monitor. This is required on setups running Windows VBS, where the L1 will be running Hyper-V which can't handle VMware backdoors. Thus on Windows VBS legacy VMware backdoor calls issued by the userspace will end up in Hyper-V (L1) and endup throwing an error.
Add a KVM cap that, in nested setups, allows the legacy VMware backdoor to be handled by the L0 monitor. Thanks to this we can make sure that VMware backdoor is always handled by the correct monitor.

Signed-off-by Zack Rusin <[email protected]>

Cc Doug Covelli <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc [email protected]

Cc "H. Peter Anvin" <[email protected]>

Cc Zack Rusin <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
  Documentation/virt/kvm/api.rst  | 14 +++++++++++
  arch/x86/include/asm/kvm_host.h |  1 +
  arch/x86/kvm/Kconfig            |  1 +
  arch/x86/kvm/kvm_vmware.h       | 42 +++++++++++++++++++++++++++++++++
  arch/x86/kvm/svm/nested.c       |  6 +++++
  arch/x86/kvm/svm/svm.c          |  3 ++-
  arch/x86/kvm/vmx/nested.c       |  6 +++++
  arch/x86/kvm/x86.c              |  8 +++++++
  include/uapi/linux/kvm.h        |  1 +
  9 files changed, 81 insertions(+), 1 deletion(-)

Documentation/virt/kvm/api.rst +7 -0

index 6d3d2a509848..55bd464ebf95 100644
@@ -8322,6 +8322,20 @@ userspace handling of hypercalls is discouraged. To implement
  such functionality, use KVM_EXIT_IO (x86) or KVM_EXIT_MMIO
  (all except s390).
 
+7.39 KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0
+------------------------------------------
+
+:Architectures: x86
+:Parameters: args[0] whether the feature should be enabled or not
+:Returns: 0 on success.
+

arch/x86/kvm/x86.c +1 -0

index 300cef9a37e2..5dc57bc57851 100644
@@ -4653,6 +4653,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
  #ifdef CONFIG_KVM_VMWARE
  	case KVM_CAP_X86_VMWARE_BACKDOOR:
  	case KVM_CAP_X86_VMWARE_HYPERCALL:
+	case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:
  #endif
  		r = 1;
  		break;

Quoted 55 lines · Zack Rusin

Allow handling VMware backdoors by the L0 monitor. This is required on setups running Windows VBS, where the L1 will be running Hyper-V which can't handle VMware backdoors. Thus on Windows VBS legacy VMware backdoor calls issued by the userspace will end up in Hyper-V (L1) and endup throwing an error.
Add a KVM cap that, in nested setups, allows the legacy VMware backdoor to be handled by the L0 monitor. Thanks to this we can make sure that VMware backdoor is always handled by the correct monitor.

Signed-off-by Zack Rusin <[email protected]>

Cc Doug Covelli <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Jonathan Corbet <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Dave Hansen <[email protected]>

Cc [email protected]

Cc "H. Peter Anvin" <[email protected]>

Cc Zack Rusin <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
  Documentation/virt/kvm/api.rst  | 14 +++++++++++
  arch/x86/include/asm/kvm_host.h |  1 +
  arch/x86/kvm/Kconfig            |  1 +
  arch/x86/kvm/kvm_vmware.h       | 42 +++++++++++++++++++++++++++++++++
  arch/x86/kvm/svm/nested.c       |  6 +++++
  arch/x86/kvm/svm/svm.c          |  3 ++-
  arch/x86/kvm/vmx/nested.c       |  6 +++++
  arch/x86/kvm/x86.c              |  8 +++++++
  include/uapi/linux/kvm.h        |  1 +
  9 files changed, 81 insertions(+), 1 deletion(-)

Documentation/virt/kvm/api.rst +11 -0

index 6d3d2a509848..55bd464ebf95 100644
@@ -8322,6 +8322,20 @@ userspace handling of hypercalls is discouraged. To implement
  such functionality, use KVM_EXIT_IO (x86) or KVM_EXIT_MMIO
  (all except s390).
 
+7.39 KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0
+------------------------------------------
+
+:Architectures: x86
+:Parameters: args[0] whether the feature should be enabled or not
+:Returns: 0 on success.
+
+Capability allows VMware backdoors to be handled by L0 when running
+on nested configurations. This is required when, for example
+running Windows guest with Hyper-V VBS enabled - in that configuration
+the VMware backdoor calls issued by VMware tools would endup in Hyper-V

Documentation/virt/kvm/api.rst userspace handling of hypercalls is discouraged. To implement · line 8341

Quoted 6 lines

Documentation/virt/kvm/api.rst +3 -0

+(L1) which doesn't handle VMware backdoor. Enable this option to have
+VMware backdoor sent to L0 monitor.
+
  8. Other capabilities.
  ======================
 

You're not basing the patch set on v6.15-rcX?

Quoted 13 lines

arch/x86/kvm/x86.c +1 -0

index 300cef9a37e2..5dc57bc57851 100644
@@ -4653,6 +4653,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
  #ifdef CONFIG_KVM_VMWARE
      case KVM_CAP_X86_VMWARE_BACKDOOR:
      case KVM_CAP_X86_VMWARE_HYPERCALL:
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:
  #endif
              r = 1;
              break;

@@ -6754,6 +6755,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,

arch/x86/kvm/x86.c int kvm_vm_ioctl_enable_cap(struct kvm *kvm, · line 6760

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

              kvm->arch.vmware.hypercall_enabled = cap->args[0];
              r = 0;
              break;
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:
+             r = -EINVAL;
+             if (cap->args[0] & ~1)

Replace ~1 with a macro for better readability please.

Quoted 9 lines · Xin Li

arch/x86/kvm/x86.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 4656

Quoted 9 lines

arch/x86/kvm/x86.c +1 -0

index 300cef9a37e2..5dc57bc57851 100644
@@ -4653,6 +4653,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
  #ifdef CONFIG_KVM_VMWARE
      case KVM_CAP_X86_VMWARE_BACKDOOR:
      case KVM_CAP_X86_VMWARE_HYPERCALL:
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:

Quoted 4 lines

Quoted 4 lines

arch/x86/kvm/x86.c +0 -0

  #endif
              r = 1;
              break;

@@ -6754,6 +6755,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_vm_ioctl_enable_cap(struct kvm *kvm, · line 6760

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

              kvm->arch.vmware.hypercall_enabled = cap->args[0];
              r = 0;
              break;
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:
+             r = -EINVAL;
+             if (cap->args[0] & ~1)

Replace ~1 with a macro for better readability please.

Are you sure about that? This code is already used elsewhere in the file (for KVM_CAP_EXIT_ON_EMULATION_FAILURE) so, ignoring the fact that it's arguable whether IS_ZERO_OR_ONE is more readable than & ~1, if we use a macro for the vmware caps and not for KVM_CAP_EXIT_ON_EMULATION_FAILURE then the code would be inconsistent and that decreases the readability.

Or are you saying that since I'm already there you'd like to see a completely separate patch that defines some kind of IS_ZERO_OR_ONE macro for KVM, use it for KVM_CAP_EXIT_ON_EMULATION_FAILURE and, once that lands then I can make use of it in this series?

Quoted 10 lines · Zack Rusin

Quoted 9 lines · Xin Li

arch/x86/kvm/x86.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 4656

Quoted 9 lines

arch/x86/kvm/x86.c +1 -0

index 300cef9a37e2..5dc57bc57851 100644
@@ -4653,6 +4653,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
  #ifdef CONFIG_KVM_VMWARE
      case KVM_CAP_X86_VMWARE_BACKDOOR:
      case KVM_CAP_X86_VMWARE_HYPERCALL:
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:

I would probably omit the L0, because KVM could be running as L1.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

arch/x86/kvm/x86.c +0 -0

  #endif
              r = 1;
              break;

@@ -6754,6 +6755,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_vm_ioctl_enable_cap(struct kvm *kvm, · line 6760

Quoted 6 lines

arch/x86/kvm/x86.c +3 -0

              kvm->arch.vmware.hypercall_enabled = cap->args[0];
              r = 0;
              break;
+     case KVM_CAP_X86_VMWARE_NESTED_BACKDOOR_L0:
+             r = -EINVAL;
+             if (cap->args[0] & ~1)

Replace ~1 with a macro for better readability please.

Quoted 6 lines

Are you sure about that? This code is already used elsewhere in the file (for KVM_CAP_EXIT_ON_EMULATION_FAILURE) so, ignoring the fact that it's arguable whether IS_ZERO_OR_ONE is more readable than & ~1, if we use a macro for the vmware caps and not for KVM_CAP_EXIT_ON_EMULATION_FAILURE then the code would be inconsistent and that decreases the readability.

Heh, KVM_CAP_EXIT_ON_EMULATION_FAILURE is the odd one out. Even if that weren't the case, this is one of the situations where diverging from the existing code is desirable, because the existing code is garbage.

arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_caps.supported_quirks)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_X2APIC_API_VALID_FLAGS)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_get_allowed_disable_exits())
arch/x86/kvm/x86.c:                 (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE))
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_BUS_LOCK_DETECTION_VALID_MODE)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_EXIT_HYPERCALL_VALID_MASK) {
arch/x86/kvm/x86.c:             if (cap->args[0] & ~1)
arch/x86/kvm/x86.c:             if (!enable_pmu || (cap->args[0] & ~KVM_CAP_PMU_VALID_MASK))
arch/x86/kvm/x86.c:             if ((u32)cap->args[0] & ~KVM_X86_NOTIFY_VMEXIT_VALID_BITS)
virt/kvm/kvm_main.c:            if (cap->flags || (cap->args[0] & ~allowed_options))

Quoted 4 lines

Or are you saying that since I'm already there you'd like to see a completely separate patch that defines some kind of IS_ZERO_OR_ONE macro for KVM, use it for KVM_CAP_EXIT_ON_EMULATION_FAILURE and, once that lands then I can make use of it in this series?

Xin is suggesting that you add a macro in arch/x86/include/uapi/asm/kvm.h to #define which bits are valid and which bits are reserved.

At a glance, you can kill multiple birds with one stone. Rather than add three separate capabilities, add one capability and then a variety of flags. E.g.

#define KVM_X86_VMWARE_HYPERCALL        _BITUL(0)
#define KVM_X86_VMWARE_BACKDOOR         _BITUL(1)
#define KVM_X86_VMWARE_NESTED_BACKDOOR  _BITUL(2)
#define KVM_X86_VMWARE_VALID_FLAGS      (KVM_X86_VMWARE_HYPERCALL |
                                         KVM_X86_VMWARE_BACKDOOR |
                                         KVM_X86_VMWARE_NESTED_BACKDOOR)

        case KVM_CAP_X86_VMWARE_EMULATION:
                r = -EINVAL;
                if (cap->args[0] & ~KVM_X86_VMWARE_VALID_FLAGS)
                        break;

                mutex_lock(&kvm->lock);
                if (!kvm->created_vcpus) {
                        if (cap->args[0] & KVM_X86_VMWARE_HYPERCALL)
                                kvm->arch.vmware.hypercall_enabled = true;
                        if (cap->args[0] & KVM_X86_VMWARE_BACKDOOR)
                                kvm->arch.vmware.backdoor_enabled;
                        if (cap->args[0] & KVM_X86_VMWARE_NESTED_BACKDOOR)
                                kvm->arch.vmware.nested_backdoor_enabled = true;
                        r = 0;
                }
                mutex_unlock(&kvm->lock);
                break;

That approach wouldn't let userspace disable previously enabled VMware capabilities, but unless there's a use case for doing so, that should be a non-issue.

Quoted 4 lines · Sean Christopherson

Heh, KVM_CAP_EXIT_ON_EMULATION_FAILURE is the odd one out. Even if that weren't the case, this is one of the situations where diverging from the existing code is desirable, because the existing code is garbage.

Quoted 11 lines

arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_caps.supported_quirks)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_X2APIC_API_VALID_FLAGS)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_get_allowed_disable_exits())
arch/x86/kvm/x86.c:                 (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE))
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_BUS_LOCK_DETECTION_VALID_MODE)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_EXIT_HYPERCALL_VALID_MASK) {
arch/x86/kvm/x86.c:             if (cap->args[0] & ~1)
arch/x86/kvm/x86.c:             if (!enable_pmu || (cap->args[0] & ~KVM_CAP_PMU_VALID_MASK))
arch/x86/kvm/x86.c:             if ((u32)cap->args[0] & ~KVM_X86_NOTIFY_VMEXIT_VALID_BITS)
virt/kvm/kvm_main.c:            if (cap->flags || (cap->args[0] & ~allowed_options))

That's because none of those other options are boolean, right? I assumed that the options that have valid masks use defines but booleans use ~1 because (val & ~1) makes it obvious to the reader that the option is in fact a boolean in a way that (val & ~KVM_SOME_VALID_BITS) can not.

Quoted 36 lines

Quoted 4 lines

Or are you saying that since I'm already there you'd like to see a completely separate patch that defines some kind of IS_ZERO_OR_ONE macro for KVM, use it for KVM_CAP_EXIT_ON_EMULATION_FAILURE and, once that lands then I can make use of it in this series?

Xin is suggesting that you add a macro in arch/x86/include/uapi/asm/kvm.h to #define which bits are valid and which bits are reserved.

At a glance, you can kill multiple birds with one stone. Rather than add three separate capabilities, add one capability and then a variety of flags. E.g.

#define KVM_X86_VMWARE_HYPERCALL        _BITUL(0)
#define KVM_X86_VMWARE_BACKDOOR         _BITUL(1)
#define KVM_X86_VMWARE_NESTED_BACKDOOR  _BITUL(2)
#define KVM_X86_VMWARE_VALID_FLAGS      (KVM_X86_VMWARE_HYPERCALL |
                                         KVM_X86_VMWARE_BACKDOOR |
                                         KVM_X86_VMWARE_NESTED_BACKDOOR)

        case KVM_CAP_X86_VMWARE_EMULATION:
                r = -EINVAL;
                if (cap->args[0] & ~KVM_X86_VMWARE_VALID_FLAGS)
                        break;

                mutex_lock(&kvm->lock);
                if (!kvm->created_vcpus) {
                        if (cap->args[0] & KVM_X86_VMWARE_HYPERCALL)
                                kvm->arch.vmware.hypercall_enabled = true;
                        if (cap->args[0] & KVM_X86_VMWARE_BACKDOOR)
                                kvm->arch.vmware.backdoor_enabled;
                        if (cap->args[0] & KVM_X86_VMWARE_NESTED_BACKDOOR)
                                kvm->arch.vmware.nested_backdoor_enabled = true;
                        r = 0;
                }
                mutex_unlock(&kvm->lock);
                break;

Quoted 2 lines

That approach wouldn't let userspace disable previously enabled VMware capabilities, but unless there's a use case for doing so, that should be a non-issue.

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Quoted 17 lines · Zack Rusin

Quoted 4 lines · Sean Christopherson

Heh, KVM_CAP_EXIT_ON_EMULATION_FAILURE is the odd one out. Even if that weren't the case, this is one of the situations where diverging from the existing code is desirable, because the existing code is garbage.

Quoted 11 lines

arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_caps.supported_quirks)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_X2APIC_API_VALID_FLAGS)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~kvm_get_allowed_disable_exits())
arch/x86/kvm/x86.c:                 (cap->args[0] & ~KVM_X86_DISABLE_EXITS_PAUSE))
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_MSR_EXIT_REASON_VALID_MASK)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_BUS_LOCK_DETECTION_VALID_MODE)
arch/x86/kvm/x86.c:             if (cap->args[0] & ~KVM_EXIT_HYPERCALL_VALID_MASK) {
arch/x86/kvm/x86.c:             if (cap->args[0] & ~1)
arch/x86/kvm/x86.c:             if (!enable_pmu || (cap->args[0] & ~KVM_CAP_PMU_VALID_MASK))
arch/x86/kvm/x86.c:             if ((u32)cap->args[0] & ~KVM_X86_NOTIFY_VMEXIT_VALID_BITS)
virt/kvm/kvm_main.c:            if (cap->flags || (cap->args[0] & ~allowed_options))

Quoted 5 lines

That's because none of those other options are boolean, right? I assumed that the options that have valid masks use defines but booleans use ~1 because (val & ~1) makes it obvious to the reader that the option is in fact a boolean in a way that (val & ~KVM_SOME_VALID_BITS) can not.

The entire reason when KVM checks and enforces cap->args[0] is so that KVM can expand the capability's functionality in the future. Whether or not a capability is *currently* a boolean, i.e. only has one supported flag, is completely irrelevant.

KVM has burned itself many times over by not performing checks, e.g. is how we ended up with things like KVM_CAP_DISABLE_QUIRKS2.

Quoted 39 lines

Quoted 38 lines

Quoted 4 lines

Or are you saying that since I'm already there you'd like to see a completely separate patch that defines some kind of IS_ZERO_OR_ONE macro for KVM, use it for KVM_CAP_EXIT_ON_EMULATION_FAILURE and, once that lands then I can make use of it in this series?

Xin is suggesting that you add a macro in arch/x86/include/uapi/asm/kvm.h to #define which bits are valid and which bits are reserved.

At a glance, you can kill multiple birds with one stone. Rather than add three separate capabilities, add one capability and then a variety of flags. E.g.

#define KVM_X86_VMWARE_HYPERCALL        _BITUL(0)
#define KVM_X86_VMWARE_BACKDOOR         _BITUL(1)
#define KVM_X86_VMWARE_NESTED_BACKDOOR  _BITUL(2)
#define KVM_X86_VMWARE_VALID_FLAGS      (KVM_X86_VMWARE_HYPERCALL |
                                         KVM_X86_VMWARE_BACKDOOR |
                                         KVM_X86_VMWARE_NESTED_BACKDOOR)

        case KVM_CAP_X86_VMWARE_EMULATION:
                r = -EINVAL;
                if (cap->args[0] & ~KVM_X86_VMWARE_VALID_FLAGS)
                        break;

                mutex_lock(&kvm->lock);
                if (!kvm->created_vcpus) {
                        if (cap->args[0] & KVM_X86_VMWARE_HYPERCALL)
                                kvm->arch.vmware.hypercall_enabled = true;
                        if (cap->args[0] & KVM_X86_VMWARE_BACKDOOR)
                                kvm->arch.vmware.backdoor_enabled;
                        if (cap->args[0] & KVM_X86_VMWARE_NESTED_BACKDOOR)
                                kvm->arch.vmware.nested_backdoor_enabled = true;
                        r = 0;
                }
                mutex_unlock(&kvm->lock);
                break;

That approach wouldn't let userspace disable previously enabled VMware capabilities, but unless there's a use case for doing so, that should be a non-issue.

Quoted 3 lines

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

How so? If kvm.enable_vmware_backdoor is true, then the backdoor is enabled for all VMs, else it's disabled by default but can be enabled on a per-VM basis by the new capability.

Quoted 3 lines

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

We could go this route, e.g. KVM does something similar for PMU virtualization. But the key difference is that enable_pmu is enabled by default, whereas enable_vmware_backdoor is disabled by default. I.e. it makes far more sense for the capability to let userspace opt-in, as opposed to opt-out.

Quoted 2 lines

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Again, that's not KVM's default, and it will never be KVM's default.

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

How so? If kvm.enable_vmware_backdoor is true, then the backdoor is enabled for all VMs, else it's disabled by default but can be enabled on a per-VM basis by the new capability.

Like you said if kvm.enable_vmware_backdoor is true, then it's enabled for all VMs, so it'd make sense to allow disabling it on a per-vm basis on those systems.
Just like when the kvm.enable_vmware_backdoor is false, the cap can be used to enable it on a per-vm basis.

Quoted 9 lines

Quoted 3 lines

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

We could go this route, e.g. KVM does something similar for PMU virtualization. But the key difference is that enable_pmu is enabled by default, whereas enable_vmware_backdoor is disabled by default. I.e. it makes far more sense for the capability to let userspace opt-in, as opposed to opt-out.

Quoted 4 lines

Quoted 2 lines

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Again, that's not KVM's default, and it will never be KVM's default.

All I'm saying is that you can enable it on a whole system via the boot flags and on the systems on which it has been turned on it'd make sense to allow disabling it on a per-vm basis.

Anyway, I'm sure I can make it work correctly under any constraints, so let me try to understand the issue because I'm not sure what we're solving here. Is the problem the fact that we have three caps and instead want to squeeze all of the functionality under one cap?

Quoted 9 lines · Zack Rusin

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

How so? If kvm.enable_vmware_backdoor is true, then the backdoor is enabled for all VMs, else it's disabled by default but can be enabled on a per-VM basis by the new capability.

Quoted 5 lines

Like you said if kvm.enable_vmware_backdoor is true, then it's enabled for all VMs, so it'd make sense to allow disabling it on a per-vm basis on those systems.
Just like when the kvm.enable_vmware_backdoor is false, the cap can be used to enable it on a per-vm basis.

Why? What use case does that serve?

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

We could go this route, e.g. KVM does something similar for PMU virtualization. But the key difference is that enable_pmu is enabled by default, whereas enable_vmware_backdoor is disabled by default. I.e. it makes far more sense for the capability to let userspace opt-in, as opposed to opt-out.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Again, that's not KVM's default, and it will never be KVM's default.

All I'm saying is that you can enable it on a whole system via the boot flags and on the systems on which it has been turned on it'd make sense to allow disabling it on a per-vm basis.

Again, why would anyone do that? If you *know* you're going to run some VMs with VMware emulation and some without, the sane approach is to not touch the module param and rely entirely on the capability. Otherwise the VMM must be able to opt-out, which means that running an older userspace that doesn't know about the new capability *can't* opt-out.

The only reason to even keep the module param is to not break existing users, e.g. to be able to run VMs that want VMware functionality using an existing VMM.

Quoted 4 lines

Anyway, I'm sure I can make it work correctly under any constraints, so let me try to understand the issue because I'm not sure what we're solving here. Is the problem the fact that we have three caps and instead want to squeeze all of the functionality under one cap?

The "problem" is that I don't want to add complexity and create ABI for a use case that doesn't exist.

Quoted 17 lines · Sean Christopherson

Quoted 14 lines · Zack Rusin

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

How so? If kvm.enable_vmware_backdoor is true, then the backdoor is enabled for all VMs, else it's disabled by default but can be enabled on a per-VM basis by the new capability.

Like you said if kvm.enable_vmware_backdoor is true, then it's enabled for all VMs, so it'd make sense to allow disabling it on a per-vm basis on those systems.
Just like when the kvm.enable_vmware_backdoor is false, the cap can be used to enable it on a per-vm basis.

Quoted 1 line

Why? What use case does that serve?

Testing purposes?

Quoted 32 lines

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

We could go this route, e.g. KVM does something similar for PMU virtualization. But the key difference is that enable_pmu is enabled by default, whereas enable_vmware_backdoor is disabled by default. I.e. it makes far more sense for the capability to let userspace opt-in, as opposed to opt-out.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Again, that's not KVM's default, and it will never be KVM's default.

All I'm saying is that you can enable it on a whole system via the boot flags and on the systems on which it has been turned on it'd make sense to allow disabling it on a per-vm basis.

Again, why would anyone do that? If you *know* you're going to run some VMs with VMware emulation and some without, the sane approach is to not touch the module param and rely entirely on the capability. Otherwise the VMM must be able to opt-out, which means that running an older userspace that doesn't know about the new capability *can't* opt-out.

The only reason to even keep the module param is to not break existing users, e.g. to be able to run VMs that want VMware functionality using an existing VMM.

Quoted 4 lines

Anyway, I'm sure I can make it work correctly under any constraints, so let me try to understand the issue because I'm not sure what we're solving here. Is the problem the fact that we have three caps and instead want to squeeze all of the functionality under one cap?

Quoted 2 lines

The "problem" is that I don't want to add complexity and create ABI for a use case that doesn't exist.

Would you like to see a v3 where I specifically do not allow disabling those caps?

Quoted 18 lines · Zack Rusin

Quoted 17 lines · Sean Christopherson

Quoted 14 lines · Zack Rusin

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

I'd say that if we desperately want to use a single cap for all of these then I'd probably prefer a different approach because this would make vmware_backdoor_enabled behavior really wacky.

How so? If kvm.enable_vmware_backdoor is true, then the backdoor is enabled for all VMs, else it's disabled by default but can be enabled on a per-VM basis by the new capability.

Like you said if kvm.enable_vmware_backdoor is true, then it's enabled for all VMs, so it'd make sense to allow disabling it on a per-vm basis on those systems.
Just like when the kvm.enable_vmware_backdoor is false, the cap can be used to enable it on a per-vm basis.

Quoted 3 lines

Quoted 1 line

Why? What use case does that serve?

Testing purposes?

Heh, testing what?

Quoted 32 lines

Quoted 32 lines

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

It's the one that currently can only be set via kernel boot flags, so having systems where the boot flag is on and disabling it on a per-vm basis makes sense and breaks with this.

We could go this route, e.g. KVM does something similar for PMU virtualization. But the key difference is that enable_pmu is enabled by default, whereas enable_vmware_backdoor is disabled by default. I.e. it makes far more sense for the capability to let userspace opt-in, as opposed to opt-out.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I'd probably still write the code to be able to disable/enable all of them because it makes sense for vmware_backdoor_enabled.

Again, that's not KVM's default, and it will never be KVM's default.

All I'm saying is that you can enable it on a whole system via the boot flags and on the systems on which it has been turned on it'd make sense to allow disabling it on a per-vm basis.

Again, why would anyone do that? If you *know* you're going to run some VMs with VMware emulation and some without, the sane approach is to not touch the module param and rely entirely on the capability. Otherwise the VMM must be able to opt-out, which means that running an older userspace that doesn't know about the new capability *can't* opt-out.

The only reason to even keep the module param is to not break existing users, e.g. to be able to run VMs that want VMware functionality using an existing VMM.

Quoted 4 lines

Anyway, I'm sure I can make it work correctly under any constraints, so let me try to understand the issue because I'm not sure what we're solving here. Is the problem the fact that we have three caps and instead want to squeeze all of the functionality under one cap?

Quoted 5 lines

Quoted 2 lines

The "problem" is that I don't want to add complexity and create ABI for a use case that doesn't exist.

Would you like to see a v3 where I specifically do not allow disabling those caps?

Yes. Though I recommend waiting to send a v3 until I (and others) have had a change to review the rest of the patches, e.g. to avoid wasting your time.

arch/x86/kvm/svm/nested.c +1 -0

index 04c375bf1ac2..74c472e51479 100644
@@ -22,6 +22,7 @@
 #include <asm/debugreg.h>
 
 #include "kvm_emulate.h"
+#include "kvm_vmware.h"
 #include "trace.h"
 #include "mmu.h"
 #include "x86.h"

Add a testcase to exercise KVM_CAP_X86_VMWARE_HYPERCALL and validate that KVM exits to userspace on hypercalls and registers are correctly preserved.

Signed-off-by Zack Rusin <[email protected]>

Cc Doug Covelli <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Shuah Khan <[email protected]>

Cc Sean Christopherson <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Joel Stanley <[email protected]>

Cc Zack Rusin <[email protected]>

Cc Isaku Yamahata <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc [email protected]

Cc [email protected]

Cc [email protected]

---
 tools/include/uapi/linux/kvm.h                |   3 +
 tools/testing/selftests/kvm/Makefile.kvm      |   1 +
 .../selftests/kvm/x86/vmware_hypercall_test.c | 121 ++++++++++++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86/vmware_hypercall_test.c

tools/include/uapi/linux/kvm.h +0 -0

index 502ea63b5d2e..3b3ad1827245 100644