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)?
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?
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 <[email protected]> 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?
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 <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
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
>
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;
>> --
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
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 [email protected], 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
> <[email protected] <mailto:[email protected]>> 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)?
>
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
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?