| Message ID | 20241119125904.2681402-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/mce: Compile do_mca() for CONFIG_PV only | expand |
On 19.11.2024 13:59, Andrew Cooper wrote: > Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's > declaration. It turns out that this is a consequence of do_mca() being > PV-only, and the declaration being compiled out in !PV builds. > > Therefore, arrange for do_mca() to be compiled out in !PV builds. This in > turn requires a number of static functions to become static inline. Which generally we advocate against. The #ifdef variant you pointed at on Matrix wasn't all that bad. Plus ... > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: consulting@bugseng.com <consulting@bugseng.com> > > Bloat-o-meter on a !PV build reports: > > add/remove: 0/6 grow/shrink: 0/0 up/down: 0/-3717 (-3717) > Function old new delta > x86_mc_mceinject 31 - -31 > do_mca.cold 117 - -117 > x86_mc_msrinject 147 - -147 > x86_mc_msrinject.cold 230 - -230 > do_mc_get_cpu_info 500 - -500 > do_mca 2692 - -2692 ... what's the effect of the addition of "inline" on a PV=y build? By using the keyword, we may end up talking the compiler into inlining of code that better wouldn't be inlined. Jan
On 19/11/2024 2:34 pm, Jan Beulich wrote: > On 19.11.2024 13:59, Andrew Cooper wrote: >> Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's >> declaration. It turns out that this is a consequence of do_mca() being >> PV-only, and the declaration being compiled out in !PV builds. >> >> Therefore, arrange for do_mca() to be compiled out in !PV builds. This in >> turn requires a number of static functions to become static inline. > Which generally we advocate against. It's `static inline` or `static __maybe_unused`, but I refer you to to the Matrix conversation on June 24th on the matter. > The #ifdef variant you pointed at on > Matrix wasn't all that bad. It worked as a test, but ifdefary like that is a maintenance nightmare. > Plus ... > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: consulting@bugseng.com <consulting@bugseng.com> >> >> Bloat-o-meter on a !PV build reports: >> >> add/remove: 0/6 grow/shrink: 0/0 up/down: 0/-3717 (-3717) >> Function old new delta >> x86_mc_mceinject 31 - -31 >> do_mca.cold 117 - -117 >> x86_mc_msrinject 147 - -147 >> x86_mc_msrinject.cold 230 - -230 >> do_mc_get_cpu_info 500 - -500 >> do_mca 2692 - -2692 > ... what's the effect of the addition of "inline" on a PV=y build? By > using the keyword, we may end up talking the compiler into inlining of > code that better wouldn't be inlined. xen.git/xen$ ../scripts/bloat-o-meter xen-syms-{before,after} add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) Function old new delta Total: Before=3901801, After=3901801, chg +0.00% xen.git/xen$ diff -u dis-{before,after} --- dis-before 2024-11-19 18:08:02.284091931 +0000 +++ dis-after 2024-11-19 18:08:24.491035756 +0000 @@ -1,5 +1,5 @@ -xen-syms-before: file format elf64-x86-64 +xen-syms-after: file format elf64-x86-64 Disassembly of section .text: xen.git/xen$ Which is not surprising because at -O2, the keyword is effectively ignored because of the various -finline-* ~Andrew
On Tue, 19 Nov 2024, Andrew Cooper wrote: > On 19/11/2024 2:34 pm, Jan Beulich wrote: > > On 19.11.2024 13:59, Andrew Cooper wrote: > >> Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's > >> declaration. It turns out that this is a consequence of do_mca() being > >> PV-only, and the declaration being compiled out in !PV builds. > >> > >> Therefore, arrange for do_mca() to be compiled out in !PV builds. This in > >> turn requires a number of static functions to become static inline. > > Which generally we advocate against. > > It's `static inline` or `static __maybe_unused`, but I refer you to to > the Matrix conversation on June 24th on the matter. > > > > The #ifdef variant you pointed at on > > Matrix wasn't all that bad. > > It worked as a test, but ifdefary like that is a maintenance nightmare. > > > Plus ... > > > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Jan Beulich <JBeulich@suse.com> > >> CC: Roger Pau Monné <roger.pau@citrix.com> > >> CC: Stefano Stabellini <sstabellini@kernel.org> > >> CC: consulting@bugseng.com <consulting@bugseng.com> > >> > >> Bloat-o-meter on a !PV build reports: > >> > >> add/remove: 0/6 grow/shrink: 0/0 up/down: 0/-3717 (-3717) > >> Function old new delta > >> x86_mc_mceinject 31 - -31 > >> do_mca.cold 117 - -117 > >> x86_mc_msrinject 147 - -147 > >> x86_mc_msrinject.cold 230 - -230 > >> do_mc_get_cpu_info 500 - -500 > >> do_mca 2692 - -2692 > > ... what's the effect of the addition of "inline" on a PV=y build? By > > using the keyword, we may end up talking the compiler into inlining of > > code that better wouldn't be inlined. > > xen.git/xen$ ../scripts/bloat-o-meter xen-syms-{before,after} > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) > Function old new delta > Total: Before=3901801, After=3901801, chg +0.00% > xen.git/xen$ diff -u dis-{before,after} > --- dis-before 2024-11-19 18:08:02.284091931 +0000 > +++ dis-after 2024-11-19 18:08:24.491035756 +0000 > @@ -1,5 +1,5 @@ > > -xen-syms-before: file format elf64-x86-64 > +xen-syms-after: file format elf64-x86-64 > > > Disassembly of section .text: > > xen.git/xen$ > > > Which is not surprising because at -O2, the keyword is effectively > ignored because of the various -finline-* Given this, and also given that this patch with the static inline looks nice, I prefer this version: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 19.11.2024 19:22, Andrew Cooper wrote: > On 19/11/2024 2:34 pm, Jan Beulich wrote: >> On 19.11.2024 13:59, Andrew Cooper wrote: >>> Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's >>> declaration. It turns out that this is a consequence of do_mca() being >>> PV-only, and the declaration being compiled out in !PV builds. >>> >>> Therefore, arrange for do_mca() to be compiled out in !PV builds. This in >>> turn requires a number of static functions to become static inline. >> Which generally we advocate against. > > It's `static inline` or `static __maybe_unused`, but I refer you to to > the Matrix conversation on June 24th on the matter. Well, and the outcome was __maybe_unused there. For consistency I'd therefore like to ask that you use __maybe_unused then here, too. As an aside, it would have helped if you had said which channel that discussion had been on. Scrolling back this far sadly doesn't work very nicely on the element.io interface (or I simply don't know how to make it go to a specific date), and I ended up trying XenDevel, then the committers channel, and only then the x86 one. >> The #ifdef variant you pointed at on >> Matrix wasn't all that bad. > > It worked as a test, but ifdefary like that is a maintenance nightmare. > >> Plus ... >> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: consulting@bugseng.com <consulting@bugseng.com> >>> >>> Bloat-o-meter on a !PV build reports: >>> >>> add/remove: 0/6 grow/shrink: 0/0 up/down: 0/-3717 (-3717) >>> Function old new delta >>> x86_mc_mceinject 31 - -31 >>> do_mca.cold 117 - -117 >>> x86_mc_msrinject 147 - -147 >>> x86_mc_msrinject.cold 230 - -230 >>> do_mc_get_cpu_info 500 - -500 >>> do_mca 2692 - -2692 >> ... what's the effect of the addition of "inline" on a PV=y build? By >> using the keyword, we may end up talking the compiler into inlining of >> code that better wouldn't be inlined. > > xen.git/xen$ ../scripts/bloat-o-meter xen-syms-{before,after} > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0) > Function old new delta > Total: Before=3901801, After=3901801, chg +0.00% > xen.git/xen$ diff -u dis-{before,after} > --- dis-before 2024-11-19 18:08:02.284091931 +0000 > +++ dis-after 2024-11-19 18:08:24.491035756 +0000 > @@ -1,5 +1,5 @@ > > -xen-syms-before: file format elf64-x86-64 > +xen-syms-after: file format elf64-x86-64 > > > Disassembly of section .text: > > xen.git/xen$ Good. Preferably with __maybe_unused Acked-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index 32c1b2756b90..2a88590525f0 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -932,7 +932,7 @@ void x86_mcinfo_dump(struct mc_info *mi) } while ( 1 ); } -static void cf_check do_mc_get_cpu_info(void *v) +static inline void cf_check do_mc_get_cpu_info(void *v) { int cpu = smp_processor_id(); int cindex, cpn; @@ -1114,7 +1114,7 @@ bool intpose_inval(unsigned int cpu_nr, uint64_t msr) (r) <= MSR_IA32_MCx_MISC(per_cpu(nr_mce_banks, cpu) - 1) && \ ((r) - MSR_IA32_MC0_CTL) % 4) /* excludes MCi_CTL */ -static bool x86_mc_msrinject_verify(struct xen_mc_msrinject *mci) +static inline bool x86_mc_msrinject_verify(struct xen_mc_msrinject *mci) { const struct cpuinfo_x86 *c = &cpu_data[mci->mcinj_cpunr]; int i, errs = 0; @@ -1192,7 +1192,7 @@ static bool x86_mc_msrinject_verify(struct xen_mc_msrinject *mci) return !errs; } -static uint64_t x86_mc_hwcr_wren(void) +static inline uint64_t x86_mc_hwcr_wren(void) { uint64_t old; @@ -1207,13 +1207,13 @@ static uint64_t x86_mc_hwcr_wren(void) return old; } -static void x86_mc_hwcr_wren_restore(uint64_t hwcr) +static inline void x86_mc_hwcr_wren_restore(uint64_t hwcr) { if ( !(hwcr & K8_HWCR_MCi_STATUS_WREN) ) wrmsrl(MSR_K8_HWCR, hwcr); } -static void cf_check x86_mc_msrinject(void *data) +static inline void cf_check x86_mc_msrinject(void *data) { struct xen_mc_msrinject *mci = data; struct mcinfo_msr *msr; @@ -1244,13 +1244,14 @@ static void cf_check x86_mc_msrinject(void *data) x86_mc_hwcr_wren_restore(hwcr); } -/*ARGSUSED*/ -static void cf_check x86_mc_mceinject(void *data) +static inline void cf_check x86_mc_mceinject(void *data) { printk("Simulating #MC on cpu %d\n", smp_processor_id()); __asm__ __volatile__("int $0x12"); } +#ifdef CONFIG_PV /* do_mca() hypercall is PV-only */ + #if BITS_PER_LONG == 64 #define ID2COOKIE(id) ((mctelem_cookie_t)(id)) @@ -1654,6 +1655,8 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) return ret; } +#endif /* CONFIG_PV */ + static int mcinfo_dumped; static int cf_check x86_mcinfo_dump_panic(mctelem_cookie_t mctc)
Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's declaration. It turns out that this is a consequence of do_mca() being PV-only, and the declaration being compiled out in !PV builds. Therefore, arrange for do_mca() to be compiled out in !PV builds. This in turn requires a number of static functions to become static inline. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: consulting@bugseng.com <consulting@bugseng.com> Bloat-o-meter on a !PV build reports: add/remove: 0/6 grow/shrink: 0/0 up/down: 0/-3717 (-3717) Function old new delta x86_mc_mceinject 31 - -31 do_mca.cold 117 - -117 x86_mc_msrinject 147 - -147 x86_mc_msrinject.cold 230 - -230 do_mc_get_cpu_info 500 - -500 do_mca 2692 - -2692 --- xen/arch/x86/cpu/mcheck/mce.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) base-commit: 3128d7248f2ad389b8e9a3e252958cbfbd1898ee prerequisite-patch-id: 46b8fc2e9df2fd6be1bbbd6b50463e0e15a8f94d prerequisite-patch-id: c122b170f57ab96fe52c37aebf1f4bb366194637 prerequisite-patch-id: 1c2d96bf17c5da0981b6c62939d3b7cc1e05933e prerequisite-patch-id: b3e43902729416e18b4fada7f529b4cb02b1815e prerequisite-patch-id: a06452180f71021893259bb3b883185f57742a31