[PATCH 0/4] Add support for updated vmware hypercall instruction

[PATCH 0/4] Add support for updated vmware hypercall instruction

[Thomas Hellström (VMware)]

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: <linux-input@vger.kernel.org>

VMware has started using "vmcall" / "vmmcall" instead of an inl instruction
for the "backdoor" interface. This series detects support for those
instructions.
Outside of the platform code we use the "ALTERNATIVES" self-patching
mechanism similarly to how this is done with KVM.
Unfortunately we need two x86 cpu feature flags for this, since we need
the default instruction to be "inl". IIRC the vmmouse driver is used by
other virtualization solutions than VMware, and those might break if
they encounter any of the other instructions.

[PATCH 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

Vmware has historically used an "inl" instruction for this, but recent
hardware versions support using VMCALL/VMMCALL instead, so use this method
if supported at platform detection time. We explicitly need to code
separate macro versions since the alternatives self-patching has not
been performed at platform detection time.

We also put tighter constraints on the assembly input parameters and update
the SPDX license info.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>
Co-developed-by: Doug Covelli <dcovelli@vmware.com>
Signed-off-by: Doug Covelli <dcovelli@vmware.com>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Doug Covelli <dcovelli@vmware.com>
---
 arch/x86/kernel/cpu/vmware.c | 89 +++++++++++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 3c648476d4fb..bf6786d86f2e 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
 /*
  * VMware Detection code.
  *
@@ -30,34 +31,70 @@
 #include <asm/hypervisor.h>
 #include <asm/timer.h>
 #include <asm/apic.h>
+#include <asm/svm.h>
 
 #undef pr_fmt
 #define pr_fmt(fmt)	"vmware: " fmt
 
-#define CPUID_VMWARE_INFO_LEAF	0x40000000
+#define CPUID_VMWARE_INFO_LEAF               0x40000000
+#define CPUID_VMWARE_FEATURES_LEAF           0x40000010
+#define CPUID_VMWARE_FEATURES_ECX_VMMCALL    BIT(0)
+#define CPUID_VMWARE_FEATURES_ECX_VMCALL     BIT(1)
+
 #define VMWARE_HYPERVISOR_MAGIC	0x564D5868
 #define VMWARE_HYPERVISOR_PORT	0x5658
 
-#define VMWARE_PORT_CMD_GETVERSION	10
-#define VMWARE_PORT_CMD_GETHZ		45
-#define VMWARE_PORT_CMD_GETVCPU_INFO	68
-#define VMWARE_PORT_CMD_LEGACY_X2APIC	3
-#define VMWARE_PORT_CMD_VCPU_RESERVED	31
+#define VMWARE_CMD_GETVERSION    10
+#define VMWARE_CMD_GETHZ         45
+#define VMWARE_CMD_GETVCPU_INFO  68
+#define VMWARE_CMD_LEGACY_X2APIC  3
+#define VMWARE_CMD_VCPU_RESERVED 31
 
 #define VMWARE_PORT(cmd, eax, ebx, ecx, edx)				\
 	__asm__("inl (%%dx)" :						\
-			"=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) :	\
-			"0"(VMWARE_HYPERVISOR_MAGIC),			\
-			"1"(VMWARE_PORT_CMD_##cmd),			\
-			"2"(VMWARE_HYPERVISOR_PORT), "3"(UINT_MAX) :	\
-			"memory");
+		"=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) :		\
+		"a"(VMWARE_HYPERVISOR_MAGIC),				\
+		"c"(VMWARE_CMD_##cmd),					\
+		"d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) :		\
+		"memory")
+
+#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx)				\
+	__asm__("vmcall" :						\
+		"=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) :		\
+		"a"(VMWARE_HYPERVISOR_MAGIC),				\
+		"c"(VMWARE_CMD_##cmd),					\
+		"d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) :		\
+		"memory")
+
+#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx)                         \
+	__asm__("vmmcall" :						\
+		"=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) :		\
+		"a"(VMWARE_HYPERVISOR_MAGIC),				\
+		"c"(VMWARE_CMD_##cmd),					\
+		"d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) :		\
+		"memory")
+
+#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do {		\
+	switch (vmware_hypercall_mode) {			\
+	case CPUID_VMWARE_FEATURES_ECX_VMCALL:			\
+		VMWARE_VMCALL(cmd, eax, ebx, ecx, edx);		\
+		break;						\
+	case CPUID_VMWARE_FEATURES_ECX_VMMCALL:			\
+		VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx);	\
+		break;						\
+	default:						\
+		VMWARE_PORT(cmd, eax, ebx, ecx, edx);		\
+		break;						\
+	}							\
+	} while (0)
 
 static unsigned long vmware_tsc_khz __ro_after_init;
+static u8 vmware_hypercall_mode     __ro_after_init;
 
 static inline int __vmware_platform(void)
 {
 	uint32_t eax, ebx, ecx, edx;
-	VMWARE_PORT(GETVERSION, eax, ebx, ecx, edx);
+	VMWARE_CMD(GETVERSION, eax, ebx, ecx, edx);
 	return eax != (uint32_t)-1 && ebx == VMWARE_HYPERVISOR_MAGIC;
 }
 
@@ -136,7 +173,7 @@ static void __init vmware_platform_setup(void)
 	uint32_t eax, ebx, ecx, edx;
 	uint64_t lpj, tsc_khz;
 
-	VMWARE_PORT(GETHZ, eax, ebx, ecx, edx);
+	VMWARE_CMD(GETHZ, eax, ebx, ecx, edx);
 
 	if (ebx != UINT_MAX) {
 		lpj = tsc_khz = eax | (((uint64_t)ebx) << 32);
@@ -174,6 +211,16 @@ static void __init vmware_platform_setup(void)
 	vmware_set_capabilities();
 }
 
+static u8
+vmware_select_hypercall(void)
+{
+	int eax, ebx, ecx, edx;
+
+	cpuid(CPUID_VMWARE_FEATURES_LEAF, &eax, &ebx, &ecx, &edx);
+	return (ecx & (CPUID_VMWARE_FEATURES_ECX_VMMCALL |
+		       CPUID_VMWARE_FEATURES_ECX_VMCALL));
+}
+
 /*
  * While checking the dmi string information, just checking the product
  * serial key should be enough, as this will always have a VMware
@@ -187,8 +234,16 @@ static uint32_t __init vmware_platform(void)
 
 		cpuid(CPUID_VMWARE_INFO_LEAF, &eax, &hyper_vendor_id[0],
 		      &hyper_vendor_id[1], &hyper_vendor_id[2]);
-		if (!memcmp(hyper_vendor_id, "VMwareVMware", 12))
+		if (!memcmp(hyper_vendor_id, "VMwareVMware", 12)) {
+			if (eax >= CPUID_VMWARE_FEATURES_LEAF)
+				vmware_hypercall_mode =
+					vmware_select_hypercall();
+
+			pr_info("hypercall mode: 0x%02x\n",
+				(unsigned int) vmware_hypercall_mode);
+
 			return CPUID_VMWARE_INFO_LEAF;
+		}
 	} else if (dmi_available && dmi_name_in_serial("VMware") &&
 		   __vmware_platform())
 		return 1;
@@ -200,9 +255,9 @@ static uint32_t __init vmware_platform(void)
 static bool __init vmware_legacy_x2apic_available(void)
 {
 	uint32_t eax, ebx, ecx, edx;
-	VMWARE_PORT(GETVCPU_INFO, eax, ebx, ecx, edx);
-	return (eax & (1 << VMWARE_PORT_CMD_VCPU_RESERVED)) == 0 &&
-	       (eax & (1 << VMWARE_PORT_CMD_LEGACY_X2APIC)) != 0;
+	VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
+	return (eax & (1 << VMWARE_CMD_VCPU_RESERVED)) == 0 &&
+	       (eax & (1 << VMWARE_CMD_LEGACY_X2APIC)) != 0;
 }
 
 const __initconst struct hypervisor_x86 x86_hyper_vmware = {
-- 
2.20.1

Re: [PATCH 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls

[Thomas Gleixner]

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

On Sun, 18 Aug 2019, Thomas Hellström (VMware) wrote:

> From: Thomas Hellstrom <thellstrom@vmware.com>
> 
> Vmware has historically used an "inl" instruction for this, but recent
> hardware versions support using VMCALL/VMMCALL instead, so use this method
> if supported at platform detection time. We explicitly need to code
> separate macro versions since the alternatives self-patching has not
> been performed at platform detection time.
> 
> We also put tighter constraints on the assembly input parameters and update
> the SPDX license info.

Can you please split the license stuff into a separate patch? You know, one
patch one thing. It's documented for a reason.

While at it could you please ask your legal folks whether that custom
license boilerplate can go away as well?

Thanks,

	tglx

Re: [PATCH 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls

[Thomas Hellström (VMware)]

On 8/18/19 9:20 PM, Thomas Gleixner wrote:
> On Sun, 18 Aug 2019, Thomas Hellström (VMware) wrote:
>
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
>> Vmware has historically used an "inl" instruction for this, but recent
>> hardware versions support using VMCALL/VMMCALL instead, so use this method
>> if supported at platform detection time. We explicitly need to code
>> separate macro versions since the alternatives self-patching has not
>> been performed at platform detection time.
>>
>> We also put tighter constraints on the assembly input parameters and update
>> the SPDX license info.
> Can you please split the license stuff into a separate patch? You know, one
> patch one thing. It's documented for a reason.
>
> While at it could you please ask your legal folks whether that custom
> license boilerplate can go away as well?

Sure, I'll drop that from the series for now.

Thanks,

/Thomas

Re: [Pv-drivers] [PATCH 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls

[Darren Hart]

> On Aug 18, 2019, at 12:20 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> While at it could you please ask your legal folks whether that custom
> license boilerplate can go away as well?

If you’re referring to the GPL boilerplate with “no warranty" and physical
address, then yes, as a matter of best practice (at VMware), that can and
should all be removed when adding the SPDX identifier - and of course, as
you said, be done as a separate patch.

Thanks,

-- 
Darren Hart

[PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

This is intended to be used by drivers using the backdoor, and
we follow the kvm example using alternatives self-patching to
choose between vmcall, vmmcall and inl instructions.

This patch defines two new x86 cpu feature flags.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Doug Covelli <dcovelli@vmware.com>
---
 MAINTAINERS                        |  1 +
 arch/x86/include/asm/cpufeatures.h |  2 ++
 arch/x86/include/asm/vmware.h      | 13 +++++++++++++
 3 files changed, 16 insertions(+)
 create mode 100644 arch/x86/include/asm/vmware.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1bd7b9c2d146..412206747270 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..69cecc3bc9cb 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_VMW_VMCALL		( 8*32+18) /* VMware prefers VMCALL hypercall 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..22d2dbfba90f
--- /dev/null
+++ b/arch/x86/include/asm/vmware.h
@@ -0,0 +1,13 @@
+/* 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>
+
+#define VMWARE_HYPERCALL \
+	ALTERNATIVE_2(".byte 0xed", \
+		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
+		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMW_VMMCALL)
+
+#endif
-- 
2.20.1

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Borislav Petkov]

On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:
> From: Thomas Hellstrom <thellstrom@vmware.com>
> 
> This is intended to be used by drivers using the backdoor, and
> we follow the kvm example using alternatives self-patching to
> choose between vmcall, vmmcall and inl instructions.
> 
> This patch defines two new x86 cpu feature flags.

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

Also, s/cpu/CPU/g in the text.

> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Doug Covelli <dcovelli@vmware.com>
> ---
>  MAINTAINERS                        |  1 +
>  arch/x86/include/asm/cpufeatures.h |  2 ++
>  arch/x86/include/asm/vmware.h      | 13 +++++++++++++
>  3 files changed, 16 insertions(+)
>  create mode 100644 arch/x86/include/asm/vmware.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1bd7b9c2d146..412206747270 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..69cecc3bc9cb 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_VMW_VMCALL		( 8*32+18) /* VMware prefers VMCALL hypercall instruction */
> +#define X86_FEATURE_VMW_VMMCALL		( 8*32+19) /* VMware prefers VMMCALL hypercall instruction */

Those are not set anywhere in the patchset. Please send them with their
user.

Then, there already is X86_FEATURE_VMMCALL which you can use too, I'd
guess.

Which would turn the macro into

	ALTERNATIVE_2(".byte 0xed", \
		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMMCALL)

and then you end up adding a single new feature bit X86_FEATURE_VMW_VMCALL.

Also, I don't see a reason to show the synthetic bit in /proc/cpuinfo
so when you define it, add "":

#define X86_FEATURE_VMW_VMCALL		( 8*32+18) /* "" VMware prefers VMCALL hypercall instruction */
						      ^
						      |
						      |
						      this here.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Borislav Petkov]

On Sun, Aug 18, 2019 at 10:19:42PM +0200, Borislav Petkov wrote:
> #define X86_FEATURE_VMW_VMCALL		( 8*32+18) /* "" VMware prefers VMCALL hypercall instruction */

And you can call that one something generic too: X86_FEATURE_VMCALL.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Hellström (VMware)]

On 8/18/19 10:19 PM, Borislav Petkov wrote:
> On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
>> This is intended to be used by drivers using the backdoor, and
>> we follow the kvm example using alternatives self-patching to
>> choose between vmcall, vmmcall and inl instructions.
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 998c2cc08363..69cecc3bc9cb 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_VMW_VMCALL		( 8*32+18) /* VMware prefers VMCALL hypercall instruction */
>> +#define X86_FEATURE_VMW_VMMCALL		( 8*32+19) /* VMware prefers VMMCALL hypercall instruction */
> Those are not set anywhere in the patchset. Please send them with their
> user.

Thanks for reviewing. Hmm, That hunk appears to have been lost. I'll 
restore in v2.

>
> Then, there already is X86_FEATURE_VMMCALL which you can use too, I'd
> guess.

Unfortunately we can't use it, because it's unconditionally set on AMD 
even if the VMware hypervisor
doesn't support it (by version or by configuration).

I was thinking of having the VMware platform code clear it if vmmcall 
wasn't supported  but that would most probably conflict with other 
hypervisors using the vmmouse driver, still using the old "inl" 
interface for the vmmouse backdoor, but otherwise requiring vmmcall for 
other hypercalls.

>
> Also, I don't see a reason to show the synthetic bit in /proc/cpuinfo
> so when you define it, add "":
>
> #define X86_FEATURE_VMW_VMCALL		( 8*32+18) /* "" VMware prefers VMCALL hypercall instruction */
> 						      ^
> 						      |
> 						      |
> 						      this here.

Ah. I wasn't aware of that. I'll add. Thanks.

Thomas


>
> Thx.
>

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Borislav Petkov]

On Mon, Aug 19, 2019 at 12:28:05AM +0200, Thomas Hellström (VMware) wrote:
> Unfortunately we can't use it, because it's unconditionally set on AMD even
> if the VMware hypervisor
> doesn't support it (by version or by configuration).

AMD sets it because they don't support VMCALL. Nothing stops us from
making that conditional depending on what the hypervisor can/supports.
I'm thinking it would be even cleaner if we use those two flags:

X86_FEATURE_VMMCALL
X86_FEATURE_VMCALL

to denote hw support for either one or the other instruction and switch
accordingly. Just like KVM does.

In your case, the HV would set the preferred flag in
arch/x86/kernel/cpu/vmware.c - just like the others do in their
respective CPU init files - and the alternatives code would switch to it
when it runs.

Or is there more? :)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Hellström (VMware)]

On 8/19/19 8:25 AM, Borislav Petkov wrote:
> On Mon, Aug 19, 2019 at 12:28:05AM +0200, Thomas Hellström (VMware) wrote:
>> Unfortunately we can't use it, because it's unconditionally set on AMD even
>> if the VMware hypervisor
>> doesn't support it (by version or by configuration).
> AMD sets it because they don't support VMCALL. Nothing stops us from
> making that conditional depending on what the hypervisor can/supports.
> I'm thinking it would be even cleaner if we use those two flags:
>
> X86_FEATURE_VMMCALL
> X86_FEATURE_VMCALL
>
> to denote hw support for either one or the other instruction and switch
> accordingly. Just like KVM does.
>
> In your case, the HV would set the preferred flag in
> arch/x86/kernel/cpu/vmware.c - just like the others do in their
> respective CPU init files - and the alternatives code would switch to it
> when it runs.
>
> Or is there more? :)

Yes, unfortunately. I agree this is is the cleanest solution and my 
first choice. It would work for VMware, but AFAICT it might break setups 
of other hypervisors running at least the vmmouse driver. Quick googling 
tells me there are likely QEMU/KVM setups that do this. My thinking is 
they would have to set X86_FEATURE_VMMCALL on AMD to get the 
kvm_hypercall right, but that would mean the vmmouse hypercall also uses 
vmmcall, which they probably haven't implemented (yet). So the safe way 
would be to use at least XF86_FEATURE_VMW_VMMCALL + XF86_FEATURE_VMCALL 
until that has happened.

/Thomas


>
> Thx.
>

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Peter Zijlstra]

On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:

> +#define VMWARE_HYPERCALL \
> +	ALTERNATIVE_2(".byte 0xed", \
> +		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
> +		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMW_VMMCALL)

For sanity, could we either add comments, or macros for those
instrucions?

Something like:

#define INSN_INL	0xed
#define INSN_VMCALL	0x0f,0x01,0xc1
#define INSN_VMMCALL	0x0f,0x01,0xd9

#define VMWARE_HYPERCALL \
	ALTERNATIVE_2(_ASM_MK_NOP(INSN_INL),
		      _ASM_MK_NOP(INSN_VMCALL), X86_FEATURE_VMCALL,
		      _ASM_MK_NOP(INSN_VMMCALL), X86_FEATURE_VMMCALL)

With possibly a patch that does 's/_ASM_MK_NOP/_ASM_MK_INSN/' on
arch/x86/ for further sanity :-)

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Hellström (VMware)]

On 8/20/19 1:32 PM, Peter Zijlstra wrote:
> On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:
>
>> +#define VMWARE_HYPERCALL \
>> +	ALTERNATIVE_2(".byte 0xed", \
>> +		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
>> +		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMW_VMMCALL)
> For sanity, could we either add comments, or macros for those
> instrucions?

Hmm. Here I followed and slightly extended what was done in asm/kvm_para.h.

What confuses me a bit is, if it's clarity we're after, why don't people use

#define VMWARE_HYPERCALL 					\
	ALTERNATIVE_2("inl (%%dx)", 				\
		      "vmcall", X86_FEATURE_VMW_VMCALL,		\	
		      "vmmcall", X86_FEATURE_VMW_VMMCALL)

Seems to build fine here. Is it fear of old assemblers not supporting, for example vmmcall

Thanks,
/Thomas
  

> Something like:
>
> #define INSN_INL	0xed
> #define INSN_VMCALL	0x0f,0x01,0xc1
> #define INSN_VMMCALL	0x0f,0x01,0xd9
>
> #define VMWARE_HYPERCALL \
> 	ALTERNATIVE_2(_ASM_MK_NOP(INSN_INL),
> 		      _ASM_MK_NOP(INSN_VMCALL), X86_FEATURE_VMCALL,
> 		      _ASM_MK_NOP(INSN_VMMCALL), X86_FEATURE_VMMCALL)
>
> With possibly a patch that does 's/_ASM_MK_NOP/_ASM_MK_INSN/' on
> arch/x86/ for further sanity :-)

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Gleixner]

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

On Tue, 20 Aug 2019, Thomas Hellström (VMware) wrote:

> On 8/20/19 1:32 PM, Peter Zijlstra wrote:
> > On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:
> > 
> > > +#define VMWARE_HYPERCALL \
> > > +	ALTERNATIVE_2(".byte 0xed", \
> > > +		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
> > > +		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMW_VMMCALL)
> > For sanity, could we either add comments, or macros for those
> > instrucions?
> 
> Hmm. Here I followed and slightly extended what was done in asm/kvm_para.h.
> 
> What confuses me a bit is, if it's clarity we're after, why don't people use
> 
> #define VMWARE_HYPERCALL 					\
> 	ALTERNATIVE_2("inl (%%dx)", 				\
> 		      "vmcall", X86_FEATURE_VMW_VMCALL,		\	
> 		      "vmmcall", X86_FEATURE_VMW_VMMCALL)
> 
> Seems to build fine here. Is it fear of old assemblers not supporting, for
> example vmmcall

The requirement for binutils is version >= 2.21. If 2.21 supports vmcall and
vmmcall all good.

Thanks,

	tglx

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Hellström (VMware)]

On 8/20/19 3:42 PM, Thomas Gleixner wrote:
> On Tue, 20 Aug 2019, Thomas Hellström (VMware) wrote:
>
>> On 8/20/19 1:32 PM, Peter Zijlstra wrote:
>>> On Sun, Aug 18, 2019 at 04:33:14PM +0200, Thomas Hellström (VMware) wrote:
>>>
>>>> +#define VMWARE_HYPERCALL \
>>>> +	ALTERNATIVE_2(".byte 0xed", \
>>>> +		      ".byte 0x0f, 0x01, 0xc1", X86_FEATURE_VMW_VMCALL,	\
>>>> +		      ".byte 0x0f, 0x01, 0xd9", X86_FEATURE_VMW_VMMCALL)
>>> For sanity, could we either add comments, or macros for those
>>> instrucions?
>> Hmm. Here I followed and slightly extended what was done in asm/kvm_para.h.
>>
>> What confuses me a bit is, if it's clarity we're after, why don't people use
>>
>> #define VMWARE_HYPERCALL 					\
>> 	ALTERNATIVE_2("inl (%%dx)", 				\
>> 		      "vmcall", X86_FEATURE_VMW_VMCALL,		\	
>> 		      "vmmcall", X86_FEATURE_VMW_VMMCALL)
>>
>> Seems to build fine here. Is it fear of old assemblers not supporting, for
>> example vmmcall
> The requirement for binutils is version >= 2.21. If 2.21 supports vmcall and
> vmmcall all good.
>
> Thanks,
>
> 	tglx

So I tested 2.20.1 and 2.21.1 from ftp.gnu.org/gnu/binutils, and both 
seem to assemble (as-new) and disassemble (objdump -S) vmcall and 
vmmcall fine so I think we should be OK using the mnemonic format then.

Thanks,

Thomas




<https://ftp.gnu.org/gnu/binutils/>

Re: [PATCH 2/4] x86/vmware: Add a header file for hypercall definitions

[Thomas Gleixner]

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

On Tue, 20 Aug 2019, Thomas Hellström (VMware) wrote:
> On 8/20/19 3:42 PM, Thomas Gleixner wrote:
> > > What confuses me a bit is, if it's clarity we're after, why don't people
> > > use
> > > 
> > > #define VMWARE_HYPERCALL 					\
> > > 	ALTERNATIVE_2("inl (%%dx)", 				\
> > > 		      "vmcall", X86_FEATURE_VMW_VMCALL,		\	
> > > 		      "vmmcall", X86_FEATURE_VMW_VMMCALL)
> > > 
> > > Seems to build fine here. Is it fear of old assemblers not supporting, for
> > > example vmmcall
> > The requirement for binutils is version >= 2.21. If 2.21 supports vmcall and
> > vmmcall all good.
> > 
> > Thanks,
> > 
> > 	tglx
> 
> So I tested 2.20.1 and 2.21.1 from ftp.gnu.org/gnu/binutils, and both seem to
> assemble (as-new) and disassemble (objdump -S) vmcall and vmmcall fine so I
> think we should be OK using the mnemonic format then.

Perfect!

[PATCH 3/4] drm/vmwgfx: Update the backdoor call with support for new instructions

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

Use the definition provided by include/asm/vmware.h

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Doug Covelli <dcovelli@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_msg.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.h b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.h
index 4907e50fb20a..7fc3ff63b1a8 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.h
@@ -32,6 +32,7 @@
 #ifndef _VMWGFX_MSG_H
 #define _VMWGFX_MSG_H
 
+#include <asm/vmware.h>
 
 /**
  * Hypervisor-specific bi-directional communication channel.  Should never
@@ -57,7 +58,7 @@
 		 port_num, magic,		\
 		 eax, ebx, ecx, edx, si, di)	\
 ({						\
-	asm volatile ("inl %%dx, %%eax;" :	\
+	asm volatile (VMWARE_HYPERCALL :	\
 		"=a"(eax),			\
 		"=b"(ebx),			\
 		"=c"(ecx),			\
-- 
2.20.1

[PATCH 4/4] input/vmmouse: Update the backdoor call with support for new instructions

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

Use the definition provided by include/asm/vmware.h

Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: <linux-input@vger.kernel.org>
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Doug Covelli <dcovelli@vmware.com>
---
 drivers/input/mouse/vmmouse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c
index 871e5b5ab129..0c7707c7bede 100644
--- a/drivers/input/mouse/vmmouse.c
+++ b/drivers/input/mouse/vmmouse.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <asm/hypervisor.h>
+#include <asm/vmware.h>
 
 #include "psmouse.h"
 #include "vmmouse.h"
@@ -84,7 +85,7 @@ struct vmmouse_data {
 #define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)	\
 ({							\
 	unsigned long __dummy1, __dummy2;		\
-	__asm__ __volatile__ ("inl %%dx" :		\
+	__asm__ __volatile__ (VMWARE_HYPERCALL :	\
 		"=a"(out1),				\
 		"=b"(out2),				\
 		"=c"(out3),				\
-- 
2.20.1

Re: [PATCH 4/4] input/vmmouse: Update the backdoor call with support for new instructions

[Dmitry Torokhov]

On Sun, Aug 18, 2019 at 04:33:16PM +0200, Thomas Hellström (VMware) wrote:
> From: Thomas Hellstrom <thellstrom@vmware.com>
> 
> Use the definition provided by include/asm/vmware.h
> 
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: <linux-input@vger.kernel.org>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Doug Covelli <dcovelli@vmware.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Please feel free to merge with the rest of the patches introducing
VMWARE_HYPERCALL.

> ---
>  drivers/input/mouse/vmmouse.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c
> index 871e5b5ab129..0c7707c7bede 100644
> --- a/drivers/input/mouse/vmmouse.c
> +++ b/drivers/input/mouse/vmmouse.c
> @@ -16,6 +16,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <asm/hypervisor.h>
> +#include <asm/vmware.h>
>  
>  #include "psmouse.h"
>  #include "vmmouse.h"
> @@ -84,7 +85,7 @@ struct vmmouse_data {
>  #define VMMOUSE_CMD(cmd, in1, out1, out2, out3, out4)	\
>  ({							\
>  	unsigned long __dummy1, __dummy2;		\
> -	__asm__ __volatile__ ("inl %%dx" :		\
> +	__asm__ __volatile__ (VMWARE_HYPERCALL :	\
>  		"=a"(out1),				\
>  		"=b"(out2),				\
>  		"=c"(out3),				\
> -- 
> 2.20.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH 4/4] input/vmmouse: Update the backdoor call with support for new instructions

[Thomas Hellström (VMware)]

On 8/21/19 7:09 AM, Dmitry Torokhov wrote:
> On Sun, Aug 18, 2019 at 04:33:16PM +0200, Thomas Hellström (VMware) wrote:
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
>> Use the definition provided by include/asm/vmware.h
>>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: <linux-input@vger.kernel.org>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Reviewed-by: Doug Covelli <dcovelli@vmware.com>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Please feel free to merge with the rest of the patches introducing
> VMWARE_HYPERCALL.

Thanks, Dmitry!

I have a version 2 of the patch were the only difference is that edx is 
initialized to zero which is default for vmcall / vmmcall. Can I use 
your ack for that one as well?

Thanks,

Thomas

Re: [PATCH 4/4] input/vmmouse: Update the backdoor call with support for new instructions

[Dmitry Torokhov]

On Wed, Aug 21, 2019 at 05:48:23PM +0200, Thomas Hellström (VMware) wrote:
> On 8/21/19 7:09 AM, Dmitry Torokhov wrote:
> > On Sun, Aug 18, 2019 at 04:33:16PM +0200, Thomas Hellström (VMware) wrote:
> > > From: Thomas Hellstrom <thellstrom@vmware.com>
> > > 
> > > Use the definition provided by include/asm/vmware.h
> > > 
> > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Cc: <linux-input@vger.kernel.org>
> > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > Reviewed-by: Doug Covelli <dcovelli@vmware.com>
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > Please feel free to merge with the rest of the patches introducing
> > VMWARE_HYPERCALL.
> 
> Thanks, Dmitry!
> 
> I have a version 2 of the patch were the only difference is that edx is
> initialized to zero which is default for vmcall / vmmcall. Can I use your
> ack for that one as well?

Yes, sure.

-- 
Dmitry

Re: [PATCH 0/4] Add support for updated vmware hypercall instruction

[Borislav Petkov]

On Sun, Aug 18, 2019 at 04:33:12PM +0200, Thomas Hellström (VMware) wrote:
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: <linux-input@vger.kernel.org>

In the future, when you CC people on patches, pls CC them for all
patches - not only a subset. Had to fish out 3/4 and 4/4 from lkml.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 0/4] Add support for updated vmware hypercall instruction

[Thomas Hellström (VMware)]

On 8/18/19 8:09 PM, Borislav Petkov wrote:
> On Sun, Aug 18, 2019 at 04:33:12PM +0200, Thomas Hellström (VMware) wrote:
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: <x86@kernel.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: <linux-input@vger.kernel.org>
> In the future, when you CC people on patches, pls CC them for all
> patches - not only a subset. Had to fish out 3/4 and 4/4 from lkml.
>
> Thx.
>
Hmm. Ok. I'll keep that in mind.
I was trying to be clever and avoid spamming people with stuff they 
might not care about.

Thanks,
/Thomas

Re: [PATCH 0/4] Add support for updated vmware hypercall instruction

[Borislav Petkov]

On Sun, Aug 18, 2019 at 09:02:04PM +0200, Thomas Hellström (VMware) wrote:
> I was trying to be clever and avoid spamming people with stuff they might
> not care about.

Sure but if, for example, you're adding VMWARE_HYPERCALL in patch 2 and
the users are in the following patches, it would be helpful if I could
see them too. That is one good reason to CC everybody with the whole
patchset.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.