unneeded internal declaration

unneeded internal declaration

[Nick Desaulniers]

today I noticed I was getting 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]

seems like this was added in commit: e9bda3b3d0ce7 "KVM: VMX:
Auto-load on CPUs with VMX"

seems like other call sites of the MODULE_DEVICE_TABLE typically get
added to an id_table of various driver structs.  Should that be the
case here, or would a `__unused` modifier be a way forward (if so,
please confirm, would be a good first bug for a friend)?

Re: unneeded internal declaration

[Josh Triplett]

On Mon, Sep 25, 2017 at 08:07:41PM -0700, Nick Desaulniers wrote:
> today I noticed I was getting 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]
> 
> seems like this was added in commit: e9bda3b3d0ce7 "KVM: VMX:
> Auto-load on CPUs with VMX"
> 
> seems like other call sites of the MODULE_DEVICE_TABLE typically get
> added to an id_table of various driver structs.  Should that be the
> case here, or would a `__unused` modifier be a way forward (if so,
> please confirm, would be a good first bug for a friend)?

Many of those users seem to have an x86_match_cpu call in their init
function. vmx has its own checks; perhaps it'd make sense to use that
instead, which would also eliminate the warning?

Re: unneeded internal declaration

[Nick Desaulniers]

Good find, will send a patch with your suggestion.  The documentation
for x86_match_cpu() says:

 25  * Arrays used to match for this should also be declared using
 26  * MODULE_DEVICE_TABLE(x86cpu, ...)

Will save another patch for a friend, then.

On Mon, Sep 25, 2017 at 8:35 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Mon, Sep 25, 2017 at 08:07:41PM -0700, Nick Desaulniers wrote:
>> today I noticed I was getting 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]
>>
>> seems like this was added in commit: e9bda3b3d0ce7 "KVM: VMX:
>> Auto-load on CPUs with VMX"
>>
>> seems like other call sites of the MODULE_DEVICE_TABLE typically get
>> added to an id_table of various driver structs.  Should that be the
>> case here, or would a `__unused` modifier be a way forward (if so,
>> please confirm, would be a good first bug for a friend)?
>
> Many of those users seem to have an x86_match_cpu call in their init
> function. vmx has its own checks; perhaps it'd make sense to use that
> instead, which would also eliminate the warning?

[PATCH] KVM: VMX: check match table

[Nick Desaulniers]

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(-)

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;
-- 
2.11.0

Re: [PATCH] KVM: VMX: check match table

[Josh Triplett]

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
> 

Re: [PATCH] KVM: VMX: check match table

[Paolo Bonzini]

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;
>> -- 

Re: [PATCH] KVM: VMX: check match table

[Nick Desaulniers]

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?

Re: [PATCH] KVM: VMX: check match table

[Paolo Bonzini]

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)

Re: [PATCH] KVM: VMX: check match table

[Nick Desaulniers]

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>

Re: [PATCH] KVM: VMX: check match table

[Nick Desaulniers]

- 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>

Re: [PATCH] KVM: VMX: check match table

[kbuild test robot]

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

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24723 bytes --]

Re: unneeded internal declaration

[Paolo Bonzini]

On 26/09/2017 05:07, Nick Desaulniers wrote:
> today I noticed I was getting 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]
> 
> seems like this was added in commit: e9bda3b3d0ce7 "KVM: VMX:
> Auto-load on CPUs with VMX"
> 
> seems like other call sites of the MODULE_DEVICE_TABLE typically get
> added to an id_table of various driver structs.  Should that be the
> case here, or would a `__unused` modifier be a way forward (if so,
> please confirm, would be a good first bug for a friend)?

It would be __used, not __unused.  But I'm not sure then why things are
working for everyone, wouldn't the MODULE_DEVICE_TABLE cause a linking
error?

What compiler is emitting this warning?

Paolo

Re: unneeded internal declaration

[Paolo Bonzini]

On 27/09/2017 14:39, Avi Ghosh wrote:
> Can someone remove me form this thread. I don’t know why I’m on it.

Because you are avi@redhat.com, which belonged to Avi Kivity before you.

In any case, no one can "remove you from this thread".  Everyone
involved would have to remember to remove you from each and every
message that is sent on this thread.  It's much simpler if you use the
"Ignore thread" or "Mute thread" functionality of your mail client.

Paolo

> On Mon, Sep 25, 2017 at 10:07 PM Nick Desaulniers
> <nick.desaulniers@gmail.com <mailto:nick.desaulniers@gmail.com>> wrote:
> 
>     today I noticed I was getting 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]
> 
>     seems like this was added in commit: e9bda3b3d0ce7 "KVM: VMX:
>     Auto-load on CPUs with VMX"
> 
>     seems like other call sites of the MODULE_DEVICE_TABLE typically get
>     added to an id_table of various driver structs.  Should that be the
>     case here, or would a `__unused` modifier be a way forward (if so,
>     please confirm, would be a good first bug for a friend)?
>