arch/x86/include/asm/apic.h declares a global variable named `apic'.
Many function arguments from arch/x86/kvm/lapic.h also uses the same
name and thus shadow the global declaration. For each case of
shadowing, rename the function argument from `apic' to `lapic'.
This patch silences below -Wshadow warnings:
| In file included from arch/x86/kernel/../kvm/vmx/capabilities.h:7,
| from arch/x86/kernel/../kvm/vmx/vmx.h:10:
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'kvm_lapic_set_irr':
| arch/x86/kernel/../kvm/vmx/../lapic.h:143:65: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 143 | static inline void kvm_lapic_set_irr(int vec, struct kvm_lapic *apic)
| | ~~~~~~~~~~~~~~~~~~^~~~
| In file included from ./arch/x86/include/asm/kvm_host.h:29,
| from ./include/linux/kvm_host.h:45:
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'kvm_lapic_get_reg':
| arch/x86/kernel/../kvm/vmx/../lapic.h:158:55: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 158 | static inline u32 kvm_lapic_get_reg(struct kvm_lapic *apic, int reg_off)
| | ~~~~~~~~~~~~~~~~~~^~~~
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'kvm_apic_hw_enabled':
| arch/x86/kernel/../kvm/vmx/../lapic.h:174:57: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 174 | static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
| | ~~~~~~~~~~~~~~~~~~^~~~
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'kvm_apic_sw_enabled':
| arch/x86/kernel/../kvm/vmx/../lapic.h:183:58: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 183 | static inline bool kvm_apic_sw_enabled(struct kvm_lapic *apic)
| | ~~~~~~~~~~~~~~~~~~^~~~
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'apic_x2apic_mode':
| arch/x86/kernel/../kvm/vmx/../lapic.h:200:54: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 200 | static inline int apic_x2apic_mode(struct kvm_lapic *apic)
| | ~~~~~~~~~~~~~~~~~~^~~~
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
| arch/x86/kernel/../kvm/vmx/../lapic.h: In function 'kvm_xapic_id':
| arch/x86/kernel/../kvm/vmx/../lapic.h:249:49: warning: declaration of 'apic' shadows a global declaration [-Wshadow]
| 249 | static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
| | ~~~~~~~~~~~~~~~~~~^~~~
| ./arch/x86/include/asm/apic.h:357:21: note: shadowed declaration is here
| 357 | extern struct apic *apic;
| | ^~~~
Signed-off-by: Vincent Mailhol <[email protected]>
---
arch/x86/kvm/lapic.h | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 117a46df5cc1..55abd5e22462 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -156,14 +156,14 @@ static inline void kvm_lapic_set_vector(int vec, void *bitmap)
set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
}
-static inline void kvm_lapic_set_irr(int vec, struct kvm_lapic *apic)
+static inline void kvm_lapic_set_irr(int vec, struct kvm_lapic *lapic)
{
- kvm_lapic_set_vector(vec, apic->regs + APIC_IRR);
+ kvm_lapic_set_vector(vec, lapic->regs + APIC_IRR);
/*
* irr_pending must be true if any interrupt is pending; set it after
* APIC_IRR to avoid race with apic_clear_irr
*/
- apic->irr_pending = true;
+ lapic->irr_pending = true;
}
static inline u32 __kvm_lapic_get_reg(char *regs, int reg_off)
@@ -171,9 +171,9 @@ static inline u32 __kvm_lapic_get_reg(char *regs, int reg_off)
return *((u32 *) (regs + reg_off));
}
-static inline u32 kvm_lapic_get_reg(struct kvm_lapic *apic, int reg_off)
+static inline u32 kvm_lapic_get_reg(struct kvm_lapic *lapic, int reg_off)
{
- return __kvm_lapic_get_reg(apic->regs, reg_off);
+ return __kvm_lapic_get_reg(lapic->regs, reg_off);
}
DECLARE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
@@ -187,19 +187,19 @@ static inline bool lapic_in_kernel(struct kvm_vcpu *vcpu)
extern struct static_key_false_deferred apic_hw_disabled;
-static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
+static inline int kvm_apic_hw_enabled(struct kvm_lapic *lapic)
{
if (static_branch_unlikely(&apic_hw_disabled.key))
- return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
+ return lapic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
return MSR_IA32_APICBASE_ENABLE;
}
extern struct static_key_false_deferred apic_sw_disabled;
-static inline bool kvm_apic_sw_enabled(struct kvm_lapic *apic)
+static inline bool kvm_apic_sw_enabled(struct kvm_lapic *lapic)
{
if (static_branch_unlikely(&apic_sw_disabled.key))
- return apic->sw_enabled;
+ return lapic->sw_enabled;
return true;
}
@@ -213,9 +213,9 @@ static inline int kvm_lapic_enabled(struct kvm_vcpu *vcpu)
return kvm_apic_present(vcpu) && kvm_apic_sw_enabled(vcpu->arch.apic);
}
-static inline int apic_x2apic_mode(struct kvm_lapic *apic)
+static inline int apic_x2apic_mode(struct kvm_lapic *lapic)
{
- return apic->vcpu->arch.apic_base & X2APIC_ENABLE;
+ return lapic->vcpu->arch.apic_base & X2APIC_ENABLE;
}
static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu)
@@ -262,9 +262,9 @@ static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
}
-static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
+static inline u8 kvm_xapic_id(struct kvm_lapic *lapic)
{
- return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
+ return kvm_lapic_get_reg(lapic, APIC_ID) >> 24;
}
#endif
--
2.35.1
On Fri, Jul 29, 2022, Vincent Mailhol wrote:
> arch/x86/include/asm/apic.h declares a global variable named `apic'.
>
> Many function arguments from arch/x86/kvm/lapic.h also uses the same
> name and thus shadow the global declaration. For each case of
> shadowing, rename the function argument from `apic' to `lapic'.
>
> This patch silences below -Wshadow warnings:
This is just the tip of the iceberg, nearly every KVM x86 .c file has at least one
"apic" variable. arch/x86/kvm/lapic.c alone has nearly 100. If this were the very
last step before a kernel-wide (or even KVM-wide) enabling of -Wshadow then maybe
it would be worth doing, but as it stands IMO it's unnecesary churn.
What I would really love is to not have the global (and exported!) "apic", but
properly solving that, i.e. not just a rename, would require a significant rework.
On Sat. 30 Jul. 2022 at 02:48, Sean Christopherson <[email protected]> wrote:
> On Fri, Jul 29, 2022, Vincent Mailhol wrote:
> > arch/x86/include/asm/apic.h declares a global variable named `apic'.
> >
> > Many function arguments from arch/x86/kvm/lapic.h also uses the same
> > name and thus shadow the global declaration. For each case of
> > shadowing, rename the function argument from `apic' to `lapic'.
> >
> > This patch silences below -Wshadow warnings:
>
> This is just the tip of the iceberg, nearly every KVM x86 .c file has at least one
> "apic" variable. arch/x86/kvm/lapic.c alone has nearly 100. If this were the very
> last step before a kernel-wide (or even KVM-wide) enabling of -Wshadow then maybe
> it would be worth doing, but as it stands IMO it's unnecesary churn.
I would say the opposite: in terms of *volume*, warnings from apic.c
would be the tip of the iceberg and apic.h is the submerged part.
When the warning occurs in a header from the include directory, it
will spam some random files which include such header. This is
annoying when trying to triage W=2 warnings because you get totally
unrelated warning (and W=2 has some useful flags such as
-Wmaybe-uninitialized so there are some insensitive to check it).
My intent is only to silence the headers. I do not really care about
the -Wshadow on *.c files because it is local.
> What I would really love is to not have the global (and exported!) "apic", but
> properly solving that, i.e. not just a rename, would require a significant rework.
I double agree. I would also like to rename the global "apic" but I do
not think this is easily feasible.
Yours sincerely,
Vincent Mailhol
On Sat, Jul 30, 2022, Vincent MAILHOL wrote:
> On Sat. 30 Jul. 2022 at 02:48, Sean Christopherson <[email protected]> wrote:
> > On Fri, Jul 29, 2022, Vincent Mailhol wrote:
> > > arch/x86/include/asm/apic.h declares a global variable named `apic'.
> > >
> > > Many function arguments from arch/x86/kvm/lapic.h also uses the same
> > > name and thus shadow the global declaration. For each case of
> > > shadowing, rename the function argument from `apic' to `lapic'.
> > >
> > > This patch silences below -Wshadow warnings:
> >
> > This is just the tip of the iceberg, nearly every KVM x86 .c file has at least one
> > "apic" variable. arch/x86/kvm/lapic.c alone has nearly 100. If this were the very
> > last step before a kernel-wide (or even KVM-wide) enabling of -Wshadow then maybe
> > it would be worth doing, but as it stands IMO it's unnecesary churn.
>
> I would say the opposite: in terms of *volume*, warnings from apic.c
> would be the tip of the iceberg and apic.h is the submerged part.
>
> When the warning occurs in a header from the include directory, it
> will spam some random files which include such header. This is
> annoying when trying to triage W=2 warnings because you get totally
> unrelated warning (and W=2 has some useful flags such as
> -Wmaybe-uninitialized so there are some insensitive to check it).
>
> My intent is only to silence the headers. I do not really care about
> the -Wshadow on *.c files because it is local.
But lapic.h is a KVM-internal header, if you only care about reducing the number
of warnings and not actually "fixing" KVM, then why not suppress or filter the
warnings when building KVM?