Re: [PATCH] x86/mce: Compile do_mca() for CONFIG_PV only

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: consulting@xxxxxxxxxxx <consulting@xxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>