thr3ads.net - Linux Virtualization - [PATCH] x86/paravirt: Cleanup native

If this information is useful, please help other people find it:
Share via:

Borislav Petkov

2018-Sep-08 15:28 UTC

[PATCH] x86/paravirt: Cleanup native_patch()

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 at suse.de>
Cc: Juergen Gross <jgross at suse.com>
Cc: x86 at kernel.org
Cc: virtualization at lists.linux-foundation.org
---

It looks even more cleaner now. :)

 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);
 
-		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);
 #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);
 	}
 #undef PATCH_SITE
-	return ret;
+	return 0;
 }
diff --git a/arch/x86/kernel/paravirt_patch_64.c
b/arch/x86/kernel/paravirt_patch_64.c
index 5ad5bcda9dc6..3549f5f9d685 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -42,15 +42,11 @@ 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
-	switch(type) {
+	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, restore_fl);
 		PATCH_SITE(irq, save_fl);
@@ -64,32 +60,27 @@ 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);
 
-		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);
 #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);
+		return paravirt_patch_default(type, ibuf, addr, len);
 		break;
 	}
 #undef PATCH_SITE
-	return ret;
+	return 0;
 }
-- 
2.17.0.582.gccdcbd54c


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Juergen Gross

2018-Sep-10 06:54 UTC

head link

[PATCH] x86/paravirt: Cleanup native_patch()

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 at suse.de>
> Cc: Juergen Gross <jgross at suse.com>
> Cc: x86 at kernel.org
> Cc: virtualization at 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

Borislav Petkov

2018-Sep-10 07:01 UTC

head link

[PATCH] x86/paravirt: Cleanup native_patch()

On Mon, Sep 10, 2018 at 08:54:12AM +0200, Juergen Gross
wrote:> > +	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 ...
Because I think that exiting right then and there is much easier to
follow than adding all those breaks and wading through ifdeffery
to realize that this is the default path and we'll end up calling
paravirt_patch_default() in the end.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Possibly Parallel Threads

Search for more possibly parallel threads

Linux Virtualization - Sep 2018 - [PATCH] x86/paravirt: Cleanup native_patch()

[PATCH] x86/paravirt: Cleanup native_patch()

[PATCH] x86/paravirt: Cleanup native_patch()

[PATCH] x86/paravirt: Cleanup native_patch()

Possibly Parallel Threads