| Message ID | 20181009183355.20597-2-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI ADXL and associated EDAC driver change | expand |
On Tue, Oct 9, 2018 at 8:35 PM Tony Luck <tony.luck@intel.com> wrote: > > Some new servers provide an interface so that the OS can ask the > BIOS to translate a system physical address to a memory address > (socket, memory controller, channel, rank, dimm, etc.). This is > useful for EDAC drivers that want to take the address of an error > reported in a machine check bank and let the user know which > DIMM may need to be replaced. > > Specification for this interface is available at: > > https://cdrdv2.intel.com/v1/dl/getContent/603354 > > [Based on earlier code by Qiuxu Zhuo <qiuxu.zhuo@intel.com>] > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > --- > Comments addressed since last version: > Rafael: > Added URL for specification (to commit & header comment) > Fixed NULL error return from error case of adxl_dsm() > > Boris: > Added sanity check on number of address components > (also added check that number of values returned by > decode operation matches number of strings) > g/pr_debug/s//pr_info/ > Added "." at end of sentences in comments > Make return from adxl_get_component_names() immutable > > drivers/acpi/Kconfig | 10 ++ > drivers/acpi/Makefile | 3 + > drivers/acpi/acpi_adxl.c | 199 +++++++++++++++++++++++++++++++++++++++ > include/linux/adxl.h | 25 +++++ > 4 files changed, 237 insertions(+) > create mode 100644 drivers/acpi/acpi_adxl.c > create mode 100644 include/linux/adxl.h > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index dd1eea90f67f..327c93b51cb7 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -498,6 +498,16 @@ config ACPI_EXTLOG > driver adds support for that functionality with corresponding > tracepoint which carries that information to userspace. > > +config ACPI_ADXL > + bool "Physical address to DIMM address translation" > + def_bool n > + help > + Enable interface that calls into BIOS using a DSM (device > + specific method) to convert system physical addresses > + to DIMM (socket, channel, rank, dimm, etc.). > + Only available on some servers. > + Used by newer EDAC drivers. I'm still not sure why this has to be a user-selectable option. If the plan is to select it from the drivers that need it, why do we need to ask about it? And what if it is enabled, but no drivers use it? Will it just be dead code then? > + > menuconfig PMIC_OPREGION > bool "PMIC (Power Management Integrated Circuit) operation region support" > help > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 6d59aa109a91..edc039313cd6 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -61,6 +61,9 @@ acpi-$(CONFIG_ACPI_LPIT) += acpi_lpit.o > acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o > acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o > > +# Address translation > +acpi-$(CONFIG_ACPI_ADXL) += acpi_adxl.o > + > # These are (potentially) separate modules > > # IPMI may be used by other drivers, so it has to initialise before them > diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c > new file mode 100644 > index 000000000000..a58bd8ec396e > --- /dev/null > +++ b/drivers/acpi/acpi_adxl.c > @@ -0,0 +1,199 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Address translation interface via ACPI DSM. > + * Copyright (C) 2018 Intel Corporation > + * > + * Specification for this interface is available at: > + * > + * https://cdrdv2.intel.com/v1/dl/getContent/603354 > + */ > + > +#ifdef CONFIG_ACPI_ADXL > +#include <linux/acpi.h> > +#include <linux/adxl.h> > + > +#define ADXL_REVISION 0x1 > +#define ADXL_IDX_GET_ADDR_PARAMS 0x1 > +#define ADXL_IDX_FORWARD_TRANSLATE 0x2 > +#define ACPI_ADXL_PATH "\\_SB.ADXL" > + > +/* > + * The specification doesn't provide a limit on how many > + * components are in a memory address. But since we allocate > + * memory based on the number the BIOS tells us, we should > + * defend against insane values. > + */ > +#define ADXL_MAX_COMPONENTS 500 > + > +#undef pr_fmt > +#define pr_fmt(fmt) "ADXL: " fmt > + > +static acpi_handle handle; > +static union acpi_object *params; > +static const guid_t adxl_guid = > + GUID_INIT(0xAA3C050A, 0x7EA4, 0x4C1F, > + 0xAF, 0xDA, 0x12, 0x67, 0xDF, 0xD3, 0xD4, 0x8D); > + > +static int adxl_count; > +static char **adxl_component_names; > + > +static union acpi_object *adxl_dsm(int cmd, union acpi_object argv[]) > +{ > + union acpi_object *obj, *o; > + > + obj = acpi_evaluate_dsm_typed(handle, &adxl_guid, ADXL_REVISION, > + cmd, argv, ACPI_TYPE_PACKAGE); > + if (!obj) { > + pr_info("Empty obj\n"); pr_debug() perhaps? It is somewhat devoid of details anyway. Thanks, Rafael
On Wed, Oct 10, 2018 at 11:08:37PM +0200, Rafael J. Wysocki wrote: > > +config ACPI_ADXL > > + bool "Physical address to DIMM address translation" > > + def_bool n > > + help > > + Enable interface that calls into BIOS using a DSM (device > > + specific method) to convert system physical addresses > > + to DIMM (socket, channel, rank, dimm, etc.). > > + Only available on some servers. > > + Used by newer EDAC drivers. > > I'm still not sure why this has to be a user-selectable option. If > the plan is to select it from the drivers that need it, why do we need > to ask about it? And what if it is enabled, but no drivers use it? > Will it just be dead code then? Ah, now I see what you mean. I should make this: config ACPI_ADXL def_bool n right? So the user isn't plagued by a question they don't understand. It will only be set if some other driver that need it selects it. So no way to include dead code. > > + obj = acpi_evaluate_dsm_typed(handle, &adxl_guid, ADXL_REVISION, > > + cmd, argv, ACPI_TYPE_PACKAGE); > > + if (!obj) { > > + pr_info("Empty obj\n"); > > pr_debug() perhaps? It is somewhat devoid of details anyway. I'll make the message more meaningful. I think I want to keep it at higher than "debug" threshold so that we can diagnose problems from the first report, without having to ask for a rerun with a new "dmesg" level. Realistically this should never happen. If it does, its likely a BIOS bug. Thanks for the review. -Tony
On Wed, Oct 10, 2018 at 11:25 PM Luck, Tony <tony.luck@intel.com> wrote: > > On Wed, Oct 10, 2018 at 11:08:37PM +0200, Rafael J. Wysocki wrote: > > > +config ACPI_ADXL > > > + bool "Physical address to DIMM address translation" > > > + def_bool n > > > + help > > > + Enable interface that calls into BIOS using a DSM (device > > > + specific method) to convert system physical addresses > > > + to DIMM (socket, channel, rank, dimm, etc.). > > > + Only available on some servers. > > > + Used by newer EDAC drivers. > > > > I'm still not sure why this has to be a user-selectable option. If > > the plan is to select it from the drivers that need it, why do we need > > to ask about it? And what if it is enabled, but no drivers use it? > > Will it just be dead code then? > > Ah, now I see what you mean. I should make this: > > config ACPI_ADXL > def_bool n > > right? Yes, and you can simply do config ACPI_ADXL bool as it won't be enabled when not explicitly selected anyway. > So the user isn't plagued by a question they don't understand. > It will only be set if some other driver that need it selects it. So > no way to include dead code. Right.
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index dd1eea90f67f..327c93b51cb7 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -498,6 +498,16 @@ config ACPI_EXTLOG driver adds support for that functionality with corresponding tracepoint which carries that information to userspace. +config ACPI_ADXL + bool "Physical address to DIMM address translation" + def_bool n + help + Enable interface that calls into BIOS using a DSM (device + specific method) to convert system physical addresses + to DIMM (socket, channel, rank, dimm, etc.). + Only available on some servers. + Used by newer EDAC drivers. + menuconfig PMIC_OPREGION bool "PMIC (Power Management Integrated Circuit) operation region support" help diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 6d59aa109a91..edc039313cd6 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -61,6 +61,9 @@ acpi-$(CONFIG_ACPI_LPIT) += acpi_lpit.o acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o acpi-$(CONFIG_ACPI_WATCHDOG) += acpi_watchdog.o +# Address translation +acpi-$(CONFIG_ACPI_ADXL) += acpi_adxl.o + # These are (potentially) separate modules # IPMI may be used by other drivers, so it has to initialise before them diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c new file mode 100644 index 000000000000..a58bd8ec396e --- /dev/null +++ b/drivers/acpi/acpi_adxl.c @@ -0,0 +1,199 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Address translation interface via ACPI DSM. + * Copyright (C) 2018 Intel Corporation + * + * Specification for this interface is available at: + * + * https://cdrdv2.intel.com/v1/dl/getContent/603354 + */ + +#ifdef CONFIG_ACPI_ADXL +#include <linux/acpi.h> +#include <linux/adxl.h> + +#define ADXL_REVISION 0x1 +#define ADXL_IDX_GET_ADDR_PARAMS 0x1 +#define ADXL_IDX_FORWARD_TRANSLATE 0x2 +#define ACPI_ADXL_PATH "\\_SB.ADXL" + +/* + * The specification doesn't provide a limit on how many + * components are in a memory address. But since we allocate + * memory based on the number the BIOS tells us, we should + * defend against insane values. + */ +#define ADXL_MAX_COMPONENTS 500 + +#undef pr_fmt +#define pr_fmt(fmt) "ADXL: " fmt + +static acpi_handle handle; +static union acpi_object *params; +static const guid_t adxl_guid = + GUID_INIT(0xAA3C050A, 0x7EA4, 0x4C1F, + 0xAF, 0xDA, 0x12, 0x67, 0xDF, 0xD3, 0xD4, 0x8D); + +static int adxl_count; +static char **adxl_component_names; + +static union acpi_object *adxl_dsm(int cmd, union acpi_object argv[]) +{ + union acpi_object *obj, *o; + + obj = acpi_evaluate_dsm_typed(handle, &adxl_guid, ADXL_REVISION, + cmd, argv, ACPI_TYPE_PACKAGE); + if (!obj) { + pr_info("Empty obj\n"); + return NULL; + } + + if (obj->package.count != 2) { + pr_info("Bad pkg count %d\n", obj->package.count); + goto err; + } + + o = obj->package.elements; + if (o->type != ACPI_TYPE_INTEGER) { + pr_info("Bad 1st element type %d\n", o->type); + goto err; + } + if (o->integer.value) { + pr_info("Bad ret val %llu\n", o->integer.value); + goto err; + } + + o = obj->package.elements + 1; + if (o->type != ACPI_TYPE_PACKAGE) { + pr_info("Bad 2nd element type %d\n", o->type); + goto err; + } + return obj; + +err: + ACPI_FREE(obj); + return NULL; +} + +/** + * adxl_get_component_names - get list of memory component names + * Returns NULL terminated list of string names + * + * Give the caller a pointer to the list of memory component names + * e.g. { "SystemAddress", "ProcessorSocketId", "ChannelId", ... NULL } + * Caller should count how many strings in order to allocate a buffer + * for the return from adxl_decode(). + */ +const char * const *adxl_get_component_names(void) +{ + return (const char * const *)adxl_component_names; +} +EXPORT_SYMBOL_GPL(adxl_get_component_names); + +/** + * adxl_decode - ask BIOS to decode a system address to memory address + * @addr: the address to decode + * @component_values: pointer to array of values for each component + * Returns 0 on success, negative error code otherwise + * + * The index of each value returned in the array matches the index of + * each component name returned by adxl_get_component_names(). + * Components that are not defined for this address translation (e.g. + * mirror channel number for a non-mirrored address) are set to ~0ull. + */ +int adxl_decode(u64 addr, u64 component_values[]) +{ + union acpi_object argv4[2], *results, *r; + int i, cnt; + + if (!adxl_component_names) + return -EOPNOTSUPP; + + argv4[0].type = ACPI_TYPE_PACKAGE; + argv4[0].package.count = 1; + argv4[0].package.elements = &argv4[1]; + argv4[1].integer.type = ACPI_TYPE_INTEGER; + argv4[1].integer.value = addr; + + results = adxl_dsm(ADXL_IDX_FORWARD_TRANSLATE, argv4); + if (!results) + return -EINVAL; + + r = results->package.elements + 1; + cnt = r->package.count; + if (cnt != adxl_count) { + ACPI_FREE(results); + return -EINVAL; + } + r = r->package.elements; + + for (i = 0; i < cnt; i++) + component_values[i] = r[i].integer.value; + + ACPI_FREE(results); + + return 0; +} +EXPORT_SYMBOL_GPL(adxl_decode); + +static bool adxl_detect(void) +{ + char *path = ACPI_ADXL_PATH; + union acpi_object *p; + acpi_status status; + int i; + + status = acpi_get_handle(NULL, path, &handle); + if (ACPI_FAILURE(status)) { + pr_info("No ACPI handle for path %s\n", path); + return false; + } + + if (!acpi_has_method(handle, "_DSM")) { + pr_info("No DSM method\n"); + return false; + } + + if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION, + ADXL_IDX_GET_ADDR_PARAMS | + ADXL_IDX_FORWARD_TRANSLATE)) { + pr_info("No ADXL DSM methods\n"); + return false; + } + + params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL); + if (!params) { + pr_info("Failed to get params\n"); + return false; + } + + p = params->package.elements + 1; + adxl_count = p->package.count; + if (adxl_count > ADXL_MAX_COMPONENTS) { + pr_info("Insane number of address component names %d\n", adxl_count); + ACPI_FREE(params); + return false; + } + p = p->package.elements; + + adxl_component_names = kcalloc(adxl_count + 1, sizeof(char *), GFP_KERNEL); + if (!adxl_component_names) { + ACPI_FREE(params); + return false; + } + + for (i = 0; i < adxl_count; i++) + adxl_component_names[i] = p[i].string.pointer; + + return true; +} + +static int __init adxl_init(void) +{ + if (!adxl_detect()) + return -ENODEV; + return 0; +} +subsys_initcall(adxl_init); + +#endif /* CONFIG_ACPI_ADXL */ diff --git a/include/linux/adxl.h b/include/linux/adxl.h new file mode 100644 index 000000000000..6023704e5d0b --- /dev/null +++ b/include/linux/adxl.h @@ -0,0 +1,25 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Address translation interface via ACPI DSM. + * Copyright (C) 2018 Intel Corporation + */ + +#ifndef _LINUX_ADXL_H +#define _LINUX_ADXL_H + +#ifdef CONFIG_ACPI_ADXL +const char * const *adxl_get_component_names(void); +int adxl_decode(u64 addr, u64 component_values[]); +#else +static inline const char * const *adxl_get_component_names(void) +{ + return NULL; +} + +static inline int adxl_decode(u64 addr, u64 component_values[]) +{ + return -EOPNOTSUPP; +} +#endif + +#endif /* _LINUX_ADXL_H */
Some new servers provide an interface so that the OS can ask the BIOS to translate a system physical address to a memory address (socket, memory controller, channel, rank, dimm, etc.). This is useful for EDAC drivers that want to take the address of an error reported in a machine check bank and let the user know which DIMM may need to be replaced. Specification for this interface is available at: https://cdrdv2.intel.com/v1/dl/getContent/603354 [Based on earlier code by Qiuxu Zhuo <qiuxu.zhuo@intel.com>] Signed-off-by: Tony Luck <tony.luck@intel.com> --- Comments addressed since last version: Rafael: Added URL for specification (to commit & header comment) Fixed NULL error return from error case of adxl_dsm() Boris: Added sanity check on number of address components (also added check that number of values returned by decode operation matches number of strings) g/pr_debug/s//pr_info/ Added "." at end of sentences in comments Make return from adxl_get_component_names() immutable drivers/acpi/Kconfig | 10 ++ drivers/acpi/Makefile | 3 + drivers/acpi/acpi_adxl.c | 199 +++++++++++++++++++++++++++++++++++++++ include/linux/adxl.h | 25 +++++ 4 files changed, 237 insertions(+) create mode 100644 drivers/acpi/acpi_adxl.c create mode 100644 include/linux/adxl.h