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

Andrew Cooper posted 1 patch 2 weeks, 3 days ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20241119125904.2681402-1-andrew.cooper3@citrix.com
xen/arch/x86/cpu/mcheck/mce.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
[PATCH] x86/mce: Compile do_mca() for CONFIG_PV only
Posted by Andrew Cooper 2 weeks, 3 days ago
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(-)

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)

base-commit: 3128d7248f2ad389b8e9a3e252958cbfbd1898ee
prerequisite-patch-id: 46b8fc2e9df2fd6be1bbbd6b50463e0e15a8f94d
prerequisite-patch-id: c122b170f57ab96fe52c37aebf1f4bb366194637
prerequisite-patch-id: 1c2d96bf17c5da0981b6c62939d3b7cc1e05933e
prerequisite-patch-id: b3e43902729416e18b4fada7f529b4cb02b1815e
prerequisite-patch-id: a06452180f71021893259bb3b883185f57742a31
-- 
2.39.5


Re: [PATCH] x86/mce: Compile do_mca() for CONFIG_PV only
Posted by Jan Beulich 2 weeks, 3 days ago
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

Re: [PATCH] x86/mce: Compile do_mca() for CONFIG_PV only
Posted by Andrew Cooper 2 weeks, 3 days ago
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

Re: [PATCH] x86/mce: Compile do_mca() for CONFIG_PV only
Posted by Jan Beulich 2 weeks, 2 days ago
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

Re: [PATCH] x86/mce: Compile do_mca() for CONFIG_PV only
Posted by Stefano Stabellini 2 weeks, 3 days ago
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>