Re: [Xen-devel] [PATCH v5 03/11] x86/mce: handle host LMCE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 05:46, <haozhong.zhang@xxxxxxxxx> wrote:
> @@ -454,6 +455,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
>      uint64_t gstatus;
>      mctelem_cookie_t mctc = NULL;
>      struct mca_summary bs;
> +    bool wait, lmce;

Considering the code being replaced as well as ...

> @@ -462,6 +464,8 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
>              sizeof(long) * BITS_TO_LONGS(clear_bank->num));
>      }
>      mctc = mcheck_mca_logout(MCA_MCE_SCAN, bankmask, &bs, clear_bank);
> +    lmce = bs.lmce;
> +    wait = mce_broadcast && !lmce;

... this, I think the former variable would better be named
"broadcast" (or "bcast"). Same in mce_softirq() then, of course.

> @@ -497,16 +501,16 @@ void mcheck_cmn_handler(const struct cpu_user_regs 
> *regs)
>      }
>      mce_spin_unlock(&mce_logout_lock);
>  
> -    mce_barrier_enter(&mce_trap_bar, mce_broadcast);
> +    mce_barrier_enter(&mce_trap_bar, wait);
>      if ( mctc != NULL && mce_urgent_action(regs, mctc))
>          cpumask_set_cpu(smp_processor_id(), &mce_fatal_cpus);
> -    mce_barrier_exit(&mce_trap_bar, mce_broadcast);
> +    mce_barrier_exit(&mce_trap_bar, wait);
>  
>      /*
>       * Wait until everybody has processed the trap.
>       */
> -    mce_barrier_enter(&mce_trap_bar, mce_broadcast);
> -    if (atomic_read(&severity_cpu) == smp_processor_id())
> +    mce_barrier_enter(&mce_trap_bar, wait);
> +    if (lmce || atomic_read(&severity_cpu) == smp_processor_id())
>      {

Considering the brace placement you'd really want blanks inside
the if() parens, but this file is so badly mixed in style that I wouldn't
insist. The alternative would be to move the brace up - that would
perhaps match up better with at least the "majority" of style.

>  void mctelem_process_deferred(unsigned int cpu,
> -                           int (*fn)(mctelem_cookie_t))
> +                           int (*fn)(mctelem_cookie_t),
> +                           bool lmce)
>  {
>       struct mctelem_ent *tep;
>       struct mctelem_ent *head, *prev;
> +     struct mc_telem_cpu_ctl *mctctl = &per_cpu(mctctl, cpu);
>       int ret;
>  
>       /*
>        * First, unhook the list of telemetry structures, and  
>        * hook it up to the processing list head for this CPU.
> +      *
> +      * If @lmce is true and a non-local MC# occurs before the
> +      * following atomic exchange, @lmce will not hold after
> +      * resumption, because all telemetries in @lmce_pending on
> +      * @cpu are moved to @pending on @cpu in mcheck_cmn_handler().
> +      * In such a case, no telemetries will be handled in this
> +      * function after resumption. Another round of MCE softirq,
> +      * which was raised by above mcheck_cmn_handler(), will handle
> +      * those moved telemetries in @pending on @cpu.
> +      *
> +      * If another MC# occurs after the following atomic exchange,
> +      * it will be handled by another round of MCE softirq.

This restates what the earlier paragraph of the comment already
says. With these taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel