lkml.org 
[lkml]   [2013]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/8] ACPI, x86: Extended error log driver for x86 platform
On Fri, Oct 11, 2013 at 05:24:52PM +0200, Borislav Petkov wrote:
> Date: Fri, 11 Oct 2013 17:24:52 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: "Chen, Gong" <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-kernel@vger.kernel.org,
> linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 3/8] ACPI, x86: Extended error log driver for x86
> platform
> User-Agent: Mutt/1.5.21 (2010-09-15)
>
> On Fri, Oct 11, 2013 at 02:32:41AM -0400, Chen, Gong wrote:
> > This error log driver (a.k.a eMCA driver) is implemented based on
> > http://www.intel.com/content/www/us/en/architecture-and-technology/enhanced-mca-logging-xeon-paper.html.
> > After errors are captured, more valuable information can be
> > got with this new enhanced error log driver.
> >
> > Signed-off-by: Chen, Gong <gong.chen@linux.intel.com>
> > ---
> > arch/x86/include/asm/mce.h | 5 +
> > arch/x86/kernel/cpu/mcheck/mce.c | 10 ++
> > drivers/acpi/Kconfig | 8 +
> > drivers/acpi/Makefile | 2 +
> > drivers/acpi/acpi_extlog.c | 339 +++++++++++++++++++++++++++++++++++++++
> > drivers/acpi/bus.c | 3 +-
> > include/linux/acpi.h | 1 +
> > 7 files changed, 367 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/acpi/acpi_extlog.c
> >
> > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> > index cbe6b9e..f8c33e2 100644
> > --- a/arch/x86/include/asm/mce.h
> > +++ b/arch/x86/include/asm/mce.h
> > @@ -12,6 +12,7 @@
> > #define MCG_CTL_P (1ULL<<8) /* MCG_CTL register available */
> > #define MCG_EXT_P (1ULL<<9) /* Extended registers available */
> > #define MCG_CMCI_P (1ULL<<10) /* CMCI supported */
> > +#define MCG_EXT_ERR_LOG (1ULL<<26) /* Extended error log supported */
> > #define MCG_EXT_CNT_MASK 0xff0000 /* Number of Extended registers */
> > #define MCG_EXT_CNT_SHIFT 16
> > #define MCG_EXT_CNT(c) (((c) & MCG_EXT_CNT_MASK) >> MCG_EXT_CNT_SHIFT)
>
> Let's keep them numerically sorted by the bit numbers so put
> MCG_EXT_ERR_LOG after bit 24, MCG_SER_P.
>
> Besides, this bit should be called MCG_ELOG_P as it is in the docs
> although your name is much more descriptive than what the hw guys came
> up with but I know they like to abbreviate to the lowest letter count
> possible :)
>
> > @@ -204,6 +205,10 @@ extern void mce_disable_bank(int bank);
> > * Exception handler
> > */
> >
> > +extern spinlock_t mce_elog_lock;
>
> Uuh, I don't think we want to expose the naked spinlock. Rather, we'd
> need a couple of functions
>
> mce_elog_lock
> mce_elog_unlock
>
> which get called by users.
>
> Actually, it would be even better to keep all those details private
> to acpi_extlog.c and have machine_check_poll() call extlog_print()
> and in the cases where there's no extlog support, you have an empty
> extlog_print function.
>
But this driver can be loaded as a module. If this module is unloaded,
extlog_print is gone. I can't keep such a pointer internally.

> > +extern int (*mce_extlog_support)(void);
>
> Unused leftover?
>
Yes, it should be deleted.

> > +/* Call the installed extended error log print function */
> > +extern int (*mce_ext_err_print)(const char *, int cpu, int bank);
> > /* Call the installed machine check handler for this CPU setup. */
> > extern void (*machine_check_vector)(struct pt_regs *, long error_code);
> > void do_machine_check(struct pt_regs *, long);
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index b3218cd..6802637 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -48,6 +48,12 @@
> >
> > #include "mce-internal.h"
> >
> > +DEFINE_SPINLOCK(mce_elog_lock);
> > +EXPORT_SYMBOL_GPL(mce_elog_lock);
>
> Yeah, private to acpi_extlog and it can be simply 'elog_lock' there,
> without the "mce_" prefix.
>
> > +
> > +int (*mce_ext_err_print)(const char *, int, int) = NULL;
> > +EXPORT_SYMBOL_GPL(mce_ext_err_print);
> > +
> > static DEFINE_MUTEX(mce_chrdev_read_mutex);
> >
> > #define rcu_dereference_check_mce(p) \
> > @@ -624,6 +630,10 @@ void machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> > (m.status & (mca_cfg.ser ? MCI_STATUS_S : MCI_STATUS_UC)))
> > continue;
> >
> > + spin_lock(&mce_elog_lock);
> > + if (mce_ext_err_print)
> > + mce_ext_err_print(NULL, m.extcpu, i);
> > + spin_unlock(&mce_elog_lock);
>
> private to acpi_extlog.c
>
> > mce_read_aux(&m, i);
> >
> > if (!(flags & MCP_TIMESTAMP))
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 22327e6..1465fa8 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -372,4 +372,12 @@ config ACPI_BGRT
> >
> > source "drivers/acpi/apei/Kconfig"
> >
> > +config ACPI_EXTLOG
> > + tristate "Extended Error Log support"
> > + depends on X86 && X86_MCE
>
> I guess we can depend only on X86_MCE
>
> > + default n
> > + help
> > + This driver adds support for decoding extended errors from hardware.
> > + which allows the operating system to obtain data from trace.
>
> Let's make this description a bit better:
>
> "Enhanced MCA Logging allows firmware to provide additional error
> information to system software, synchronous with MCE or CMCI. This
> driver adds support for that functionality plus an additional special
> tracepoint which carries that information to userspace."
>
>
> > +
> > endif # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> > index cdaf68b..bce34af 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -82,3 +82,5 @@ processor-$(CONFIG_CPU_FREQ) += processor_perflib.o
> > obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
> >
> > obj-$(CONFIG_ACPI_APEI) += apei/
> > +
> > +obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
> > diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> > new file mode 100644
> > index 0000000..3e3e286
> > --- /dev/null
> > +++ b/drivers/acpi/acpi_extlog.c
> > @@ -0,0 +1,339 @@
> > +/*
> > + * Extended Error Log driver
> > + *
> > + * Copyright (C) 2013 Intel Corp.
> > + * Author: Chen, Gong <gong.chen@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> Please no more of that boilerplate. Simply say that this file is
> licensed under GPLv2 and that's it.
>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <acpi/acpi_bus.h>
> > +#include <linux/cper.h>
> > +#include <linux/ratelimit.h>
> > +#include <asm/mce.h>
> > +
> > +#include "apei/apei-internal.h"
> > +
> > +#define EXT_ELOG_ENTRY_MASK 0xfffffffffffff /* elog entry address mask */
>
> Am I reading this correctly that these are bits [51:0]?
>
> Btw, we have a GENMASK macro in drivers/edac/amd64_edac.h which can be used for
> generating contiguous bitmasks:
>
> #define EXT_ELOG_ENTRY_MASK GENMASK(0, 51)
>
> which makes it much more readable.
>
> You could lift it into a more global header like, say,
> arch/x86/include/asm/edac.h maybe...
>
>
This macro is great and I'd loved to use it. But it looks like a litttle bit
weird to let eMCA depends on a header file like edac.h. Meanwhile, I found
in drivers/video/sis/init.c:3323 we have a very similar macro for this
purpose. So how about writing a separate patch to clean it up first?

> > +
> > +#define EXTLOG_DSM_REV 0x0
> > +#define EXTLOG_FN_QUERY 0x0
> > +#define EXTLOG_FN_ADDR 0x1
> > +
> > +#define FLAG_OS_OPTIN BIT(0)
> > +#define EXTLOG_QUERY_L1_EXIST BIT(1)
> > +#define ELOG_ENTRY_VALID (1ULL<<63)
> > +#define ELOG_ENTRY_LEN 0x1000
> > +
> > +#define EMCA_BUG \
> > + "Can not request iomem region <0x%016llx-0x%016llx> - eMCA disabled\n"
> > +
> > +struct extlog_l1_head {
> > + u32 ver; /* Header Version */
> > + u32 hdr_len; /* Header Length */
> > + u64 total_len; /* entire L1 Directory length including this header */
> > + u64 elog_base; /* MCA Error Log Directory base address */
> > + u64 elog_len; /* MCA Error Log Directory length */
> > + u32 flags; /* bit 0 - OS/VMM Opt-in */
> > + u8 rev0[12];
> > + u32 entries; /* Valid L1 Directory entries per logical processor */
> > + u8 rev1[12];
> > +};
> > +
> > +static u8 extlog_dsm_uuid[] = "663E35AF-CC10-41A4-88EA-5470AF055295";
> > +
> > +/* L1 table related physical address */
> > +static u64 elog_base;
> > +static size_t elog_size;
> > +static u64 l1_dirbase;
> > +static size_t l1_size;
> > +
> > +/* L1 table related virtual address */
> > +static void __iomem *extlog_l1_addr;
> > +static void __iomem *elog_addr;
> > +
> > +static void *elog_buf;
> > +
> > +static u64 *l1_entry_base;
> > +static u32 l1_percpu_entry;
> > +
> > +#define ELOG_IDX(cpu, bank) \
> > + (cpu_physical_id(cpu) * l1_percpu_entry + (bank))
> > +
> > +#define ELOG_ENTRY_DATA(idx) \
> > + (*(l1_entry_base + (idx)))
> > +
> > +#define ELOG_ENTRY_ADDR(phyaddr) \
> > + (phyaddr - elog_base + (u8 *)elog_addr)
> > +
> > +static struct acpi_generic_status *extlog_elog_entry_check(int cpu, int bank)
> > +{
> > + int idx;
> > + u64 data;
> > + struct acpi_generic_status *estatus;
> > +
> > + BUG_ON(cpu < 0);
>
> What is this supposed to guard? And why such a heavy hammer?
>

Because I think in theory "CPU < 0" is impossible. When it hits such
situation, it should be a very serious H/W or firmware bug. At least,
It think it should be a WARN_ON.

> > + idx = ELOG_IDX(cpu, bank);
> > + data = ELOG_ENTRY_DATA(idx);
> > + if ((data & ELOG_ENTRY_VALID) == 0)
> > + return NULL;
> > +
> > + data &= EXT_ELOG_ENTRY_MASK;
> > + estatus = (struct acpi_generic_status *)ELOG_ENTRY_ADDR(data);
> > +
> > + /* if no valid data in elog entry, just return */
> > + if (estatus->block_status == 0)
> > + return NULL;
> > +
> > + return estatus;
> > +}
> > +
> > +static void __print_extlog_rcd(const char *pfx,
> > + struct acpi_generic_status *estatus, int cpu)
>
> Please align args after the opening bracket.
>
> > +{
> > + static atomic_t seqno;
> > + unsigned int curr_seqno;
> > + char pfx_seq[64];
> > +
> > + if (pfx == NULL) {
>
> if (!pfx)
>
> > + if (estatus->error_severity <= CPER_SEV_CORRECTED)
> > + pfx = KERN_INFO;
> > + else
> > + pfx = KERN_ERR;
> > + }
> > + curr_seqno = atomic_inc_return(&seqno);
> > + snprintf(pfx_seq, sizeof(pfx_seq), "%s{%u}", pfx, curr_seqno);
> > + printk("%s""Hardware error detected on CPU%d\n", pfx_seq, cpu);
> > + cper_estatus_print(pfx_seq, estatus);
> > +}
> > +
> > +static int print_extlog_rcd(const char *pfx,
> > + struct acpi_generic_status *estatus, int cpu)a
>
> Ditto.
>
> > +{
> > + /* Not more than 2 messages every 5 seconds */
> > + static DEFINE_RATELIMIT_STATE(ratelimit_corrected, 5*HZ, 2);
> > + static DEFINE_RATELIMIT_STATE(ratelimit_uncorrected, 5*HZ, 2);
> > + struct ratelimit_state *ratelimit;
> > +
> > + if (estatus->error_severity == CPER_SEV_CORRECTED ||
> > + (estatus->error_severity == CPER_SEV_INFORMATIONAL))
>
> alignment:
>
> if ( estatus->error_severity == CPER_SEV_CORRECTED ||
> (estatus->error_severity == CPER_SEV_INFORMATIONAL))
>
> > + ratelimit = &ratelimit_corrected;
> > + else
> > + ratelimit = &ratelimit_uncorrected;
> > + if (__ratelimit(ratelimit)) {
> > + __print_extlog_rcd(pfx, estatus, cpu);
>
> Btw, __print_extlog_rcd is only called once, AFAICT. Why not fold it
> in here?

Just make codes cleaner.

>
> > + return 0;
> > + }
> > +
> > + return 1;
> > +}
> > +
> > +static int extlog_print(const char *pfx, int cpu, int bank)
> > +{
> > + struct acpi_generic_status *estatus;
> > + int rc;
> > +
> > + estatus = extlog_elog_entry_check(cpu, bank);
> > + if (estatus == NULL)
> > + return -EINVAL;
> > +
> > + memcpy(elog_buf, (void *)estatus, ELOG_ENTRY_LEN);
> > + /* clear record status to enable BIOS to update it again */
> > + estatus->block_status = 0;
> > +
> > + rc = print_extlog_rcd(pfx, (struct acpi_generic_status *)elog_buf, cpu);
> > +
> > + return rc;
> > +}
> > +
> > +static int extlog_get_dsm(acpi_handle handle, int rev, int func, u64 *ret)
> > +{
> > + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> > + struct acpi_object_list input;
> > + union acpi_object params[4], *obj;
> > + u8 uuid[16];
> > + int i;
> > +
> > + acpi_str_to_uuid(extlog_dsm_uuid, uuid);
> > + input.count = 4;
> > + input.pointer = params;
> > + params[0].type = ACPI_TYPE_BUFFER;
> > + params[0].buffer.length = 16;
> > + params[0].buffer.pointer = uuid;
> > + params[1].type = ACPI_TYPE_INTEGER;
> > + params[1].integer.value = rev;
> > + params[2].type = ACPI_TYPE_INTEGER;
> > + params[2].integer.value = func;
> > + params[3].type = ACPI_TYPE_PACKAGE;
> > + params[3].package.count = 0;
> > + params[3].package.elements = NULL;
> > +
> > + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
> > + return -1;
> > +
> > + *ret = 0;
> > + obj = (union acpi_object *)buf.pointer;
> > + if (obj->type == ACPI_TYPE_INTEGER)
> > + *ret = obj->integer.value;
> > + else if (obj->type == ACPI_TYPE_BUFFER) {
> > + if (obj->buffer.length <= 8) {
> > + for (i = 0; i < obj->buffer.length; i++)
> > + *ret |= (obj->buffer.pointer[i] << (i * 8));
> > + }
> > + }
> > + kfree(buf.pointer);
>
> I'm guessing this is how acpi does allocation - ACPI_ALLOCATE_BUFFER and
> caller has to free it?
>

Yes it is.

> > +
> > + return 0;
> > +}
> > +
> > +static bool extlog_get_l1addr(void)
> > +{
> > + acpi_handle handle;
> > + u64 ret;
> > +
> > + if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> > + goto fail;
> > +
> > + if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_QUERY, &ret) ||
> > + !(ret & EXTLOG_QUERY_L1_EXIST))
> > + goto fail;
> > +
> > + if (extlog_get_dsm(handle, EXTLOG_DSM_REV, EXTLOG_FN_ADDR, &ret))
> > + goto fail;
> > +
> > + l1_dirbase = ret;
> > + /* Spec says L1 directory must be 4K aligned, bail out if it isn't */
> > + if (l1_dirbase & ((1 << 12) - 1)) {
>
> & (PAGE_SIZE - 1)
>
> > + pr_warn(FW_BUG "L1 Directory is invalid at physical %llx\n",
> > + l1_dirbase);
> > + goto fail;
> > + }
> > +
> > + return true;
> > +fail:
> > + return false;
>
> You probably could drop the labels and simply do "return false" in the
> error cases as it is obvious.
>
> Thanks.
>
> --
> Regards/Gruss,
> Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-10-14 05:41    [W:0.140 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site