* [PATCH] x86/paravirt: Cleanup native_patch()
@ 2018-09-08 15:28 ` Borislav Petkov
0 siblings, 0 replies; 20+ messages in thread
From: Borislav Petkov @ 2018-09-08 15:28 UTC (permalink / raw)
To: Juergen Gross; +Cc: x86, LKML, virtualization
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-08 15:28 ` Borislav Petkov
(?)
@ 2018-09-10 6:54 ` Juergen Gross
2018-09-10 7:01 ` Borislav Petkov
2018-09-10 7:01 ` Borislav Petkov
-1 siblings, 2 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-10 6:54 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML, x86, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-10 6:54 ` Juergen Gross
@ 2018-09-10 7:01 ` Borislav Petkov
2018-09-10 7:01 ` Borislav Petkov
1 sibling, 0 replies; 20+ messages in thread
From: Borislav Petkov @ 2018-09-10 7:01 UTC (permalink / raw)
To: Juergen Gross; +Cc: x86, LKML, virtualization
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-10 6:54 ` Juergen Gross
2018-09-10 7:01 ` Borislav Petkov
@ 2018-09-10 7:01 ` Borislav Petkov
2018-09-10 8:33 ` Juergen Gross
2018-09-10 8:33 ` Juergen Gross
1 sibling, 2 replies; 20+ messages in thread
From: Borislav Petkov @ 2018-09-10 7:01 UTC (permalink / raw)
To: Juergen Gross; +Cc: LKML, x86, virtualization
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-10 7:01 ` Borislav Petkov
@ 2018-09-10 8:33 ` Juergen Gross
2018-09-10 8:33 ` Juergen Gross
1 sibling, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-10 8:33 UTC (permalink / raw)
To: Borislav Petkov; +Cc: x86, LKML, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-10 7:01 ` Borislav Petkov
2018-09-10 8:33 ` Juergen Gross
@ 2018-09-10 8:33 ` Juergen Gross
2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov
2018-09-11 9:15 ` [PATCH v2] x86/paravirt: Cleanup native_patch() Borislav Petkov
1 sibling, 2 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-10 8:33 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML, x86, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] x86/paravirt: Cleanup native_patch()
2018-09-10 8:33 ` Juergen Gross
@ 2018-09-11 9:15 ` Borislav Petkov
2018-09-11 9:44 ` Juergen Gross
` (2 more replies)
2018-09-11 9:15 ` [PATCH v2] x86/paravirt: Cleanup native_patch() Borislav Petkov
1 sibling, 3 replies; 20+ messages in thread
From: Borislav Petkov @ 2018-09-11 9:15 UTC (permalink / raw)
To: Juergen Gross; +Cc: LKML, x86, virtualization
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.
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] x86/paravirt: Cleanup native_patch()
2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov
@ 2018-09-11 9:44 ` Juergen Gross
2018-09-11 9:44 ` Juergen Gross
2018-09-11 15:34 ` [tip:x86/paravirt] x86/paravirt: Clean up native_patch() tip-bot for Borislav Petkov
2 siblings, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-11 9:44 UTC (permalink / raw)
To: Borislav Petkov; +Cc: x86, LKML, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] x86/paravirt: Cleanup native_patch()
2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov
2018-09-11 9:44 ` Juergen Gross
@ 2018-09-11 9:44 ` Juergen Gross
2018-09-11 15:34 ` [tip:x86/paravirt] x86/paravirt: Clean up native_patch() tip-bot for Borislav Petkov
2 siblings, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-11 9:44 UTC (permalink / raw)
To: Borislav Petkov; +Cc: LKML, x86, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [tip:x86/paravirt] x86/paravirt: Clean up native_patch()
2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov
2018-09-11 9:44 ` Juergen Gross
2018-09-11 9:44 ` Juergen Gross
@ 2018-09-11 15:34 ` tip-bot for Borislav Petkov
2 siblings, 0 replies; 20+ messages in thread
From: tip-bot for Borislav Petkov @ 2018-09-11 15:34 UTC (permalink / raw)
To: linux-tip-commits
Cc: mingo, hpa, peterz, linux-kernel, torvalds, bp, bp, jgross, tglx
Commit-ID: 3637897b6c9bc2f12f38956d64724a6d0bbb56fd
Gitweb: https://git.kernel.org/tip/3637897b6c9bc2f12f38956d64724a6d0bbb56fd
Author: Borislav Petkov <bp@alien8.de>
AuthorDate: Tue, 11 Sep 2018 11:15:10 +0200
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 11 Sep 2018 12:45:14 +0200
x86/paravirt: Clean up native_patch()
When CONFIG_PARAVIRT_SPINLOCKS=n, it generates a warning:
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>
Reviewed-by: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: virtualization@lists.linux-foundation.org
Link: http://lkml.kernel.org/r/20180911091510.GA12094@zn.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
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);
}
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2] x86/paravirt: Cleanup native_patch()
2018-09-10 8:33 ` Juergen Gross
2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov
@ 2018-09-11 9:15 ` Borislav Petkov
1 sibling, 0 replies; 20+ messages in thread
From: Borislav Petkov @ 2018-09-11 9:15 UTC (permalink / raw)
To: Juergen Gross; +Cc: x86, LKML, virtualization
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch()
2018-09-08 15:28 ` Borislav Petkov
(?)
(?)
@ 2018-09-10 6:54 ` Juergen Gross
-1 siblings, 0 replies; 20+ messages in thread
From: Juergen Gross @ 2018-09-10 6:54 UTC (permalink / raw)
To: Borislav Petkov; +Cc: x86, LKML, virtualization
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
^ permalink raw reply [flat|nested] 20+ messages in thread