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@xxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> ---
>
> 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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
