On Wed, Apr 23, 2025 at 9:31 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Wed, Apr 23, 2025, Zack Rusin wrote:
> > On Wed, Apr 23, 2025 at 3:56 AM Xin Li <xin@xxxxxxxxx> 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.
Yea, that sounds good to me.
> > > > #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))
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.
I don't want to be defending the code in there, especially to its
maintainers :) I'm very happy to change it in any way you feel is more
readable to you, but I don't think it's crap :)
> > 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.
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. Of course, we want it on all the time, so I
also don't necessarily want to be arguing about being able to disable
those options ;)
z
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
