* [PATCH] x86/paravirt: Get rid of patch_site label @ 2018-09-07 10:49 Borislav Petkov 2018-09-07 10:51 ` Borislav Petkov 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2018-09-07 10:49 UTC (permalink / raw) To: LKML; +Cc: Juergen Gross, x86, virtualization 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. 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_64.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c index 5ad5bcda9dc6..f09d264cbd2d 100644 --- a/arch/x86/kernel/paravirt_patch_64.c +++ b/arch/x86/kernel/paravirt_patch_64.c @@ -76,7 +76,8 @@ unsigned native_patch(u8 type, void *ibuf, unsigned long addr, unsigned len) 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; #endif @@ -86,7 +87,6 @@ patch_default: __maybe_unused ret = paravirt_patch_default(type, ibuf, addr, len); break; -patch_site: ret = paravirt_patch_insns(ibuf, len, start, end); break; } -- 2.17.0.582.gccdcbd54c ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/paravirt: Get rid of patch_site label 2018-09-07 10:49 [PATCH] x86/paravirt: Get rid of patch_site label Borislav Petkov @ 2018-09-07 10:51 ` Borislav Petkov 2018-09-07 20:10 ` [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels Borislav Petkov 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2018-09-07 10:51 UTC (permalink / raw) To: LKML; +Cc: Juergen Gross, x86, virtualization 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels 2018-09-07 10:51 ` Borislav Petkov @ 2018-09-07 20:10 ` Borislav Petkov 2018-09-08 5:45 ` Juergen Gross 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2018-09-07 20:10 UTC (permalink / raw) To: LKML; +Cc: Juergen Gross, x86, virtualization 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. ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels 2018-09-07 20:10 ` [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels Borislav Petkov @ 2018-09-08 5:45 ` Juergen Gross 2018-09-08 15:28 ` [PATCH] x86/paravirt: Cleanup native_patch() Borislav Petkov 0 siblings, 1 reply; 11+ messages in thread From: Juergen Gross @ 2018-09-08 5:45 UTC (permalink / raw) To: Borislav Petkov, LKML; +Cc: x86, virtualization 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] x86/paravirt: Cleanup native_patch() 2018-09-08 5:45 ` Juergen Gross @ 2018-09-08 15:28 ` Borislav Petkov 2018-09-10 6:54 ` Juergen Gross 0 siblings, 1 reply; 11+ messages in thread From: Borislav Petkov @ 2018-09-08 15:28 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. 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. ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86/paravirt: Cleanup native_patch() 2018-09-08 15:28 ` [PATCH] x86/paravirt: Cleanup native_patch() Borislav Petkov @ 2018-09-10 6:54 ` Juergen Gross 2018-09-10 7:01 ` Borislav Petkov 0 siblings, 1 reply; 11+ 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] 11+ 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 8:33 ` Juergen Gross 0 siblings, 1 reply; 11+ 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] 11+ 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-11 9:15 ` [PATCH v2] " Borislav Petkov 0 siblings, 1 reply; 11+ 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] 11+ 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 2018-09-11 15:34 ` [tip:x86/paravirt] x86/paravirt: Clean up native_patch() tip-bot for Borislav Petkov 0 siblings, 2 replies; 11+ 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] 11+ 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 15:34 ` [tip:x86/paravirt] x86/paravirt: Clean up native_patch() tip-bot for Borislav Petkov 1 sibling, 0 replies; 11+ 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] 11+ 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 15:34 ` tip-bot for Borislav Petkov 1 sibling, 0 replies; 11+ 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] 11+ messages in thread
end of thread, other threads:[~2018-09-11 15:35 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-07 10:49 [PATCH] x86/paravirt: Get rid of patch_site label Borislav Petkov 2018-09-07 10:51 ` Borislav Petkov 2018-09-07 20:10 ` [PATCH v2] x86/paravirt: Get rid of patch_site and patch_default labels Borislav Petkov 2018-09-08 5:45 ` Juergen Gross 2018-09-08 15:28 ` [PATCH] x86/paravirt: Cleanup native_patch() Borislav Petkov 2018-09-10 6:54 ` Juergen Gross 2018-09-10 7:01 ` Borislav Petkov 2018-09-10 8:33 ` Juergen Gross 2018-09-11 9:15 ` [PATCH v2] " Borislav Petkov 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).