Messages in this thread | | | Subject | Re: [PATCH] x86/paravirt: Cleanup native_patch() | From | Juergen Gross <> | Date | Mon, 10 Sep 2018 08:54:12 +0200 |
| |
On 08/09/18 17:28, Borislav Petkov wrote: > When CONFIG_PARAVIRT_SPINLOCKS=n, it fires > > arch/x86/kernel/paravirt_patch_64.c: In function ‘native_patch’: > arch/x86/kernel/paravirt_patch_64.c:89:1: warning: label ‘patch_site’ defined but not used [-Wunused-label] > patch_site: > > but those labels can simply be removed by directly calling the > respective functions there. > > Get rid of local variables too, while at it. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Juergen Gross <jgross@suse.com> > Cc: x86@kernel.org > Cc: virtualization@lists.linux-foundation.org > --- > > It looks even more cleaner now. :)
And there is still some more clean up possible:
> > arch/x86/kernel/paravirt_patch_32.c | 48 +++++++++++----------------- > arch/x86/kernel/paravirt_patch_64.c | 49 ++++++++++++----------------- > 2 files changed, 39 insertions(+), 58 deletions(-) > > diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c > index d460cbcabcfe..0865323c2716 100644 > --- a/arch/x86/kernel/paravirt_patch_32.c > +++ b/arch/x86/kernel/paravirt_patch_32.c > @@ -34,14 +34,10 @@ extern bool pv_is_native_vcpu_is_preempted(void); > > unsigned native_patch(u8 type, void *ibuf, unsigned long addr, unsigned len) > { > - 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 > + case PARAVIRT_PATCH(ops.x): \ > + return paravirt_patch_insns(ibuf, len, start_##ops##_##x, end_##ops##_##x) > + > switch (type) { > #ifdef CONFIG_PARAVIRT_XXL > PATCH_SITE(irq, irq_disable); > @@ -54,32 +50,26 @@ unsigned native_patch(u8 type, void *ibuf, unsigned long addr, unsigned len) > PATCH_SITE(mmu, write_cr3); > #endif > #if defined(CONFIG_PARAVIRT_SPINLOCKS) > - case PARAVIRT_PATCH(lock.queued_spin_unlock): > - if (pv_is_native_spin_unlock()) { > - start = start_lock_queued_spin_unlock; > - end = end_lock_queued_spin_unlock; > - goto patch_site; > - } > - goto patch_default; > + case PARAVIRT_PATCH(lock.queued_spin_unlock): > + if (pv_is_native_spin_unlock()) > + return paravirt_patch_insns(ibuf, len, > + start_lock_queued_spin_unlock, > + end_lock_queued_spin_unlock); > + else > + return paravirt_patch_default(type, ibuf, addr, len);
Why not replace the else clause by a "break;" and ...
> > - case PARAVIRT_PATCH(lock.vcpu_is_preempted): > - if (pv_is_native_vcpu_is_preempted()) { > - start = start_lock_vcpu_is_preempted; > - end = end_lock_vcpu_is_preempted; > - goto patch_site; > - } > - goto patch_default; > + case PARAVIRT_PATCH(lock.vcpu_is_preempted): > + if (pv_is_native_vcpu_is_preempted()) > + return paravirt_patch_insns(ibuf, len, > + start_lock_vcpu_is_preempted, > + end_lock_vcpu_is_preempted); > + else > + return paravirt_patch_default(type, ibuf, addr, len);
... this as well and ...
> #endif > > default: > -patch_default: __maybe_unused > - ret = paravirt_patch_default(type, ibuf, addr, len); > - break; > - > -patch_site: > - ret = paravirt_patch_insns(ibuf, len, start, end); > - break; > + return paravirt_patch_default(type, ibuf, addr, len);
... remove this return and ...
> } > #undef PATCH_SITE > - return ret; > + return 0;
Replace this by return paravirt_patch_default(type, ibuf, addr, len);
Same for paravirt_patch_64.c, of course.
Juergen
| |