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

[Zack Rusin <zack.rusin@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4093 bytes --]

On Wed, Apr 23, 2025 at 2:58 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Apr 23, 2025, Zack Rusin wrote:
> > On Wed, Apr 23, 2025 at 1:16 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Wed, Apr 23, 2025, Zack Rusin wrote:
> > > > On Wed, Apr 23, 2025 at 11:54 AM Sean Christopherson <seanjc@google.com> wrote:
> > > > > > 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.
> > >
> > > Why?  What use case does that serve?
> >
> > Testing purposes?
>
> Heh, testing what?

Running VMware and non-VMware guests on the same system... I'm in a
weird spot where I'm defending not my own code, so I'd prefer not
having to do that. We don't use kvm.vmware_backdoor_enabled as a boot
flag, we haven't written that code, so I don't want to be arguing on
behalf of it either way. I was just trying to make sure this nicely
works with the new cap's. In this case having it just work is actually
less effort than making it not work so it just seemed like a nice and
proper thing to do.

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

Sounds good.

z

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5414 bytes --]