Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739475 > unrolled thread

unneeded internal declaration

Back to article view | Back to linux.kernel

Contents

  unneeded internal declaration Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-09-26 05:10 +0200
    Re: unneeded internal declaration Josh Triplett <josh@joshtriplett.org> - 2017-09-26 05:40 +0200
      Re: unneeded internal declaration Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-09-26 06:20 +0200
        [PATCH] KVM: VMX: check match table Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-09-26 06:30 +0200
          Re: [PATCH] KVM: VMX: check match table Josh Triplett <josh@joshtriplett.org> - 2017-09-26 19:20 +0200
            Re: [PATCH] KVM: VMX: check match table Paolo Bonzini <pbonzini@redhat.com> - 2017-09-27 14:40 +0200
              Re: [PATCH] KVM: VMX: check match table Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-10-01 01:30 +0200
          Re: [PATCH] KVM: VMX: check match table kbuild test robot <lkp@intel.com> - 2017-09-27 16:20 +0200
    Re: unneeded internal declaration Paolo Bonzini <pbonzini@redhat.com> - 2017-09-27 14:40 +0200
    Re: unneeded internal declaration Paolo Bonzini <pbonzini@redhat.com> - 2017-09-27 15:00 +0200

#1739475 — unneeded internal declaration

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

[toc] | [next] | [standalone]

#1739488

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?

[toc] | [prev] | [next] | [standalone]

#1739503

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?

[toc] | [prev] | [next] | [standalone]

#1739504 — [PATCH] KVM: VMX: check match table

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

[toc] | [prev] | [next] | [standalone]

#1740045 — Re: [PATCH] KVM: VMX: check match table

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
> 

[toc] | [prev] | [next] | [standalone]

#1740678 — Re: [PATCH] KVM: VMX: check match table

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

[toc] | [prev] | [next] | [standalone]

#1742704 — Re: [PATCH] KVM: VMX: check match table

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?

[toc] | [prev] | [next] | [standalone]

#1740777 — Re: [PATCH] KVM: VMX: check match table

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1740676

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

[toc] | [prev] | [next] | [standalone]

#1740694

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web