| Message ID | 20190823081316.28478-3-thomas_os@shipmail.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Fri, Aug 23, 2019 at 10:13:14AM +0200, Thomas Hellström (VMware) wrote: > From: Thomas Hellstrom <thellstrom@vmware.com> > > The new header is intended to be used by drivers using the backdoor. > Follow the kvm example using alternatives self-patching to > choose between vmcall, vmmcall and io instructions. > > Also define two new CPU feature flags to indicate hypervisor support > for vmcall- and vmmcall instructions. I could use some of the explanation why we need two feature flags added here from: https://lkml.kernel.org/r/970d2bb6-ab29-315f-f5d8-5d11095859af@shipmail.org Thx.
On Fri, Aug 23, 2019 at 10:13:14AM +0200, Thomas Hellström (VMware) wrote: > +/* > + * The high bandwidth out call. The low word of edx is presumed to have the > + * HB and OUT bits set. > + */ > +#define VMWARE_HYPERCALL_HB_OUT \ > + ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT_HB ", %%dx; rep outsb", \ Hmm, that looks fishy: This call in vmw_port_hb_out(), for example, gets converted to the asm below (I've left in the asm touching only rDX). # drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:160: VMW_PORT_HB_OUT( #NO_APP movzwl 0(%rbp), %edx # channel_20(D)->channel_id, channel_20(D)->channel_id ... sall $16, %edx #, tmp172 orl $3, %edx #, tmp173 this is adding channel_id and flags: VMWARE_HYPERVISOR_HB | (channel->channel_id << 16) | VMWARE_HYPERVISOR_OUT, the $3 being (VMWARE_HYPERVISOR_HB | VMWARE_HYPERVISOR_OUT). movslq %edx, %rdx # tmp173, tmp174 Here it is sign-extending it. #APP # 160 "drivers/gpu/drm/vmwgfx/vmwgfx_msg.c" 1 push %rbp;mov %r8, %rbp;# ALT: oldinstr2 # bp 661: movw $0x5659, %dx; rep outsb And now here you're overwriting the low word of %edx. And now it contains: 0x[channel_id]5659 and the low word doesn't contain the 3, i.e., (VMWARE_HYPERVISOR_HB | VMWARE_HYPERVISOR_OUT) anymore. And that's before you do the hypercall so I'm guessing that cannot be right. Or?
On 8/27/19 5:44 PM, Borislav Petkov wrote: > On Fri, Aug 23, 2019 at 10:13:14AM +0200, Thomas Hellström (VMware) wrote: >> +/* >> + * The high bandwidth out call. The low word of edx is presumed to have the >> + * HB and OUT bits set. >> + */ >> +#define VMWARE_HYPERCALL_HB_OUT \ >> + ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT_HB ", %%dx; rep outsb", \ > Hmm, that looks fishy: > > This call in vmw_port_hb_out(), for example, gets converted to the asm > below (I've left in the asm touching only rDX). > > # drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:160: VMW_PORT_HB_OUT( > #NO_APP > movzwl 0(%rbp), %edx # channel_20(D)->channel_id, channel_20(D)->channel_id > > ... > > sall $16, %edx #, tmp172 > orl $3, %edx #, tmp173 > > this is adding channel_id and flags: > > VMWARE_HYPERVISOR_HB | (channel->channel_id << 16) | > VMWARE_HYPERVISOR_OUT, > > the $3 being (VMWARE_HYPERVISOR_HB | VMWARE_HYPERVISOR_OUT). > > movslq %edx, %rdx # tmp173, tmp174 > > Here it is sign-extending it. > > #APP > # 160 "drivers/gpu/drm/vmwgfx/vmwgfx_msg.c" 1 > push %rbp;mov %r8, %rbp;# ALT: oldinstr2 # bp > 661: > movw $0x5659, %dx; rep outsb > > And now here you're overwriting the low word of %edx. And now it > contains: > > 0x[channel_id]5659 > > and the low word doesn't contain the 3, i.e., (VMWARE_HYPERVISOR_HB | > VMWARE_HYPERVISOR_OUT) anymore. And that's before you do the hypercall > so I'm guessing that cannot be right. > > Or? > It should be correct. The flags VMWARE_HYPERVISOR_HB and VMWARE_HYPERVISOR_OUT are only valid for the vmcall / vmmcall versions. For the legacy version, the direction is toggled by the instruction (in vs out) and LB vs HB is toggled by the port number (0x5658 vs 0x5659) So in essence the low word definition of %edx is different in the two versions. I've chosen to use the new vmcall/vmmcall definition in the driver code. /Thomas
On Tue, Aug 27, 2019 at 09:19:03PM +0200, Thomas Hellström (VMware) wrote: > It should be correct. The flags VMWARE_HYPERVISOR_HB and > VMWARE_HYPERVISOR_OUT are only valid for the vmcall / vmmcall versions. > > For the legacy version, the direction is toggled by the instruction (in vs > out) and LB vs HB is toggled by the port number (0x5658 vs 0x5659) > > So in essence the low word definition of %edx is different in the two > versions. I've chosen to use the new vmcall/vmmcall definition in the driver > code. Ah, ok, I see what you mean. The old method would overwrite the low word of %edx but the new one would have the flags already prepared and *not* overwrite them so all good. Can you please document that more explicitly in the comment in arch/x86/include/asm/vmware.h? Something like: "... The new vmcall interface instead uses a set of flags to select bandwidth mode and transfer direction. The set of flags is already loaded into %edx by the macros which use VMWARE_HYPERCALL* and only when the guest must use the old VMWARE_HYPERVISOR_PORT* method, the low word is overwritten by the respective port number." Anyway, something along those lines. We want to have the alternatives code as clear and as transparent as possible because, well, of obvious reasons. :-) Thx.
diff --git a/MAINTAINERS b/MAINTAINERS index c2d975da561f..5bf65a49fa19 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17203,6 +17203,7 @@ M: "VMware, Inc." <pv-drivers@vmware.com> L: virtualization@lists.linux-foundation.org S: Supported F: arch/x86/kernel/cpu/vmware.c +F: arch/x86/include/asm/vmware.h VMWARE PVRDMA DRIVER M: Adit Ranadive <aditr@vmware.com> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 998c2cc08363..55fa3b3f0bac 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -232,6 +232,8 @@ #define X86_FEATURE_VMMCALL ( 8*32+15) /* Prefer VMMCALL to VMCALL */ #define X86_FEATURE_XENPV ( 8*32+16) /* "" Xen paravirtual guest */ #define X86_FEATURE_EPT_AD ( 8*32+17) /* Intel Extended Page Table access-dirty bit */ +#define X86_FEATURE_VMCALL ( 8*32+18) /* "" Hypervisor supports the VMCALL instruction */ +#define X86_FEATURE_VMW_VMMCALL ( 8*32+19) /* "" VMware prefers VMMCALL hypercall instruction */ /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */ #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/ diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h new file mode 100644 index 000000000000..4b220e2bb3e8 --- /dev/null +++ b/arch/x86/include/asm/vmware.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: GPL-2.0 or MIT */ +#ifndef _ASM_X86_VMWARE_H +#define _ASM_X86_VMWARE_H + +#include <asm/cpufeatures.h> +#include <asm/alternative.h> + +/* + * The hypercall definitions differ in the low word of the edx argument in + * the following way: The old port base interface uses the port number to + * distinguish between high- and low bandwidth versions. The new vmcall + * interface instead uses a set of flags to select bandwidth mode and + * transfer direction. New driver code should strictly use the new + * definition of edx content. + */ + +/* Old port-based version */ +#define VMWARE_HYPERVISOR_PORT "0x5658" +#define VMWARE_HYPERVISOR_PORT_HB "0x5659" + +/* Current vmcall / vmmcall version */ +#define VMWARE_HYPERVISOR_HB BIT(0) +#define VMWARE_HYPERVISOR_OUT BIT(1) + +/* The low bandwidth call. The low word of edx is presumed clear. */ +#define VMWARE_HYPERCALL \ + ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT ", %%dx; inl (%%dx)", \ + "vmcall", X86_FEATURE_VMCALL, \ + "vmmcall", X86_FEATURE_VMW_VMMCALL) + +/* + * The high bandwidth out call. The low word of edx is presumed to have the + * HB and OUT bits set. + */ +#define VMWARE_HYPERCALL_HB_OUT \ + ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT_HB ", %%dx; rep outsb", \ + "vmcall", X86_FEATURE_VMCALL, \ + "vmmcall", X86_FEATURE_VMW_VMMCALL) + +/* + * The high bandwidth in call. The low word of edx is presumed to have the + * HB bit set. + */ +#define VMWARE_HYPERCALL_HB_IN \ + ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT_HB ", %%dx; rep insb", \ + "vmcall", X86_FEATURE_VMCALL, \ + "vmmcall", X86_FEATURE_VMW_VMMCALL) +#endif diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c index fcb84b1e099e..abaa1b27353c 100644 --- a/arch/x86/kernel/cpu/vmware.c +++ b/arch/x86/kernel/cpu/vmware.c @@ -31,6 +31,7 @@ #include <asm/timer.h> #include <asm/apic.h> #include <asm/svm.h> +#include <asm/vmware.h> #undef pr_fmt #define pr_fmt(fmt) "vmware: " fmt @@ -41,7 +42,6 @@ #define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1) #define VMWARE_HYPERVISOR_MAGIC 0x564D5868 -#define VMWARE_HYPERVISOR_PORT 0x5658 #define VMWARE_CMD_GETVERSION 10 #define VMWARE_CMD_GETHZ 45 @@ -165,6 +165,10 @@ static void __init vmware_set_capabilities(void) { setup_force_cpu_cap(X86_FEATURE_CONSTANT_TSC); setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE); + if (vmware_hypercall_mode == CPUID_VMWARE_FEATURES_ECX_VMCALL) + setup_force_cpu_cap(X86_FEATURE_VMCALL); + else if (vmware_hypercall_mode == CPUID_VMWARE_FEATURES_ECX_VMMCALL) + setup_force_cpu_cap(X86_FEATURE_VMW_VMMCALL); } static void __init vmware_platform_setup(void)