Re: [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels

[Juergen Gross]

On 07/09/18 22:10, Borislav Petkov wrote:
> On Fri, Sep 07, 2018 at 12:51:12PM +0200, Borislav Petkov wrote:
>> Whoops, no, it is still being used. Lemme see if I can remove all the
>> labels in that function.
> 
> Ok, I think I've gotten rid of them all, in both 32- and 64-bit
> versions. It boots here and a bunch of all*config builds pass.
> 
> ---
> From: Borislav Petkov <bp@xxxxxxx>
> Date: Fri, 7 Sep 2018 12:47:10 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 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.
> 
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: x86@xxxxxxxxxx
> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> ---
>  arch/x86/kernel/paravirt_patch_32.c | 21 ++++++++++-----------
>  arch/x86/kernel/paravirt_patch_64.c | 20 +++++++++-----------
>  2 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
> index d460cbcabcfe..af57f0d0789f 100644
> --- a/arch/x86/kernel/paravirt_patch_32.c
> +++ b/arch/x86/kernel/paravirt_patch_32.c
> @@ -34,14 +34,16 @@ 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;
> +	const unsigned char *start __maybe_unused, *end __maybe_unused;
>  	unsigned ret;
>  
>  #define PATCH_SITE(ops, x)					\
>  		case PARAVIRT_PATCH(ops.x):			\
>  			start = start_##ops##_##x;		\
>  			end = end_##ops##_##x;			\
> -			goto patch_site
> +								\
> +			return paravirt_patch_insns(ibuf, len, start, end);
> +
>  	switch (type) {
>  #ifdef CONFIG_PARAVIRT_XXL
>  		PATCH_SITE(irq, irq_disable);
> @@ -58,27 +60,24 @@ unsigned native_patch(u8 type, void *ibuf, unsigned long addr, unsigned len)
>  			if (pv_is_native_spin_unlock()) {
>  				start = start_lock_queued_spin_unlock;
>  				end   = end_lock_queued_spin_unlock;
> -				goto patch_site;
> +
> +				return paravirt_patch_insns(ibuf, len, start, end);
>  			}
> -			goto patch_default;
> +			return paravirt_patch_default(type, ibuf, addr, len);
>  
>  		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;
> +
> +				return paravirt_patch_insns(ibuf, len, start, end);
>  			}
> -			goto patch_default;
> +			return paravirt_patch_default(type, ibuf, addr, len);
>  #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;
>  	}
>  #undef PATCH_SITE
>  	return ret;
> diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
> index 5ad5bcda9dc6..2db6c615932f 100644
> --- a/arch/x86/kernel/paravirt_patch_64.c
> +++ b/arch/x86/kernel/paravirt_patch_64.c
> @@ -42,14 +42,15 @@ 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;
> +	const unsigned char *start __maybe_unused, *end __maybe_unused;

Drop start and end and ...

>  	unsigned ret;
>  
>  #define PATCH_SITE(ops, x)					\
>  		case PARAVIRT_PATCH(ops.x):			\
>  			start = start_##ops##_##x;		\
>  			end = end_##ops##_##x;			\
> -			goto patch_site
> +								\
> +			return paravirt_patch_insns(ibuf, len, start, end);

... do: return paravirt_patch_insns(ibuf, len, start_##ops##_##x,
end_##ops##_##x)

and please omit the semicolon after the last instruction in the macro.

(below the same)

>  	switch(type) {
>  #ifdef CONFIG_PARAVIRT_XXL
>  		PATCH_SITE(irq, restore_fl);
> @@ -68,27 +69,24 @@ unsigned native_patch(u8 type, void *ibuf, unsigned long addr, unsigned len)
>  			if (pv_is_native_spin_unlock()) {
>  				start = start_lock_queued_spin_unlock;
>  				end   = end_lock_queued_spin_unlock;
> -				goto patch_site;
> +
> +				return paravirt_patch_insns(ibuf, len, start, end);
>  			}
> -			goto patch_default;
> +			return paravirt_patch_default(type, ibuf, addr, len);
>  
>  		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;
> +
> +				return paravirt_patch_insns(ibuf, len, start, end);
>  			}
> -			goto patch_default;
> +			return paravirt_patch_default(type, ibuf, addr, len);
>  #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;
>  	}
>  #undef PATCH_SITE
>  	return ret;
> 

Juergen