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-07 20:10 UTC

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

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

-- 
Regards/Gruss,
    Boris.

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

Juergen Gross

2018-Sep-08 05:45 UTC

head link

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

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

Borislav Petkov

2018-Sep-08 15:28 UTC

head link

[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.

Maybe Matching Threads

Search for more reasonably related threads

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

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

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

[PATCH] x86/paravirt: Cleanup native_patch()

Maybe Matching Threads