Re: [PATCH] x86/paravirt: Get rid of patch_site label

Re: [PATCH] x86/paravirt: Get rid of patch_site label

[Borislav Petkov]

On Fri, Sep 07, 2018 at 12:49:17PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> 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 that label can simply be removed by directly calling
> paravirt_patch_insns() there.

Whoops, no, it is still being used. Lemme see if I can remove all the
labels in that function.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Borislav Petkov]

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@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@suse.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: x86@kernel.org
Cc: virtualization@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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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@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@suse.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: x86@kernel.org
> Cc: virtualization@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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] x86/paravirt: Cleanup native_patch()

[Borislav Petkov]

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@suse.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: x86@kernel.org
Cc: virtualization@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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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@suse.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: x86@kernel.org
> Cc: virtualization@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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Borislav Petkov]

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.

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

[Juergen Gross]

On 10/09/18 09:01, Borislav Petkov wrote:
> 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.
> 

Really? All of the remaining coding would fit easily on one screen. No
hidden breaks anywhere and no deep nesting involved. And the ifdefs
hardly makes it more difficult to understand, as they are not even
nested.


Juergen

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

[Borislav Petkov]

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. Also, simplify function
flow for better readability.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: x86@kernel.org
Cc: virtualization@lists.linux-foundation.org
---

Here it is, rebased ontop of rc3+tip/master.


 arch/x86/kernel/paravirt_patch_32.c | 44 ++++++++++-----------------
 arch/x86/kernel/paravirt_patch_64.c | 46 +++++++++++------------------
 2 files changed, 33 insertions(+), 57 deletions(-)

diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c
index d460cbcabcfe..6368c22fa1fa 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,24 @@ 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);
+		break;
 
-		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);
+		break;
 #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;
+	return paravirt_patch_default(type, ibuf, addr, len);
 }
diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 5ad5bcda9dc6..7ca9cb726f4d 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,24 @@ 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);
+		break;
 
-		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);
+		break;
 #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;
+	return paravirt_patch_default(type, ibuf, addr, len);
 }
-- 
2.17.0.582.gccdcbd54c

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Juergen Gross]

On 11/09/18 11:15, 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. Also, simplify function
> flow for better readability.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: x86@kernel.org
> Cc: virtualization@lists.linux-foundation.org

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization