[PATCH v2 0/4] xen/mce: fix file-scoped static variables in mce.c

[PATCH v2 0/4] xen/mce: fix file-scoped static variables in mce.c

[Haozhong Zhang]

Changes in v2:
 * (Patch 4) Move v1 patch 1 to the last one in this series, so as to
   avoid blocking others in case it's too late for 4.9.
 * (Patch 1) Make static mce barriers private to their users.
 * (Patch 2&3) Separate v1 patch 2 into two patches, as they fix
   different bugs.

Haozhong Zhang (4):
  xen/mce: make mce barriers private to their users
  xen/mce: make 'severity_cpu' private to its users
  xen/mce: always re-initialize 'severity_cpu' in mcheck_cmn_handler()
  xen/mce: make 'found_error' and 'mce_fatal_cpus' private to mcheck_cmn_handler()

 xen/arch/x86/cpu/mcheck/barrier.h |  7 +++++++
 xen/arch/x86/cpu/mcheck/mce.c     | 30 +++++++++++++++++++-----------
 2 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.10.1


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

[PATCH v2 1/4] xen/mce: make mce barriers private to their users

[Haozhong Zhang]

Each of current mce barriers is actually used by only one function, so
move their definitions into their users. A static mce barrier initializer
is introduced so we can move the initialization of above mce barriers
to their definitions.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/barrier.h |  7 +++++++
 xen/arch/x86/cpu/mcheck/mce.c     | 10 +++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/barrier.h b/xen/arch/x86/cpu/mcheck/barrier.h
index 87f7550..c22321c 100644
--- a/xen/arch/x86/cpu/mcheck/barrier.h
+++ b/xen/arch/x86/cpu/mcheck/barrier.h
@@ -10,6 +10,13 @@ struct mce_softirq_barrier {
     atomic_t outgen;
 };
 
+#define DEFINE_MCE_BARRIER(barrier)        \
+    struct mce_softirq_barrier barrier = { \
+        .val    = ATOMIC_INIT(0),          \
+        .ingen  = ATOMIC_INIT(0),          \
+        .outgen = ATOMIC_INIT(0),          \
+    }
+
 /*
  * Initialize a barrier. Just set it to 0.
  */
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 11d0e23..437d155 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -174,10 +174,6 @@ void mce_need_clearbank_register(mce_need_clearbank_t cbfunc)
     mc_need_clearbank_scan = cbfunc;
 }
 
-
-static struct mce_softirq_barrier mce_inside_bar, mce_severity_bar;
-static struct mce_softirq_barrier mce_trap_bar;
-
 /*
  * mce_logout_lock should only be used in the trap handler,
  * while MCIP has not been cleared yet in the global status
@@ -453,6 +449,7 @@ static int mce_urgent_action(const struct cpu_user_regs *regs,
 /* Shared #MC handler. */
 void mcheck_cmn_handler(const struct cpu_user_regs *regs)
 {
+    static DEFINE_MCE_BARRIER(mce_trap_bar);
     struct mca_banks *bankmask = mca_allbanks;
     struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
     uint64_t gstatus;
@@ -1698,6 +1695,8 @@ static int mce_delayed_action(mctelem_cookie_t mctc)
 /* Softirq Handler for this MCE# processing */
 static void mce_softirq(void)
 {
+    static DEFINE_MCE_BARRIER(mce_inside_bar);
+    static DEFINE_MCE_BARRIER(mce_severity_bar);
     int cpu = smp_processor_id();
     unsigned int workcpu;
 
@@ -1764,9 +1763,6 @@ void mce_handler_init(void)
 
     /* callback register, do we really need so many callback? */
     /* mce handler data initialization */
-    mce_barrier_init(&mce_inside_bar);
-    mce_barrier_init(&mce_severity_bar);
-    mce_barrier_init(&mce_trap_bar);
     spin_lock_init(&mce_logout_lock);
     open_softirq(MACHINE_CHECK_SOFTIRQ, mce_softirq);
 }
-- 
2.10.1


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

Re: [PATCH v2 1/4] xen/mce: make mce barriers private to their users

[Jan Beulich]

>>> On 06.04.17 at 14:33, <haozhong.zhang@intel.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/barrier.h
> +++ b/xen/arch/x86/cpu/mcheck/barrier.h
> @@ -10,6 +10,13 @@ struct mce_softirq_barrier {
>      atomic_t outgen;
>  };
>  
> +#define DEFINE_MCE_BARRIER(barrier)        \

With the parameter name changed to "name" or "sym" or some such
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

[PATCH v2 2/4] xen/mce: make 'severity_cpu' private to its users

[Haozhong Zhang]

The current 'severity_cpu' is used by both mcheck_cmn_handler() and
mce_softirq(). If MC# happens during mce_softirq(), the values set in
mcheck_cmn_handler() and mce_softirq() may interfere with each
other. Use private 'severity_cpu' for each function to fix this issue.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 437d155..a2e9668 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -182,7 +182,6 @@ void mce_need_clearbank_register(mce_need_clearbank_t cbfunc)
  */
 static DEFINE_SPINLOCK(mce_logout_lock);
 
-static atomic_t severity_cpu = ATOMIC_INIT(-1);
 static atomic_t found_error = ATOMIC_INIT(0);
 static cpumask_t mce_fatal_cpus;
 
@@ -450,6 +449,7 @@ static int mce_urgent_action(const struct cpu_user_regs *regs,
 void mcheck_cmn_handler(const struct cpu_user_regs *regs)
 {
     static DEFINE_MCE_BARRIER(mce_trap_bar);
+    static atomic_t severity_cpu = ATOMIC_INIT(-1);
     struct mca_banks *bankmask = mca_allbanks;
     struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
     uint64_t gstatus;
@@ -1697,6 +1697,7 @@ static void mce_softirq(void)
 {
     static DEFINE_MCE_BARRIER(mce_inside_bar);
     static DEFINE_MCE_BARRIER(mce_severity_bar);
+    static atomic_t severity_cpu;
     int cpu = smp_processor_id();
     unsigned int workcpu;
 
-- 
2.10.1


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

Re: [PATCH v2 2/4] xen/mce: make 'severity_cpu' private to its users

[Jan Beulich]

>>> On 06.04.17 at 14:33, <haozhong.zhang@intel.com> wrote:
> The current 'severity_cpu' is used by both mcheck_cmn_handler() and
> mce_softirq(). If MC# happens during mce_softirq(), the values set in
> mcheck_cmn_handler() and mce_softirq() may interfere with each
> other. Use private 'severity_cpu' for each function to fix this issue.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Other than patch 1 I think this qualifies for 4.9.

Jan


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

Re: [PATCH v2 2/4] xen/mce: make 'severity_cpu' private to its users

[Jan Beulich]

>>> On 07.04.17 at 15:48, <JBeulich@suse.com> wrote:
>>>> On 06.04.17 at 14:33, <haozhong.zhang@intel.com> wrote:
>> The current 'severity_cpu' is used by both mcheck_cmn_handler() and
>> mce_softirq(). If MC# happens during mce_softirq(), the values set in
>> mcheck_cmn_handler() and mce_softirq() may interfere with each
>> other. Use private 'severity_cpu' for each function to fix this issue.
>> 
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Other than patch 1 I think this qualifies for 4.9.

And for that reason it is generally better to put bug fixes first in
your series, and pure cleanup last. This patch will now need
re-basing and the committed patch 2 (which I did adjust as
necessary to apply on its own).

Jan


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

[PATCH v2 3/4] xen/mce: always re-initialize 'severity_cpu' in mcheck_cmn_handler()

[Haozhong Zhang]

mcheck_cmn_handler() does not always set 'severity_cpu' to override
its value taken from previous rounds of MC handling, which will
interfere the current round of MC handling. Always re-initialize it to
clear the historical value.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index a2e9668..d1bc642 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -449,13 +449,25 @@ static int mce_urgent_action(const struct cpu_user_regs *regs,
 void mcheck_cmn_handler(const struct cpu_user_regs *regs)
 {
     static DEFINE_MCE_BARRIER(mce_trap_bar);
-    static atomic_t severity_cpu = ATOMIC_INIT(-1);
+    static DEFINE_MCE_BARRIER(mce_handler_init_bar);
+    static atomic_t severity_cpu;
     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;
 
+    /*
+     * Re-initialize severity_cpu to clear historical information
+     * taken from previous rounds of MC handling. Besides this
+     * initialization, severity_cpu is not always set below to
+     * override the previous value, so the re-initialization is
+     * necessary.
+     */
+    mce_barrier_enter(&mce_handler_init_bar);
+    atomic_set(&severity_cpu, -1);
+    mce_barrier_exit(&mce_handler_init_bar);
+
     mce_spin_lock(&mce_logout_lock);
 
     if (clear_bank != NULL) {
-- 
2.10.1


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

[RESEND PATCH v2 3/4] xen/mce: always re-initialize 'severity_cpu' in mcheck_cmn_handler()

[Haozhong Zhang]

mcheck_cmn_handler() does not always set 'severity_cpu' to override
its value taken from previous rounds of MC handling, which will
interfere the current round of MC handling. Always re-initialize it to
clear the historical value.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Reason for resend:
 * Move the re-initialization after its last use, so that we can
   avoid introducing a new mce barrier.
---
 xen/arch/x86/cpu/mcheck/mce.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index a2e9668..0d9d5b0 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -523,6 +523,7 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
             mc_panic(ebuf);
         }
         atomic_set(&found_error, 0);
+        atomic_set(&severity_cpu, -1);
     }
     mce_barrier_exit(&mce_trap_bar);
 
-- 
2.10.1


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

Re: [RESEND PATCH v2 3/4] xen/mce: always re-initialize 'severity_cpu' in mcheck_cmn_handler()

[Jan Beulich]

>>> On 07.04.17 at 09:52, <haozhong.zhang@intel.com> wrote:
> mcheck_cmn_handler() does not always set 'severity_cpu' to override
> its value taken from previous rounds of MC handling, which will
> interfere the current round of MC handling. Always re-initialize it to
> clear the historical value.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Same here as to 4.9.

> ---
> Reason for resend:
>  * Move the re-initialization after its last use, so that we can
>    avoid introducing a new mce barrier.

In the future please don't call this a "resend" - it is a new version if
you make a change.

Jan


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

[PATCH v2 4/4] xen/mce: make 'found_error' and 'mce_fatal_cpus' private to mcheck_cmn_handler()

[Haozhong Zhang]

mcheck_cmn_handler() is the only user of 'found_error' and
'mce_fatal_cpus'.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 xen/arch/x86/cpu/mcheck/mce.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index d1bc642..39d19a3 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -182,9 +182,6 @@ void mce_need_clearbank_register(mce_need_clearbank_t cbfunc)
  */
 static DEFINE_SPINLOCK(mce_logout_lock);
 
-static atomic_t found_error = ATOMIC_INIT(0);
-static cpumask_t mce_fatal_cpus;
-
 const struct mca_error_handler *__read_mostly mce_dhandlers;
 const struct mca_error_handler *__read_mostly mce_uhandlers;
 unsigned int __read_mostly mce_dhandler_num;
@@ -451,6 +448,8 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
     static DEFINE_MCE_BARRIER(mce_trap_bar);
     static DEFINE_MCE_BARRIER(mce_handler_init_bar);
     static atomic_t severity_cpu;
+    static atomic_t found_error = ATOMIC_INIT(0);
+    static cpumask_t mce_fatal_cpus;
     struct mca_banks *bankmask = mca_allbanks;
     struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
     uint64_t gstatus;
-- 
2.10.1


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

[RESEND PATCH v2 4/4] xen/mce: make 'found_error' and 'mce_fatal_cpus' private to mcheck_cmn_handler()

[Haozhong Zhang]

mcheck_cmn_handler() is the only user of 'found_error' and
'mce_fatal_cpus'.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Reason for resend:
 * Adapt for resent patch 3.
---
 xen/arch/x86/cpu/mcheck/mce.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index 0d9d5b0..54fd000 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -182,9 +182,6 @@ void mce_need_clearbank_register(mce_need_clearbank_t cbfunc)
  */
 static DEFINE_SPINLOCK(mce_logout_lock);
 
-static atomic_t found_error = ATOMIC_INIT(0);
-static cpumask_t mce_fatal_cpus;
-
 const struct mca_error_handler *__read_mostly mce_dhandlers;
 const struct mca_error_handler *__read_mostly mce_uhandlers;
 unsigned int __read_mostly mce_dhandler_num;
@@ -450,6 +447,8 @@ void mcheck_cmn_handler(const struct cpu_user_regs *regs)
 {
     static DEFINE_MCE_BARRIER(mce_trap_bar);
     static atomic_t severity_cpu = ATOMIC_INIT(-1);
+    static atomic_t found_error = ATOMIC_INIT(0);
+    static cpumask_t mce_fatal_cpus;
     struct mca_banks *bankmask = mca_allbanks;
     struct mca_banks *clear_bank = __get_cpu_var(mce_clear_banks);
     uint64_t gstatus;
-- 
2.10.1


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

Re: [RESEND PATCH v2 4/4] xen/mce: make 'found_error' and 'mce_fatal_cpus' private to mcheck_cmn_handler()

[Jan Beulich]

>>> On 07.04.17 at 09:52, <haozhong.zhang@intel.com> wrote:
> mcheck_cmn_handler() is the only user of 'found_error' and
> 'mce_fatal_cpus'.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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