Re: [PATCH 2/3] KVM: x86: Add support for VMware guest specific hypercalls

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/7/24 23:32, Sean Christopherson wrote:
> On Mon, Nov 04, 2024, Zack Rusin wrote:
>> On Mon, Nov 4, 2024 at 5:13 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>> On Wed, Oct 30, 2024 at 4:35 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>>>>
>>>> VMware products handle hypercalls in userspace. Give KVM the ability
>>>> to run VMware guests unmodified by fowarding all hypercalls to the
>>>> userspace.
>>>>
>>>> Enabling of the KVM_CAP_X86_VMWARE_HYPERCALL_ENABLE capability turns
>>>> the feature on - it's off by default. This allows vmx's built on top
>>>> of KVM to support VMware specific hypercalls.
>>>
>>> Hi Zack,
>>
>> Hi, Paolo.
>>
>> Thank you for looking at this.
>>
>>> is there a spec of the hypercalls that are supported by userspace? I
>>> would like to understand if there's anything that's best handled in
>>> the kernel.
>>
>> There's no spec but we have open headers listing the hypercalls.
>> There's about a 100 of them (a few were deprecated), the full
>> list starts here:
>> https://github.com/vmware/open-vm-tools/blob/739c5a2f4bfd4cdda491e6a6f6869d88c0bd6972/open-vm-tools/lib/include/backdoor_def.h#L97
>> They're not well documented, but the names are pretty self-explenatory.
> 
> At a quick glance, this one needs to be handled in KVM:
> 
>    BDOOR_CMD_VCPU_MMIO_HONORS_PAT
> 
> and these probably should be in KVM:
> 
>    BDOOR_CMD_GETTIME
>    BDOOR_CMD_SIDT
>    BDOOR_CMD_SGDT
>    BDOOR_CMD_SLDT_STR
>    BDOOR_CMD_GETTIMEFULL
>    BDOOR_CMD_VCPU_LEGACY_X2APIC_OK
>    BDOOR_CMD_STEALCLOCK
> 
> and these maybe? (it's not clear what they do, from the name alone)
> 
>    BDOOR_CMD_GET_VCPU_INFO
>    BDOOR_CMD_VCPU_RESERVED
> 
>>> If we allow forwarding _all_ hypercalls to userspace, then people will
>>> use it for things other than VMware and there goes all hope of
>>> accelerating stuff in the kernel in the future.
> 
> To some extent, that ship has sailed, no?  E.g. do KVM_XEN_HVM_CONFIG with
> KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL set, and userspace can intercept pretty much
> all hypercalls with very few side effects.

Yes, but "pretty much all" is different from "this is a blanket 
hypercall vmexit for you to do as you please".

>>> So even having _some_ checks in the kernel before going out to
>>> userspace would keep that door open, or at least try.
>>
>> Doug just looked at this and I think I might have an idea on how to
>> limit the scope at least a bit: if you think it would help we could
>> limit forwarding of hypercalls to userspace only to those that that
>> come with a BDOOR_MAGIC (which is 0x564D5868) in eax. Would that help?
> 
> I don't think it addresses Paolo's concern (if I understood Paolo's concern
> correctly),

It does alleviate it.  Yeah, it would be just a tiny hurdle for 
userspace to set eax to a specific hex value to get them hypercalls. 
But it is _something_ at least.  It's enough to decrease substantially 
my level of sympathy for whoever does it, and as you point out it's also 
justified in terms of interoperability.

> but it would help from the perspective of allowing KVM to support
> VMware hypercalls and Xen/Hyper-V/KVM hypercalls in the same VM.

That too.  VMware in fact might be interested in reusing Hyper-V 
support.  Zack?

> I also think we should add CONFIG_KVM_VMWARE from the get-go, and if we're feeling
> lucky, maybe even retroactively bury KVM_CAP_X86_VMWARE_BACKDOOR behind that
> Kconfig.  That would allow limiting the exposure to VMware specific code, e.g. if
> KVM does end up handling hypercalls in-kernel.  And it might deter abuse to some
> extent.

A bit of wishful thinking on the last sentence but yes, we should do it.

Also we should have a single cap, KVM_CAP_X86_VMWARE, with flags 
KVM_CAP_X86_VMWARE_{BACKDOOR,HYPERCALL}.  Depending on exact details of 
VMware's spec it may even make sense to split further as in 
KVM_CAP_X86_VMWARE_{IOPORT,PMC,HYPERCALL}.  The I/O port is a bit nasty 
with how it bypasses the TSS; if VMware wanted to deprecate it, I would 
not complain at all.

To sum up:

- new Kconfig symbol hiding all existing VMware code

- new cap, KVM_CAP_X86_VMWARE returning the bits that you can set with 
KVM_ENABLE_CAP.  As in your patch, enable_vmware_backdoor provides a 
default for KVM_CAP_X86_VMWARE when the cap is not enabled, but it is 
generally deprecated.

- enable_vmware_backdoor should *not* enable KVM_CAP_X86_VMWARE_HYPERCALL

Paolo