[PATCH] KVM: x86: do not shadow apic global definition

[PATCH] KVM: x86: do not shadow apic global definition

[Vincent Mailhol]

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 <mailhol.vincent@wanadoo.fr>
---
 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

Re: [PATCH] KVM: x86: do not shadow apic global definition

[Sean Christopherson]

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.

Re: [PATCH] KVM: x86: do not shadow apic global definition

[Vincent MAILHOL]

On Sat. 30 Jul. 2022 at 02:48, Sean Christopherson <seanjc@google.com> 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

Re: [PATCH] KVM: x86: do not shadow apic global definition

[Sean Christopherson]

On Sat, Jul 30, 2022, Vincent MAILHOL wrote:
> On Sat. 30 Jul. 2022 at 02:48, Sean Christopherson <seanjc@google.com> 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?