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

[Borislav Petkov]

On Fri, Aug 23, 2019 at 10:13:13AM +0200, Thomas HellstrÃm (VMware) wrote:
> From: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: <x86@xxxxxxxxxx>
> Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx>
> Co-developed-by: Doug Covelli <dcovelli@xxxxxxxxxx>
> Signed-off-by: Doug Covelli <dcovelli@xxxxxxxxxx>
> Signed-off-by: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
> Reviewed-by: Doug Covelli <dcovelli@xxxxxxxxxx>
> ---
>  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.