KVM: VMX: check match table
diff mbox series

Message ID 20170926042540.10100-1-nick.desaulniers@gmail.com
State New, archived
Headers show
Series
  • KVM: VMX: check match table
Related show

Commit Message

Nick Desaulniers Sept. 26, 2017, 4:25 a.m. UTC
Fixes the warning:
arch/x86/kvm/vmx.c:64:32: warning: variable 'vmx_cpu_id' is not needed
and will not be emitted [-Wunneeded-internal-declaration]``

Other callers of MODULE_DEVICE_TABLE() seem to check their second
argument during driver init with the x86_match_cpu() function, if their
first argument to MODULE_DEVICE_TABLE() is x86cpu.  The documentation
for x86_match_cpu() seems to agree.

Suggested-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/kernel/cpu/match.c | 2 +-
 arch/x86/kvm/vmx.c          | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Josh Triplett Sept. 26, 2017, 5:12 p.m. UTC | #1
On Mon, Sep 25, 2017 at 09:25:38PM -0700, Nick Desaulniers wrote:
> Fixes the warning:
> arch/x86/kvm/vmx.c:64:32: warning: variable 'vmx_cpu_id' is not needed
> and will not be emitted [-Wunneeded-internal-declaration]``
> 
> Other callers of MODULE_DEVICE_TABLE() seem to check their second
> argument during driver init with the x86_match_cpu() function, if their
> first argument to MODULE_DEVICE_TABLE() is x86cpu.  The documentation
> for x86_match_cpu() seems to agree.
> 
> Suggested-by: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>

Comments below.

> ---
>  arch/x86/kernel/cpu/match.c | 2 +-
>  arch/x86/kvm/vmx.c          | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
> index e42117d5f4d7..fb1aeafa5cc7 100644
> --- a/arch/x86/kernel/cpu/match.c
> +++ b/arch/x86/kernel/cpu/match.c
> @@ -5,7 +5,7 @@
>  #include <linux/slab.h>
>  
>  /**
> - * x86_match_cpu - match current CPU again an array of x86_cpu_ids
> + * x86_match_cpu - match current CPU against an array of x86_cpu_ids

This is a good fix as well, but it shouldn't be in the same commit.

>   * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
>   *         {}.
>   *
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6970249c09fc..e1a00b130935 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -12074,7 +12074,12 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
>  
>  static int __init vmx_init(void)
>  {
> -	int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
> +	int r;
> +
> +	if (!x86_match_cpu(vmx_cpu_id))
> +		return -ENODEV;

Does this make any other checks redundant and removable?

> +
> +	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
>                       __alignof__(struct vcpu_vmx), THIS_MODULE);
>  	if (r)
>  		return r;
> -- 
> 2.11.0
>
Paolo Bonzini Sept. 27, 2017, 12:36 p.m. UTC | #2
On 26/09/2017 19:12, Josh Triplett wrote:
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 6970249c09fc..e1a00b130935 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -12074,7 +12074,12 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
>>  
>>  static int __init vmx_init(void)
>>  {
>> -	int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
>> +	int r;
>> +
>> +	if (!x86_match_cpu(vmx_cpu_id))
>> +		return -ENODEV;
> Does this make any other checks redundant and removable?

It would make sense to place it in cpu_has_kvm_support instead, and the
same in svm.c's has_svm.

But there's a lot of pointless indirection to clean up there...

Paolo

>> +
>> +	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
>>                       __alignof__(struct vcpu_vmx), THIS_MODULE);
>>  	if (r)
>>  		return r;
>> --
kbuild test robot Sept. 27, 2017, 2:14 p.m. UTC | #3
Hi Nick,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.14-rc2 next-20170927]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nick-Desaulniers/KVM-VMX-check-match-table/20170927-213040
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: i386-randconfig-x076-201739 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86//kvm/vmx.c: In function 'vmx_init':
>> arch/x86//kvm/vmx.c:12080:7: error: implicit declaration of function 'x86_match_cpu' [-Werror=implicit-function-declaration]
     if (!x86_match_cpu(vmx_cpu_id))
          ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/x86_match_cpu +12080 arch/x86//kvm/vmx.c

 12075	
 12076	static int __init vmx_init(void)
 12077	{
 12078		int r;
 12079	
 12080		if (!x86_match_cpu(vmx_cpu_id))
 12081			return -ENODEV;
 12082	
 12083		r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
 12084	                     __alignof__(struct vcpu_vmx), THIS_MODULE);
 12085		if (r)
 12086			return r;
 12087	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Nick Desaulniers Sept. 30, 2017, 11:22 p.m. UTC | #4
On Wed, Sep 27, 2017 at 02:36:03PM +0200, Paolo Bonzini wrote:
> On 26/09/2017 19:12, Josh Triplett wrote:
> > Does this make any other checks redundant and removable?
> 
> It would make sense to place it in cpu_has_kvm_support instead

cpu_has_kvm_support() or cpu_has_vmx()?

>, and the same in svm.c's has_svm.

I don't follow (but I also don't know what any of these three letter
acryonyms acronyms stand for), does svm depend on vmx or vice-versa?
Paolo Bonzini Oct. 3, 2017, 9:42 a.m. UTC | #5
On 01/10/2017 01:22, Nick Desaulniers wrote:
> On Wed, Sep 27, 2017 at 02:36:03PM +0200, Paolo Bonzini wrote:
>> On 26/09/2017 19:12, Josh Triplett wrote:
>>> Does this make any other checks redundant and removable?
>>
>> It would make sense to place it in cpu_has_kvm_support instead
> 
> cpu_has_kvm_support() or cpu_has_vmx()?
> 
>> , and the same in svm.c's has_svm.
> 
> I don't follow (but I also don't know what any of these three letter
> acryonyms acronyms stand for), does svm depend on vmx or vice-versa?

Neither, one is Intel (VMX), the other is AMD (SVM).

Would this work for you?  And again, is this only with clang?

---------- 8< ------------
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: [PATCH] KVM: clean up virtext.h

virtext.h does a lot of complicated checks where it could just use
boot_cpu_has instead.  Remove all the cruft and stop using it in KVM
even, because KVM can just use x86_match_cpu.

As a side effect, this fixes

arch/x86/kvm/vmx.c:64:32: warning: variable vmx_cpu_id is not needed
and will not be emitted [-Wunneeded-internal-declaration]

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 0116b2ee9e64..c7f7d5008d75 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -25,13 +25,6 @@
  * VMX functions:
  */
 
-static inline int cpu_has_vmx(void)
-{
-	unsigned long ecx = cpuid_ecx(1);
-	return test_bit(5, &ecx); /* CPUID.1:ECX.VMX[bit 5] -> VT */
-}
-
-
 /** Disable VMX on the current CPU
  *
  * vmxoff causes a undefined-opcode exception if vmxon was not run
@@ -49,22 +42,13 @@ static inline int cpu_vmx_enabled(void)
 	return __read_cr4() & X86_CR4_VMXE;
 }
 
-/** Disable VMX if it is enabled on the current CPU
- *
- * You shouldn't call this if cpu_has_vmx() returns 0.
- */
-static inline void __cpu_emergency_vmxoff(void)
-{
-	if (cpu_vmx_enabled())
-		cpu_vmxoff();
-}
-
 /** Disable VMX if it is supported and enabled on the current CPU
  */
 static inline void cpu_emergency_vmxoff(void)
 {
-	if (cpu_has_vmx())
-		__cpu_emergency_vmxoff();
+	if (boot_cpu_has(X86_FEATURE_VMX) &&
+	    cpu_vmx_enabled())
+		cpu_vmxoff();
 }
 
 
@@ -74,36 +58,6 @@ static inline void cpu_emergency_vmxoff(void)
  * SVM functions:
  */
 
-/** Check if the CPU has SVM support
- *
- * You can use the 'msg' arg to get a message describing the problem,
- * if the function returns zero. Simply pass NULL if you are not interested
- * on the messages; gcc should take care of not generating code for
- * the messages on this case.
- */
-static inline int cpu_has_svm(const char **msg)
-{
-	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) {
-		if (msg)
-			*msg = "not amd";
-		return 0;
-	}
-
-	if (boot_cpu_data.extended_cpuid_level < SVM_CPUID_FUNC) {
-		if (msg)
-			*msg = "can't execute cpuid_8000000a";
-		return 0;
-	}
-
-	if (!boot_cpu_has(X86_FEATURE_SVM)) {
-		if (msg)
-			*msg = "svm not available";
-		return 0;
-	}
-	return 1;
-}
-
-
 /** Disable SVM on the current CPU
  *
  * You should call this only if cpu_has_svm() returned true.
@@ -117,11 +71,17 @@ static inline void cpu_svm_disable(void)
 	wrmsrl(MSR_EFER, efer & ~EFER_SVME);
 }
 
+static inline int cpu_svm_enabled(void)
+{
+	return rdmsrl(MSR_EFER) & EFER_SVME;
+}
+
 /** Makes sure SVM is disabled, if it is supported on the CPU
  */
 static inline void cpu_emergency_svm_disable(void)
 {
-	if (cpu_has_svm(NULL))
+	if (boot_cpu_has(X86_FEATURE_SVM) &&
+	    cpu_svm_enabled())
 		cpu_svm_disable();
 }
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0e68f0b3cbf7..4ad26be59327 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -736,14 +736,7 @@ static void svm_init_osvw(struct kvm_vcpu *vcpu)
 
 static int has_svm(void)
 {
-	const char *msg;
-
-	if (!cpu_has_svm(&msg)) {
-		printk(KERN_INFO "has_svm: %s\n", msg);
-		return 0;
-	}
-
-	return 1;
+	return x86_match_cpu(svm_cpu_id);
 }
 
 static void svm_hardware_disable(void)
@@ -769,10 +762,6 @@ static int svm_hardware_enable(void)
 	if (efer & EFER_SVME)
 		return -EBUSY;
 
-	if (!has_svm()) {
-		pr_err("%s: err EOPNOTSUPP on %d\n", __func__, me);
-		return -EINVAL;
-	}
 	sd = per_cpu(svm_data, me);
 	if (!sd) {
 		pr_err("%s: svm_data is NULL on %d\n", __func__, me);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a2b804e10c95..9f6bcd9dd3c6 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3468,7 +3468,7 @@ static void vmx_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
 
 static __init int cpu_has_kvm_support(void)
 {
-	return cpu_has_vmx();
+	return x86_match_cpu(vmx_cpu_id);
 }
 
 static __init int vmx_disabled_by_bios(void)
Nick Desaulniers Oct. 4, 2017, 2:54 a.m. UTC | #6
On Tue, Oct 03, 2017 at 11:42:18AM +0200, Paolo Bonzini wrote:
> On 01/10/2017 01:22, Nick Desaulniers wrote:
> > I don't follow (but I also don't know what any of these three letter
> > acryonyms acronyms stand for), does svm depend on vmx or vice-versa?
> Neither, one is Intel (VMX), the other is AMD (SVM).

Oh, neat, did not realize the vendors did not have different names for
their virtualization extensions.

https://rtfmp.com/2016/03/21/what-does-vmx-svm-cpu-flags-mean/

> Would this work for you?

It doesn't apply cleanly on Linus' tree, or the KVM tree master branch,
so I couldn't fully test it.  But it does look like it will do the
trick.

> And again, is this only with clang?

Indeed the warning was coming from Clang, but looks like some
additional cleanup was done, which is good.

Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Nick Desaulniers Oct. 4, 2017, 2:58 a.m. UTC | #7
- avi

I think an extra `did not` made it in to that last email...sorry!

On Tue, Oct 3, 2017 at 7:54 PM, Nick Desaulniers
<nick.desaulniers@gmail.com> wrote:
> On Tue, Oct 03, 2017 at 11:42:18AM +0200, Paolo Bonzini wrote:
>> On 01/10/2017 01:22, Nick Desaulniers wrote:
>> > I don't follow (but I also don't know what any of these three letter
>> > acryonyms acronyms stand for), does svm depend on vmx or vice-versa?
>> Neither, one is Intel (VMX), the other is AMD (SVM).
>
> Oh, neat, did not realize the vendors did not have different names for
> their virtualization extensions.
>
> https://rtfmp.com/2016/03/21/what-does-vmx-svm-cpu-flags-mean/
>
>> Would this work for you?
>
> It doesn't apply cleanly on Linus' tree, or the KVM tree master branch,
> so I couldn't fully test it.  But it does look like it will do the
> trick.
>
>> And again, is this only with clang?
>
> Indeed the warning was coming from Clang, but looks like some
> additional cleanup was done, which is good.
>
> Reviewed-by: Nick Desaulniers <nick.desaulniers@gmail.com>

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
index e42117d5f4d7..fb1aeafa5cc7 100644
--- a/arch/x86/kernel/cpu/match.c
+++ b/arch/x86/kernel/cpu/match.c
@@ -5,7 +5,7 @@ 
 #include <linux/slab.h>
 
 /**
- * x86_match_cpu - match current CPU again an array of x86_cpu_ids
+ * x86_match_cpu - match current CPU against an array of x86_cpu_ids
  * @match: Pointer to array of x86_cpu_ids. Last entry terminated with
  *         {}.
  *
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6970249c09fc..e1a00b130935 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -12074,7 +12074,12 @@  static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 
 static int __init vmx_init(void)
 {
-	int r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
+	int r;
+
+	if (!x86_match_cpu(vmx_cpu_id))
+		return -ENODEV;
+
+	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
                      __alignof__(struct vcpu_vmx), THIS_MODULE);
 	if (r)
 		return r;