Messages in this thread | | | Subject | Re: [PATCH v2 1/4] x86/vmware: Update platform detection code for VMCALL/VMMCALL hypercalls | From | Thomas Hellström (VMware) <> | Date | Tue, 27 Aug 2019 21:19:54 +0200 |
| |
Thanks for reviewing, Borislav. Comments inline.
On 8/27/19 2:56 PM, Borislav Petkov wrote:
> On Fri, Aug 23, 2019 at 10:13:13AM +0200, Thomas Hellström (VMware) wrote: > >> + >> +#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.
We *do* have checkpatch.pl in the workflow. In this case I figured the warnings actually didn't make sense. There are breaks present and -Wimplicit-fallthrough doesn't complain...
(unsigned int) vmware_hypercall_mode);
> Is that supposed to be debug output? If so, pr_dbg().
This is intentionally intended to be part of the initial output.
> >> + >> 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?
Yes, Perhaps I should add a comment about that.
> > 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; > } > Sure, I'll add that as a separate patch.
For the other comments, I'll fix up and respin.
Thanks,
Thomas
| |