| Message ID | 20170926042540.10100-1-nick.desaulniers@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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; >> --
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?
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;
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(-)