All of lore.kernel.org
 help / color / mirror / Atom feed
* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-12 18:01 Luck, Tony
  0 siblings, 0 replies; 7+ messages in thread
From: Luck, Tony @ 2018-10-12 18:01 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Rafael J. Wysocki, Qiuxu Zhuo, Aristeu Rozanski,
	Mauro Carvalho Chehab, linux-edac, linux-acpi

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>]

Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

   Boris:
	Add verb to first line commit description
	Don't need #ifdef CONFIG_ACPI_ADXL guard any more
	Comment for allocation of extra element for NULL terminator
	Merge adxl_detect() into adxl_init()

   Tony:
	Make couple of error messages more descriptive
---
 drivers/acpi/Kconfig     |  10 ++
 drivers/acpi/Makefile    |   3 +
 drivers/acpi/acpi_adxl.c | 192 +++++++++++++++++++++++++++++++++++++++
 include/linux/adxl.h     |  25 +++++
 4 files changed, 230 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.
+
 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..683235f96011
--- /dev/null
+++ b/drivers/acpi/acpi_adxl.c
@@ -0,0 +1,192 @@
+// 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
+ */
+
+#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 int __init adxl_init(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 -ENODEV;
+	}
+
+	if (!acpi_has_method(handle, "_DSM")) {
+		pr_info("No DSM method\n");
+		return -ENODEV;
+	}
+
+	if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REVISION,
+			    ADXL_IDX_GET_ADDR_PARAMS |
+			    ADXL_IDX_FORWARD_TRANSLATE)) {
+		pr_info("DSM method does not support forward translate\n");
+		return -ENODEV;
+	}
+
+	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
+	if (!params) {
+		pr_info("Failed to get component names\n");
+		return -ENODEV;
+	}
+
+	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 -ENODEV;
+	}
+	p = p->package.elements;
+
+	/*
+	 * Allocate one extra for NULL termination.
+	 */
+	adxl_component_names = kcalloc(adxl_count + 1, sizeof(char *), GFP_KERNEL);
+	if (!adxl_component_names) {
+		ACPI_FREE(params);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < adxl_count; i++)
+		adxl_component_names[i] = p[i].string.pointer;
+
+	return 0;
+}
+subsys_initcall(adxl_init);
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 */

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-31 18:16 Borislav Petkov
  0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2018-10-31 18:16 UTC (permalink / raw)
  To: Luck, Tony
  Cc: Rafael J. Wysocki, Rafael Wysocki, qiuxu.zhuo, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

On Wed, Oct 31, 2018 at 11:08:01AM -0700, Luck, Tony wrote:
> It's not exactly ideal to build the skx_edac.c driver with these
> stubs. Sure, it make the kernel link without errors. But now you
> silently end up with a driver that doesn't really do all you want
> it to do.
> 
> Perhaps this isn't a huge issue. Only "randconfig" would try to
> build a kernel without ACPI. The user will find other stuff is
> broken in an ACPI=n kernel long before they notice the lack of
> EDAC error reporting.

Exactly!

> So:
> 
> Acked-by: Tony Luck <tony.luck@intel.com>

Thx. I'll add it to the lineup for Linus.

> Oops. Return from this isn't "bool". Caller expects 0 = success.
> 
> > +static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }
> 
> This should return -EOPNOTSUPP;

Done.

Thx.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-31 18:12 Luck, Tony
  0 siblings, 0 replies; 7+ messages in thread
From: Luck, Tony @ 2018-10-31 18:12 UTC (permalink / raw)
  To: Luck, Tony, Borislav Petkov
  Cc: Rafael J. Wysocki, Wysocki, Rafael J, Zhuo, Qiuxu, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

Oops. Return from this isn't "bool". Caller expects 0 = success.

> +static inline int adxl_decode(u64 addr, u64 component_values[])   { return false; }

This should return -EOPNOTSUPP;

-Tony

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-31 18:08 Luck, Tony
  0 siblings, 0 replies; 7+ messages in thread
From: Luck, Tony @ 2018-10-31 18:08 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Rafael J. Wysocki, Rafael Wysocki, qiuxu.zhuo, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

On Wed, Oct 31, 2018 at 07:00:33PM +0100, Borislav Petkov wrote:
> +#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 false; }
> +#endif
>  
>  #endif /* _LINUX_ADXL_H */

It's not exactly ideal to build the skx_edac.c driver with these
stubs. Sure, it make the kernel link without errors. But now you
silently end up with a driver that doesn't really do all you want
it to do.

Perhaps this isn't a huge issue. Only "randconfig" would try to
build a kernel without ACPI. The user will find other stuff is
broken in an ACPI=n kernel long before they notice the lack of
EDAC error reporting.

So:

Acked-by: Tony Luck <tony.luck@intel.com>

-Tony

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-31 18:00 Borislav Petkov
  0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2018-10-31 18:00 UTC (permalink / raw)
  To: Luck, Tony
  Cc: Rafael J. Wysocki, Rafael Wysocki, qiuxu.zhuo, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

On Wed, Oct 31, 2018 at 10:57:54AM -0700, Luck, Tony wrote:
> That make Kconfig happy, but leads to a couple of link errors:
> 
>   MODPOST vmlinux.o
> drivers/edac/skx_edac.o: In function `skx_mce_check_error':
> skx_edac.c:(.text+0xab): undefined reference to `adxl_decode'
> drivers/edac/skx_edac.o: In function `skx_init':
> skx_edac.c:(.init.text+0x863): undefined reference to `adxl_get_component_names'
> make: *** [Makefile:1036: vmlinux] Error 1
> 
> Perhaps Boris is right and we do need to make ACPI_ADXL user selectable,
> and have the skylake EDAC driver "depends on ACPI_ADXL" :-(

That seems to work:

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index ffd349c12479..68e479b4d9c9 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -234,7 +234,7 @@ config EDAC_SKX
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
 	depends on ACPI_NFIT || !ACPI_NFIT # if ACPI_NFIT=m, EDAC_SKX can't be y
 	select DMI
-	select ACPI_ADXL
+	select ACPI_ADXL if ACPI
 	help
 	  Support for error detection and correction the Intel
 	  Skylake server Integrated Memory Controllers. If your
diff --git a/include/linux/adxl.h b/include/linux/adxl.h
index 2a629acb4c3f..6d04991962b0 100644
--- a/include/linux/adxl.h
+++ b/include/linux/adxl.h
@@ -7,7 +7,12 @@
 #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 false; }
+#endif
 
 #endif /* _LINUX_ADXL_H */

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-15 15:07 Luck, Tony
  0 siblings, 0 replies; 7+ messages in thread
From: Luck, Tony @ 2018-10-15 15:07 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Borislav Petkov, Rafael Wysocki, qiuxu.zhuo, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

On Mon, Oct 15, 2018 at 09:38:14AM +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.
> > +
> 
> Why is this user-selectable again?
> 
> It wasn't in the previous iteration.  Any reason to restore this?

Apparently because I can't pick the largest number from a list.
[I'm a git branch hoarder, so I have "adxl_v1", "adxl_v2", ...
 somehow I didn't pick the newest when updating with the latest
 feedback from Boris]

I'll dig through all the posted versions and make sure I have
all the feedback covered before posting a new version.

Sorry.

-Tony

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM
@ 2018-10-15  7:38 Rafael J. Wysocki
  0 siblings, 0 replies; 7+ messages in thread
From: Rafael J. Wysocki @ 2018-10-15  7:38 UTC (permalink / raw)
  To: Tony Luck
  Cc: Borislav Petkov, Rafael Wysocki, qiuxu.zhuo, aris, mchehab,
	open list:EDAC-CORE, ACPI Devel Maling List

On Fri, Oct 12, 2018 at 8:01 PM Luck, Tony <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>]
>
> Tested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
>
> ---
>
>    Boris:
>         Add verb to first line commit description
>         Don't need #ifdef CONFIG_ACPI_ADXL guard any more
>         Comment for allocation of extra element for NULL terminator
>         Merge adxl_detect() into adxl_init()
>
>    Tony:
>         Make couple of error messages more descriptive
> ---
>  drivers/acpi/Kconfig     |  10 ++
>  drivers/acpi/Makefile    |   3 +
>  drivers/acpi/acpi_adxl.c | 192 +++++++++++++++++++++++++++++++++++++++
>  include/linux/adxl.h     |  25 +++++
>  4 files changed, 230 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.
> +

Why is this user-selectable again?

It wasn't in the previous iteration.  Any reason to restore this?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-10-31 18:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-12 18:01 [v3,1/3] ACPI / adxl: Add address translation interface using ACPI DSM Luck, Tony
2018-10-15  7:38 Rafael J. Wysocki
2018-10-15 15:07 Luck, Tony
2018-10-31 18:00 Borislav Petkov
2018-10-31 18:08 Luck, Tony
2018-10-31 18:12 Luck, Tony
2018-10-31 18:16 Borislav Petkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.