| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190827125636.GE29752@zn.tnic>
Date: Tue, 27 Aug 2019 14:56:36 +0200
From: Borislav Petkov <bp@...en8.de>
To: Thomas Hellström (VMware)
<thomas_os@...pmail.org>
Cc: linux-kernel@...r.kernel.org, pv-drivers@...are.com,
linux-graphics-maintainer@...are.com,
Thomas Hellstrom <thellstrom@...are.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
dri-devel@...ts.freedekstop.org, Doug Covelli <dcovelli@...are.com>
Subject: Re: [PATCH v2 1/4] x86/vmware: Update platform detection code for
VMCALL/VMMCALL hypercalls
On Fri, Aug 23, 2019 at 10:13:13AM +0200, Thomas Hellström (VMware) wrote:
> From: Thomas Hellstrom <thellstrom@...are.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
Pls use passive voice in your commit message: no "we" or "I", etc, and
describe your changes in imperative mood.
> separate macro versions since the alternatives self-patching has not
> been performed at platform detection time.
>
> Also put tighter constraints on the assembly input parameters.
>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: <x86@...nel.org>
> Cc: <dri-devel@...ts.freedekstop.org>
> Co-developed-by: Doug Covelli <dcovelli@...are.com>
> Signed-off-by: Doug Covelli <dcovelli@...are.com>
> Signed-off-by: Thomas Hellstrom <thellstrom@...are.com>
> Reviewed-by: Doug Covelli <dcovelli@...are.com>
> ---
> arch/x86/kernel/cpu/vmware.c | 88 +++++++++++++++++++++++++++++-------
> 1 file changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index 3c648476d4fb..fcb84b1e099e 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -30,34 +30,70 @@
> #include <asm/hypervisor.h>
> #include <asm/timer.h>
> #include <asm/apic.h>
> +#include <asm/svm.h>
That include is for?
> #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"(0), "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"(0), "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: \
Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense:
WARNING: Possible switch case/default not preceded by break or fallthrough comment
#110: FILE: arch/x86/kernel/cpu/vmware.c:81:
+ case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
WARNING: Possible switch case/default not preceded by break or fallthrough comment
#113: FILE: arch/x86/kernel/cpu/vmware.c:84:
+ default:
In this case, we're going to enable -Wimplicit-fallthrough by default at
some point.
> + 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 +172,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 +210,16 @@ static void __init vmware_platform_setup(void)
> vmware_set_capabilities();
> }
>
> +static u8
No need for that linebreak here.
> +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 +233,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);
Is that supposed to be debug output? If so, pr_dbg().
> +
> return CPUID_VMWARE_INFO_LEAF;
> + }
> } else if (dmi_available && dmi_name_in_serial("VMware") &&
> __vmware_platform())
What sets vmware_hypercall_mode in this case? Or is the 0 magic to mean,
use the default: VMWARE_PORT inl call?
Also, you could restructure that function something like this to save yourself
an indentation level or two and make it more easily readable:
static uint32_t __init vmware_platform(void)
{
unsigned int hyper_vendor_id[3];
unsigned int eax;
if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
if (dmi_available && dmi_name_in_serial("VMware") && __vmware_platform())
return 1;
}
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 (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;
}
return 0;
}
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Powered by blists - more mailing lists