* [PATCH] arch: x86: kernel: fixed unused label issue
@ 2016-12-14 11:15 Piotr Gregor
2016-12-14 17:06 ` Peter Zijlstra
0 siblings, 1 reply; 4+ messages in thread
From: Piotr Gregor @ 2016-12-14 11:15 UTC (permalink / raw)
To: tglx; +Cc: mingo, hpa, x86, virtualization, linux-kernel, jeremy, akataria,
rusty
The patch_default label is only used from within
case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
and
case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true.
Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS
is not defined and label should be removed in that case.
Moving #endif directive just after that label fixes the issue.
In addition,there are three errors reported by checkpatch script
on this file. This commit fixes two of them. The last one is
ERROR: Macros with multiple statements should be enclosed
in a do - while loop
which probably is a false alarm here as PATCH_SITE macro defines
a case in switch local to native_patch function not meant to be used
in other places.
Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
---
arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
1 file changed, 34 insertions(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index f4fcf26..7ce2848 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -44,43 +44,44 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
const unsigned char *start, *end;
unsigned ret;
-#define PATCH_SITE(ops, x) \
- case PARAVIRT_PATCH(ops.x): \
- start = start_##ops##_##x; \
- end = end_##ops##_##x; \
- goto patch_site
- switch(type) {
- PATCH_SITE(pv_irq_ops, restore_fl);
- PATCH_SITE(pv_irq_ops, save_fl);
- PATCH_SITE(pv_irq_ops, irq_enable);
- PATCH_SITE(pv_irq_ops, irq_disable);
- PATCH_SITE(pv_cpu_ops, usergs_sysret64);
- PATCH_SITE(pv_cpu_ops, swapgs);
- PATCH_SITE(pv_mmu_ops, read_cr2);
- PATCH_SITE(pv_mmu_ops, read_cr3);
- PATCH_SITE(pv_mmu_ops, write_cr3);
- PATCH_SITE(pv_mmu_ops, flush_tlb_single);
- PATCH_SITE(pv_cpu_ops, wbinvd);
+#define PATCH_SITE(ops, x) \
+ case PARAVIRT_PATCH(ops.x): \
+ start = start_##ops##_##x; \
+ end = end_##ops##_##x; \
+ goto patch_site
+
+ switch (type) {
+ PATCH_SITE(pv_irq_ops, restore_fl);
+ PATCH_SITE(pv_irq_ops, save_fl);
+ PATCH_SITE(pv_irq_ops, irq_enable);
+ PATCH_SITE(pv_irq_ops, irq_disable);
+ PATCH_SITE(pv_cpu_ops, usergs_sysret64);
+ PATCH_SITE(pv_cpu_ops, swapgs);
+ PATCH_SITE(pv_mmu_ops, read_cr2);
+ PATCH_SITE(pv_mmu_ops, read_cr3);
+ PATCH_SITE(pv_mmu_ops, write_cr3);
+ PATCH_SITE(pv_mmu_ops, flush_tlb_single);
+ PATCH_SITE(pv_cpu_ops, wbinvd);
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
- case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
- if (pv_is_native_spin_unlock()) {
- start = start_pv_lock_ops_queued_spin_unlock;
- end = end_pv_lock_ops_queued_spin_unlock;
- goto patch_site;
- }
- goto patch_default;
+ case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
+ if (pv_is_native_spin_unlock()) {
+ start = start_pv_lock_ops_queued_spin_unlock;
+ end = end_pv_lock_ops_queued_spin_unlock;
+ goto patch_site;
+ }
+ goto patch_default;
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
-#endif
+ case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
+ if (pv_is_native_vcpu_is_preempted()) {
+ start = start_pv_lock_ops_vcpu_is_preempted;
+ end = end_pv_lock_ops_vcpu_is_preempted;
+ goto patch_site;
+ }
+ goto patch_default;
- default:
patch_default:
+#endif
+ default:
ret = paravirt_patch_default(type, clobbers, ibuf, addr, len);
break;
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] arch: x86: kernel: fixed unused label issue
2016-12-14 11:15 [PATCH] arch: x86: kernel: fixed unused label issue Piotr Gregor
@ 2016-12-14 17:06 ` Peter Zijlstra
0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2016-12-14 17:06 UTC (permalink / raw)
To: Piotr Gregor
Cc: jeremy, x86, linux-kernel, virtualization, mingo, hpa, tglx,
akataria
On Wed, Dec 14, 2016 at 11:15:13AM +0000, Piotr Gregor wrote:
> The patch_default label is only used from within
> case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
> and
> case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
> i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true.
> Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS
> is not defined and label should be removed in that case.
> Moving #endif directive just after that label fixes the issue.
>
> In addition,there are three errors reported by checkpatch script
> on this file. This commit fixes two of them. The last one is
> ERROR: Macros with multiple statements should be enclosed
> in a do - while loop
> which probably is a false alarm here as PATCH_SITE macro defines
> a case in switch local to native_patch function not meant to be used
> in other places.
>
> Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
> ---
> arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
> 1 file changed, 34 insertions(+), 33 deletions(-)
*urgh*
So you forgot to look up the patch that caused it, which made you fail
to Cc the author of the patch that caused it (me).
That also made you miss the fact that you're only fixing half the
problem, namely you failed to touch arch/x86/kernel/paravirt_patch_32.c
which has identical code in.
Thirdly you used checkpatch and moved muck about creating a trainwreck
of a patch.
Anyway, you can use __maybe_unused on a label, I'm not sure which I
prefer, this way you jump into the previous case block and use
fall-through, while with the variant I did you jump into the right case.
Its a shame you cannot jump to the default label itself.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] arch: x86: kernel: fixed unused label issue
@ 2016-12-14 17:06 ` Peter Zijlstra
0 siblings, 0 replies; 4+ messages in thread
From: Peter Zijlstra @ 2016-12-14 17:06 UTC (permalink / raw)
To: Piotr Gregor
Cc: tglx, mingo, hpa, x86, virtualization, linux-kernel, jeremy,
akataria, rusty
On Wed, Dec 14, 2016 at 11:15:13AM +0000, Piotr Gregor wrote:
> The patch_default label is only used from within
> case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
> and
> case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
> i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true.
> Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS
> is not defined and label should be removed in that case.
> Moving #endif directive just after that label fixes the issue.
>
> In addition,there are three errors reported by checkpatch script
> on this file. This commit fixes two of them. The last one is
> ERROR: Macros with multiple statements should be enclosed
> in a do - while loop
> which probably is a false alarm here as PATCH_SITE macro defines
> a case in switch local to native_patch function not meant to be used
> in other places.
>
> Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
> ---
> arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
> 1 file changed, 34 insertions(+), 33 deletions(-)
*urgh*
So you forgot to look up the patch that caused it, which made you fail
to Cc the author of the patch that caused it (me).
That also made you miss the fact that you're only fixing half the
problem, namely you failed to touch arch/x86/kernel/paravirt_patch_32.c
which has identical code in.
Thirdly you used checkpatch and moved muck about creating a trainwreck
of a patch.
Anyway, you can use __maybe_unused on a label, I'm not sure which I
prefer, this way you jump into the previous case block and use
fall-through, while with the variant I did you jump into the right case.
Its a shame you cannot jump to the default label itself.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] arch: x86: kernel: fixed unused label issue
@ 2016-12-14 11:15 Piotr Gregor
0 siblings, 0 replies; 4+ messages in thread
From: Piotr Gregor @ 2016-12-14 11:15 UTC (permalink / raw)
To: tglx; +Cc: jeremy, x86, linux-kernel, virtualization, mingo, hpa, akataria
The patch_default label is only used from within
case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock)
and
case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted)
i.e. when #if defined(CONFIG_PARAVIRT_SPINLOCKS) is true.
Therefore no code jumps to this label in case CONFIG_PARAVIRT_SPINLOCKS
is not defined and label should be removed in that case.
Moving #endif directive just after that label fixes the issue.
In addition,there are three errors reported by checkpatch script
on this file. This commit fixes two of them. The last one is
ERROR: Macros with multiple statements should be enclosed
in a do - while loop
which probably is a false alarm here as PATCH_SITE macro defines
a case in switch local to native_patch function not meant to be used
in other places.
Signed-off-by: Piotr Gregor <piotrgregor@rsyncme.org>
---
arch/x86/kernel/paravirt_patch_64.c | 67 +++++++++++++++++++------------------
1 file changed, 34 insertions(+), 33 deletions(-)
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index f4fcf26..7ce2848 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -44,43 +44,44 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
const unsigned char *start, *end;
unsigned ret;
-#define PATCH_SITE(ops, x) \
- case PARAVIRT_PATCH(ops.x): \
- start = start_##ops##_##x; \
- end = end_##ops##_##x; \
- goto patch_site
- switch(type) {
- PATCH_SITE(pv_irq_ops, restore_fl);
- PATCH_SITE(pv_irq_ops, save_fl);
- PATCH_SITE(pv_irq_ops, irq_enable);
- PATCH_SITE(pv_irq_ops, irq_disable);
- PATCH_SITE(pv_cpu_ops, usergs_sysret64);
- PATCH_SITE(pv_cpu_ops, swapgs);
- PATCH_SITE(pv_mmu_ops, read_cr2);
- PATCH_SITE(pv_mmu_ops, read_cr3);
- PATCH_SITE(pv_mmu_ops, write_cr3);
- PATCH_SITE(pv_mmu_ops, flush_tlb_single);
- PATCH_SITE(pv_cpu_ops, wbinvd);
+#define PATCH_SITE(ops, x) \
+ case PARAVIRT_PATCH(ops.x): \
+ start = start_##ops##_##x; \
+ end = end_##ops##_##x; \
+ goto patch_site
+
+ switch (type) {
+ PATCH_SITE(pv_irq_ops, restore_fl);
+ PATCH_SITE(pv_irq_ops, save_fl);
+ PATCH_SITE(pv_irq_ops, irq_enable);
+ PATCH_SITE(pv_irq_ops, irq_disable);
+ PATCH_SITE(pv_cpu_ops, usergs_sysret64);
+ PATCH_SITE(pv_cpu_ops, swapgs);
+ PATCH_SITE(pv_mmu_ops, read_cr2);
+ PATCH_SITE(pv_mmu_ops, read_cr3);
+ PATCH_SITE(pv_mmu_ops, write_cr3);
+ PATCH_SITE(pv_mmu_ops, flush_tlb_single);
+ PATCH_SITE(pv_cpu_ops, wbinvd);
#if defined(CONFIG_PARAVIRT_SPINLOCKS)
- case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
- if (pv_is_native_spin_unlock()) {
- start = start_pv_lock_ops_queued_spin_unlock;
- end = end_pv_lock_ops_queued_spin_unlock;
- goto patch_site;
- }
- goto patch_default;
+ case PARAVIRT_PATCH(pv_lock_ops.queued_spin_unlock):
+ if (pv_is_native_spin_unlock()) {
+ start = start_pv_lock_ops_queued_spin_unlock;
+ end = end_pv_lock_ops_queued_spin_unlock;
+ goto patch_site;
+ }
+ goto patch_default;
- case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
- if (pv_is_native_vcpu_is_preempted()) {
- start = start_pv_lock_ops_vcpu_is_preempted;
- end = end_pv_lock_ops_vcpu_is_preempted;
- goto patch_site;
- }
- goto patch_default;
-#endif
+ case PARAVIRT_PATCH(pv_lock_ops.vcpu_is_preempted):
+ if (pv_is_native_vcpu_is_preempted()) {
+ start = start_pv_lock_ops_vcpu_is_preempted;
+ end = end_pv_lock_ops_vcpu_is_preempted;
+ goto patch_site;
+ }
+ goto patch_default;
- default:
patch_default:
+#endif
+ default:
ret = paravirt_patch_default(type, clobbers, ibuf, addr, len);
break;
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-14 17:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-14 11:15 [PATCH] arch: x86: kernel: fixed unused label issue Piotr Gregor
2016-12-14 17:06 ` Peter Zijlstra
2016-12-14 17:06 ` Peter Zijlstra
-- strict thread matches above, loose matches on Subject: below --
2016-12-14 11:15 Piotr Gregor
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.