| Message ID | 20170217063936.13208-7-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > Implementations of these two functions are effectively the same, so > unify them by a common intel_default_mce_handler(). Them being the same right now may also be an issue with the earlier authors never having completed their job. I'd like to see justification here that the two handlers also are conceptionally (mostly) identical. Mechanically the patch is fine. Jan
On 02/17/17 03:01 -0700, Jan Beulich wrote: > >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > > Implementations of these two functions are effectively the same, so > > unify them by a common intel_default_mce_handler(). > > Them being the same right now may also be an issue with the > earlier authors never having completed their job. I'd like to see > justification here that the two handlers also are conceptionally > (mostly) identical. Mechanically the patch is fine. > Maybe conceptually not, because 1) uhandler is called in MCE context and, IIUC, should be more careful than dhandler; 2) the 3rd argument "regs" only makes sense in MCE context, so dhandler should never use them. I think a better alternative is to move the same code to a common function and let uhandler and dhandler call that common function. In addition, I'll leave a comment to explain why not merge them. Haozhong
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c index 498e8e4..b5ee8b8 100644 --- a/xen/arch/x86/cpu/mcheck/mce_intel.c +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c @@ -342,7 +342,7 @@ static int intel_default_check(uint64_t status) return 1; } -static void intel_default_mce_dhandler( +static void intel_default_mce_handler( struct mca_binfo *binfo, enum mce_result *result, const struct cpu_user_regs * regs) @@ -361,32 +361,11 @@ static void intel_default_mce_dhandler( static const struct mca_error_handler intel_mce_dhandlers[] = { {intel_srao_check, intel_srao_dhandler}, {intel_srar_check, intel_srar_dhandler}, - {intel_default_check, intel_default_mce_dhandler} + {intel_default_check, intel_default_mce_handler} }; -static void intel_default_mce_uhandler( - struct mca_binfo *binfo, - enum mce_result *result, - const struct cpu_user_regs *regs) -{ - uint64_t status = binfo->mib->mc_status; - enum intel_mce_type type; - - type = intel_check_mce_type(status); - - switch (type) - { - case intel_mce_fatal: - *result = MCER_RESET; - break; - default: - *result = MCER_CONTINUE; - break; - } -} - static const struct mca_error_handler intel_mce_uhandlers[] = { - {intel_default_check, intel_default_mce_uhandler} + {intel_default_check, intel_default_mce_handler} }; /* According to MCA OS writer guide, CMCI handler need to clear bank when
Implementations of these two functions are effectively the same, so unify them by a common intel_default_mce_handler(). Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- Cc: Christoph Egger <chegger@amazon.de> Cc: Liu Jinsong <jinsong.liu@alibaba-inc.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/cpu/mcheck/mce_intel.c | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-)