Re: [PATCH v2 4/5] KVM: x86: Add support for legacy VMware backdoors in nested setups

[Sean Christopherson <seanjc@google.com>]

On Wed, Apr 23, 2025, Zack Rusin wrote:
> On Wed, Apr 23, 2025 at 3:56 AM Xin Li <xin@zytor.com> wrote:
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 300cef9a37e2..5dc57bc57851 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -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.

> > >   #endif
> > >               r = 1;
> > >               break;
> > > @@ -6754,6 +6755,13 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> > >               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.

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


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