Re: [PATCH] x86/paravirt: Cleanup native_patch()

[Juergen Gross]

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