Re: [PATCH 2/2] x86/mce: Sanitise the #MC entry path

["Egger, Christoph" <chegger@amazon.de>]

On 09.06.14 12:56, Andrew Cooper wrote:
> The 'error_code' function parameters are not used at all; drop it from the
> call chain.  If it is needed at some point in the future, it is available via
> cpu_user_regs.
> 
> Having do_machine_check() call the non-inlineable machine_check_vector() just
> to get at the static function pointer '_machine_check_vector' is silly.  Move
> do_machine_check() from traps.c to mce.c and do away with
> machine_check_vector() entirely.
> 
> Both {intel,amd}_init_mce() register their own local function as the #MC
> handler, each of which call mcheck_cmn_handler() in an identical way.  Fix
> this craziness by actually turning mcheck_cmn_handler() into a valid #MC
> handler (as its comments already state), and have {intel,amd}_init_mce()
> register it instead of their own private handlers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Christoph Egger <chegger@amazon.de>
> CC: Liu Jinsong <jinsong.liu@alibaba-inc.com>

Acked-by: Christoph Egger <chegger@amazon.de>

> ---
>  xen/arch/x86/cpu/mcheck/mce.c       |   11 ++++++-----
>  xen/arch/x86/cpu/mcheck/mce.h       |    5 ++---
>  xen/arch/x86/cpu/mcheck/mce_amd.c   |    9 +--------
>  xen/arch/x86/cpu/mcheck/mce_intel.c |    8 +-------
>  xen/arch/x86/traps.c                |    5 -----
>  xen/include/asm-x86/traps.h         |    2 --
>  6 files changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
> index 13e5547..812daf6 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -70,7 +70,7 @@ static void __init mce_set_verbosity(char *str)
>  custom_param("mce_verbosity", mce_set_verbosity);
>  
>  /* Handle unconfigured int18 (should never happen) */
> -static void unexpected_machine_check(const struct cpu_user_regs *regs, long error_code)
> +static void unexpected_machine_check(const struct cpu_user_regs *regs)
>  {
>      console_force_unlock();
>      printk("Unexpected Machine Check Exception\n");
> @@ -88,9 +88,9 @@ void x86_mce_vector_register(x86_mce_vector_t hdlr)
>  
>  /* Call the installed machine check handler for this CPU setup. */
>  
> -void machine_check_vector(const struct cpu_user_regs *regs, long error_code)
> +void do_machine_check(const struct cpu_user_regs *regs)
>  {
> -    _machine_check_vector(regs, error_code);
> +    _machine_check_vector(regs);
>  }
>  
>  /* Init machine check callback handler
> @@ -459,9 +459,10 @@ static int mce_urgent_action(const struct cpu_user_regs *regs,
>  }
>  
>  /* Shared #MC handler. */
> -void mcheck_cmn_handler(const struct cpu_user_regs *regs, long error_code,
> -    struct mca_banks *bankmask, struct mca_banks *clear_bank)
> +void mcheck_cmn_handler(const struct cpu_user_regs *regs)
>  {
> +    struct mca_banks *bankmask = mca_allbanks;
> +    struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
>      uint64_t gstatus;
>      mctelem_cookie_t mctc = NULL;
>      struct mca_summary bs;
> diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
> index 0397f80..e83d431 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -62,13 +62,12 @@ void x86_mc_get_cpu_info(unsigned, uint32_t *, uint16_t *, uint16_t *,
>  			 uint32_t *, uint32_t *, uint32_t *, uint32_t *);
>  
>  /* Register a handler for machine check exceptions. */
> -typedef void (*x86_mce_vector_t)(const struct cpu_user_regs *, long);
> +typedef void (*x86_mce_vector_t)(const struct cpu_user_regs *regs);
>  extern void x86_mce_vector_register(x86_mce_vector_t);
>  
>  /* Common generic MCE handler that implementations may nominate
>   * via x86_mce_vector_register. */
> -extern void mcheck_cmn_handler(const struct cpu_user_regs *, long,
> -    struct mca_banks *, struct mca_banks *);
> +extern void mcheck_cmn_handler(const struct cpu_user_regs *regs);
>  
>  /* Register a handler for judging whether mce is recoverable. */
>  typedef int (*mce_recoverable_t)(uint64_t status);
> diff --git a/xen/arch/x86/cpu/mcheck/mce_amd.c b/xen/arch/x86/cpu/mcheck/mce_amd.c
> index 9daa461..4e8ad38 100644
> --- a/xen/arch/x86/cpu/mcheck/mce_amd.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_amd.c
> @@ -241,13 +241,6 @@ amd_f10_handler(struct mc_info *mi, uint16_t bank, uint64_t status)
>      return mc_ext;
>  }
>  
> -/* Common AMD Machine Check Handler for AMD K8 and higher */
> -static void amd_cmn_machine_check(const struct cpu_user_regs *regs, long error_code)
> -{
> -    mcheck_cmn_handler(regs, error_code, mca_allbanks,
> -                       __get_cpu_var(mce_clear_banks));
> -}
> -
>  static int amd_need_clearbank_scan(enum mca_source who, uint64_t status)
>  {
>      if ( who != MCA_MCE_SCAN )
> @@ -287,7 +280,7 @@ amd_mcheck_init(struct cpuinfo_x86 *ci)
>      /* Assume that machine check support is available.
>       * The minimum provided support is at least the K8. */
>      mce_handler_init();
> -    x86_mce_vector_register(amd_cmn_machine_check);
> +    x86_mce_vector_register(mcheck_cmn_handler);
>      mce_need_clearbank_register(amd_need_clearbank_scan);
>  
>      for ( i = 0; i < nr_mce_banks; i++ )
> diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
> index ad06efc..48e797e 100644
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
> @@ -384,12 +384,6 @@ static const struct mca_error_handler intel_mce_uhandlers[] = {
>      {intel_default_check, intel_default_mce_uhandler}
>  };
>  
> -static void intel_machine_check(const struct cpu_user_regs * regs, long error_code)
> -{
> -    mcheck_cmn_handler(regs, error_code, mca_allbanks,
> -        __get_cpu_var(mce_clear_banks));
> -}
> -
>  /* According to MCA OS writer guide, CMCI handler need to clear bank when
>   * 1) CE (UC = 0)
>   * 2) ser_support = 1, Superious error, OVER = 0, EN = 0, [UC = 1]
> @@ -772,7 +766,7 @@ static void intel_init_mce(void)
>      if (firstbank) /* if cmci enabled, firstbank = 0 */
>          wrmsrl(MSR_IA32_MC0_STATUS, 0x0ULL);
>  
> -    x86_mce_vector_register(intel_machine_check);
> +    x86_mce_vector_register(mcheck_cmn_handler);
>      mce_recoverable_register(intel_recoverable_scan);
>      mce_need_clearbank_register(intel_need_clearbank_scan);
>      mce_register_addrcheck(intel_checkaddr);
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 44ac014..8e38c5a 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1086,11 +1086,6 @@ void do_int3(struct cpu_user_regs *regs)
>      do_guest_trap(TRAP_int3, regs, 0);
>  }
>  
> -void do_machine_check(const struct cpu_user_regs *regs)
> -{
> -    machine_check_vector(regs, regs->error_code);
> -}
> -
>  static void reserved_bit_page_fault(
>      unsigned long addr, struct cpu_user_regs *regs)
>  {
> diff --git a/xen/include/asm-x86/traps.h b/xen/include/asm-x86/traps.h
> index 47b7ab9..ebb6378 100644
> --- a/xen/include/asm-x86/traps.h
> +++ b/xen/include/asm-x86/traps.h
> @@ -28,8 +28,6 @@ struct softirq_trap {
>  
>  struct cpu_user_regs;
>  
> -extern void machine_check_vector(const struct cpu_user_regs *regs, long error_code);
> -
>  void async_exception_cleanup(struct vcpu *);
>   
>  /**
>