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

[<haozhong.zhang@xxxxxxxxx>]

On 06/27/17 01:13 -0600, Jan Beulich wrote:
> >>> Haozhong Zhang <haozhong.zhang@xxxxxxxxx> 06/26/17 11:17 AM >>>
> >+/**
> >+ * Append a telemetry of deferred MCE to a per-cpu pending list,
> >+ * either @pending or @lmce_pending, according to rules below:
> >+ *  - if @pending is not empty, then the new telemetry will be
> >+ *    appended to @pending;
> >+ *  - if @pending is empty and the new telemetry is for a deferred
> >+ *    LMCE, then the new telemetry will be appended to @lmce_pending;
> >+ *  - if @pending is empty and the new telemetry is for a deferred
> >+ *    non-local MCE, all existing telemetries in @lmce_pending will be
> >+ *    moved to @pending and then the new telemetry will be appended to
> >+ *    @pending.
> >+ *
> >+ * This function must be called with MCIP bit set, so that it does not
> >+ * need to worry about MC# re-occurring in this function.
> >+ *
> >+ * As a result, this function can preserve the mutual exclusivity
> >+ * between @pending and @lmce_pending (see their comments in struct
> >+ * mc_telem_cpu_ctl).
> >+ *
> >+ * Parameters:
> >+ *  @cookie: telemetry of the deferred MCE
> >+ *  @lmce:   indicate whether the telemetry is for LMCE
> >+ */
> >+void mctelem_defer(mctelem_cookie_t cookie, bool lmce)
>  >{
>       >struct mctelem_ent *tep = COOKIE2MCTE(cookie);
> >-
> >-    mctelem_xchg_head(&this_cpu(mctctl.pending), &tep->mcte_next, tep);
> >+    struct mc_telem_cpu_ctl *mctctl = &this_cpu(mctctl);
> >+
> >+    ASSERT(mctctl->pending == NULL || mctctl->lmce_pending == NULL);
> >+
> >+    if (mctctl->pending)
> >+            mctelem_xchg_head(&mctctl->pending, &tep->mcte_next, tep);
> >+    else if (lmce)
> >+            mctelem_xchg_head(&mctctl->lmce_pending, &tep->mcte_next, tep);
> >+    else {
> >+            if (mctctl->lmce_pending)
> >+                    mctelem_xchg_head(&mctctl->lmce_pending,
> >+                                      &mctctl->pending, NULL);
> 
> I don't think this is sufficiently proven to be safe: This may set ->pending 
> to
> non-NULL more than once, and while your comment above considers the
> producer side, it doesn't consider the consumer(s). This is even more so that
> the consumer side uses potentially stale information to tell which list head 
> to
> update.

What problems do you think will be caused by setting ->pending to
non-NULL more than once? The only such case is the last else branch:
it corresponds to a broadcasting MC#, so all CPUs are in the exception
context and no one is consuming ->pending at this moment.

Haozhong


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