[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov]

On Fri, Oct 05, 2018 at 03:25:57PM -0700, Luck, Tony wrote:
> It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
> to error reporting.  No, we aren't abandoning extlog, but it seems
> that fewer OEMs than we hoped are adopting eMCA and implementing
> the extended log in their BIOS.

Ok, but what is the difference with this DSM thing and why would OEMs
use that? They get it for free from you? Or?

> The module syning part is painful/ugly.  Making this built-in would
> be far prettier (more maintainable).

Yap.

> > Practically, this is going to be enabled on 99.9% of the boxes so making
> > it part of the core is, I guess, just as well.
> 
> So I dropped Qiuxu's code into a blender and ran it on high
> for a few minutes.  Below is an RFC patch (compiles, but
> otherwise untested) to see what things would looks like with
> a built in ACPI block with no registrations to EDAC core, or
> other stuff that is specific to the EDAC usage model.

Looks nice and clean to me.

...

> diff --git a/drivers/acpi/acpi_adxl.c b/drivers/acpi/acpi_adxl.c
> new file mode 100644
> index 000000000000..a0e026680c3d
> --- /dev/null
> +++ b/drivers/acpi/acpi_adxl.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Address translation interface via ACPI DSM.
> + * Copyright (C) 2018 Intel Corporation
> + */
> +
> +#ifdef CONFIG_ACPI_ADXL
> +#include <linux/acpi.h>
> +#include <linux/adxl.h>
> +
> +#define ADXL_REV				0x1
> +#define ADXL_IDX_GET_ADDR_PARAMS		0x1
> +#define ADXL_IDX_FORWARD_TRANSLATE	0x2
> +#define ACPI_ADXL_PATH			"\\_SB.ADXL"
> +
> +#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);
> +
> +int adxl_count;
> +EXPORT_SYMBOL_GPL(adxl_count);

That's the length of the address components adxl_component_names array?

> +char **adxl_component_names;
> +EXPORT_SYMBOL_GPL(adxl_component_names);

I guess I'm still unclear on how this is going to be used...

/me goes and looks at the initial patch.

aha, the DSM replies with a bunch of components which map to the names
and they'll get filled up by BIOS and the driver simply dumps them.
Something along those lines at least.

So you don't need count if you NULL-terminate the names array, right?

So you can simply do

	kcalloc(cnt + 1, ...

and the last element will be NULL.

Somehow exporting that naked count looks strange to me. If at all, it
should be read-only, I guess __ro_after_init or so.

Other than that minor stuff, it looks ok to me.

Thx.

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

Part 1: Add code to ACPI to make the DSM calls and export interface functions
Part 2: Some EDAC cleanup (used to be part 3 of the earlier 5 part series)
Part 3: Update skx_edac.c to use (if we have some NVDIMMs present)

Qiuxu Zhuo (2):
  EDAC, skx_edac: Clean up debugfs
  EDAC, skx_edac: Add address translation for non-volatile DIMMs

Tony Luck (1):
  ACPI / adxl: Address translation interface using ACPI DSM

 drivers/acpi/Kconfig     |  10 ++
 drivers/acpi/Makefile    |   3 +
 drivers/acpi/acpi_adxl.c | 199 +++++++++++++++++++++++++++++++
 drivers/edac/Kconfig     |   1 +
 drivers/edac/skx_edac.c  | 246 +++++++++++++++++++++++++++++++--------
 include/linux/adxl.h     |  25 ++++
 6 files changed, 435 insertions(+), 49 deletions(-)
 create mode 100644 drivers/acpi/acpi_adxl.c
 create mode 100644 include/linux/adxl.h

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

On Tue, Oct 09, 2018 at 05:14:55PM +0200, Borislav Petkov wrote:
> Not so nitty: there's no guarantee that the order and string formatting of
> all those is stable, right? I think you should state that in the driver
> so that people don't get crazy ideas of relying on any of this. It is a
> best effort and no more, I'd assume.

When we designed the interface for this call we didn't ever
want to update it again. So we went overboard with this idea
that a memory address can be expressed as a series of:

	{ component_name, value }

tuples.  So in the future if someone adds a whole bunch of new
layers between existing "normal" systems, the inetrface will
just work (TM).

Then when Qiuxu updated the EDAC driver, we found that we do
care about three components. So if the BIOS messes with the
names, or order, we'd be in a bind.

So we added module parameters for each of the three names.
If the driver doesn't find the hard-coded names, it prints
out all the ones the ADXL DSM gave us, and the user can
make the best guess on which match what the driver needs.

About to post new patch series.  See part 3.

-Tony

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov]

On Mon, Oct 08, 2018 at 09:57:06AM -0700, Luck, Tony wrote:
> Added linux-acpi list to Cc:
> 
> On Sat, Oct 06, 2018 at 10:44:56PM +0200, Borislav Petkov wrote:
> > On Fri, Oct 05, 2018 at 03:25:57PM -0700, Luck, Tony wrote:
> > > It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
> > > to error reporting.  No, we aren't abandoning extlog, but it seems
> > > that fewer OEMs than we hoped are adopting eMCA and implementing
> > > the extended log in their BIOS.
> > 
> > Ok, but what is the difference with this DSM thing and why would OEMs
> > use that? They get it for free from you? Or?
> 
> This shouldn't conflict with other BIOS "value add" code for firmware
> first mode, etc. 
> 
> Yes, they get it for free with the reference/example BIOS code.

Btw, have you seen this GHES_ASSIST thing in the ACPI 6.2 spec?

"Bit [2] - GHES_ASSIST: If set, this bit indicates that although
OSPM is responsible for directly handling the error (as expected
when FIRMWARE_FIRST is not set), system firmware reports additional
information in the context of an exception generated by the error. The
additional information is reported in a Generic Hardware Error Source
structure with a matching Related Source Id."

I'm being told this will tell you DIMM ID and FRU too...

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Rafael J. Wysocki]

On Tue, Oct 9, 2018 at 5:26 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> On Tue, Oct 09, 2018 at 12:28:54PM +0200, Rafael J. Wysocki wrote:
> > Well, it looks reasonable, one question though (below).
> >
> > > 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 wonder if the extra config option is really needed.  I guess it is useful
> > for the "tinification" people, but then can it simply depend on something
> > else we already have?
> >
> > Distros will set it to "Y" anyway.
>
> I was planning on having the EDAC driver(s) that need this do a
>
>         select ACPI_ADXL
>
> That way the tinification people are happy, and distros that include
> EDAC will set it without any other changes.
>
> > > +#define ACPI_ADXL_PATH                     "\\_SB.ADXL"
> >
> > It would be good to add a comment to say where the ADXL object is
> > defined.
>
> Can you expand on what you want here? My naive ACPI knowledge
> thought that "\\_SB.ADXL" was a "pathname" saying that the ADXL
> device is part of the "SB" (is that System Board?).
>
> Are you asking which platforms include this?

No, I'm asking about where the contents of that object are documented.
Presumably, the patch is based on some documentation, so it would be
good to give a pointer to it here, if available.

> Qiuxu has tested this. So I can re-post to linux-acpi for any
> other comments from the community.
>
> Procedure question. If you like this, can you "Ack" it and let
> Boris take it through his tree.

No, I can't ACK it at this time yet, sorry.

> The changes to the Skylake EDAC driver are dependent on this, so we need
> to have the commits apply in the right order.

No problem with that when the patch is ready.

Thanks,
Rafael

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Rafael J. Wysocki]

On Mon, Oct 8, 2018 at 6:57 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> Added linux-acpi list to Cc:
>
> On Sat, Oct 06, 2018 at 10:44:56PM +0200, Borislav Petkov wrote:
> > On Fri, Oct 05, 2018 at 03:25:57PM -0700, Luck, Tony wrote:
> > > It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
> > > to error reporting.  No, we aren't abandoning extlog, but it seems
> > > that fewer OEMs than we hoped are adopting eMCA and implementing
> > > the extended log in their BIOS.
> >
> > Ok, but what is the difference with this DSM thing and why would OEMs
> > use that? They get it for free from you? Or?
>
> This shouldn't conflict with other BIOS "value add" code for firmware
> first mode, etc.
>
> Yes, they get it for free with the reference/example BIOS code.
>
> > > So I dropped Qiuxu's code into a blender and ran it on high
> > > for a few minutes.  Below is an RFC patch (compiles, but
> > > otherwise untested) to see what things would looks like with
> > > a built in ACPI block with no registrations to EDAC core, or
> > > other stuff that is specific to the EDAC usage model.
> >
> > Looks nice and clean to me.
>
> Thanks!
>
> > > +int adxl_count;
> > > +EXPORT_SYMBOL_GPL(adxl_count);
> >
> > That's the length of the address components adxl_component_names array?
>
> Yes. But see below.
>
> > > +char **adxl_component_names;
> > > +EXPORT_SYMBOL_GPL(adxl_component_names);
> >
> > I guess I'm still unclear on how this is going to be used...
>
> Comments added in new version to save people from mailing list
> archeology.
>
> > aha, the DSM replies with a bunch of components which map to the names
> > and they'll get filled up by BIOS and the driver simply dumps them.
> > Something along those lines at least.
> >
> > So you don't need count if you NULL-terminate the names array, right?
> >
> > So you can simply do
> >
> >       kcalloc(cnt + 1, ...
> >
> > and the last element will be NULL.
>
> Good idea. Done in new version, and the EXPORT_SYMBOL_GPL(adxl_count) is
> gone.
>
> Still untested ... Qiuxu, can you try this out please.
>
> Rafael: Does this look like something you can merge (once we
> clean up any minor style stuff you find).
>
> -Tony
>
> 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..01d2b4d06430
> --- /dev/null
> +++ b/drivers/acpi/acpi_adxl.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Address translation interface via ACPI DSM.
> + * Copyright (C) 2018 Intel Corporation
> + */
> +
> +#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"
> +
> +#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 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_debug("Empty obj\n");
> +               goto out;

Why not "return NULL"?

> +       }
> +
> +       if (obj->package.count != 2) {
> +               pr_debug("Bad pkg cnt %d\n", obj->package.count);
> +               goto err;
> +       }
> +
> +       o = obj->package.elements;
> +       if (o->type != ACPI_TYPE_INTEGER) {
> +               pr_debug("Bad 1st element type %d\n", o->type);
> +               goto err;
> +       }
> +       if (o->integer.value) {
> +               pr_debug("Bad ret val %llu\n", o->integer.value);
> +               goto err;
> +       }
> +
> +       o = obj->package.elements + 1;
> +       if (o->type != ACPI_TYPE_PACKAGE) {
> +               pr_debug("Bad 2nd element type %d\n", o->type);
> +               goto err;
> +       }
> +       return obj;
> +
> +err:
> +       ACPI_FREE(obj);
> +out:
> +       return obj;

And you want to return NULL here too I think.

> +}

Thanks,
Rafael

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

On Tue, Oct 09, 2018 at 12:28:54PM +0200, Rafael J. Wysocki wrote:
> Well, it looks reasonable, one question though (below).
> 
> > 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 wonder if the extra config option is really needed.  I guess it is useful
> for the "tinification" people, but then can it simply depend on something
> else we already have?
> 
> Distros will set it to "Y" anyway.

I was planning on having the EDAC driver(s) that need this do a

	select ACPI_ADXL

That way the tinification people are happy, and distros that include
EDAC will set it without any other changes.

> > +#define ACPI_ADXL_PATH			"\\_SB.ADXL"
> 
> It would be good to add a comment to say where the ADXL object is
> defined.

Can you expand on what you want here? My naive ACPI knowledge
thought that "\\_SB.ADXL" was a "pathname" saying that the ADXL
device is part of the "SB" (is that System Board?).

Are you asking which platforms include this?


Qiuxu has tested this. So I can re-post to linux-acpi for any
other comments from the community.

Procedure question. If you like this, can you "Ack" it and let
Boris take it through his tree. The changes to the Skylake EDAC
driver are dependent on this, so we need to have the commits
apply in the right order.

Thanks

-Tony

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov]

On Mon, Oct 08, 2018 at 09:57:06AM -0700, Luck, Tony wrote:
> This shouldn't conflict with other BIOS "value add" code for firmware
> first mode, etc.

"value add", yeah. So something has been added - dunno if it is of value.

> +/**
> + * 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()

Nit: please end all your comments' sentences with a full stop.

Not so nitty: there's no guarantee that the order and string formatting of
all those is stable, right? I think you should state that in the driver
so that people don't get crazy ideas of relying on any of this. It is a
best effort and no more, I'd assume.

Also, you could point to how skx_edac is using this interface so that
people can get a good example.

> + */
> +char **adxl_get_component_names(void)
> +{
> +	return adxl_component_names;

Should this array be immutable? I.e., const char * const. So that people
don't accidentally overwrite it or poke funny chars in it.

> +}
> +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);
> +

Superfluous newline.

> +	if (!results)
> +		return -EINVAL;
> +
> +	r = results->package.elements + 1;
> +	cnt = r->package.count;
> +	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, cnt;
> +
> +	status = acpi_get_handle(NULL, path, &handle);
> +	if (ACPI_FAILURE(status)) {
> +		pr_debug("No ACPI handle for path %s\n", path);

Shouldn't all those be pr_info?

We had the same problem with einj.ko and not knowing why it wouldn't load.

> +		return false;
> +	}
> +
> +	if (!acpi_has_method(handle, "_DSM")) {
> +		pr_debug("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_debug("No ADXL DSM methods\n");
> +		return false;
> +	}
> +
> +	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
> +	if (!params) {
> +		pr_debug("Failed to get params\n");
> +		return false;
> +	}
> +
> +	p = params->package.elements + 1;
> +	cnt = p->package.count;

I guess you need to sanity-check that count - we don't trust BIOS and
you have a firmware-controlled allocation size.

> +	p = p->package.elements;
> +
> +	adxl_component_names = kcalloc(cnt + 1, sizeof(char *), GFP_KERNEL);
> +	if (!adxl_component_names) {
> +		ACPI_FREE(params);
> +		return false;
> +	}
> +
> +	for (i = 0; i < cnt; i++)
> +		adxl_component_names[i] = p[i].string.pointer;
> +
> +	return true;
> +}

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Qiuxu Zhuo]

Hi Tony,

> Still untested ... Qiuxu, can you try this out please.

   Tested this patch (with the updated skx_edac.c using the new wrapped interfaces) on Skylake (2 sockets)  + BIOS with ADXL DSM support:
   Decoding by this patch worked well (e.g. the decoded result was identical to the decoded result by original skx_edac driver for DDR4 memory for the same addresses).

Thanks!
-Qiuxu

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Rafael J. Wysocki]

On Monday, October 8, 2018 6:57:06 PM CEST Luck, Tony wrote:
> Added linux-acpi list to Cc:
> 
> On Sat, Oct 06, 2018 at 10:44:56PM +0200, Borislav Petkov wrote:
> > On Fri, Oct 05, 2018 at 03:25:57PM -0700, Luck, Tony wrote:
> > > It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
> > > to error reporting.  No, we aren't abandoning extlog, but it seems
> > > that fewer OEMs than we hoped are adopting eMCA and implementing
> > > the extended log in their BIOS.
> > 
> > Ok, but what is the difference with this DSM thing and why would OEMs
> > use that? They get it for free from you? Or?
> 
> This shouldn't conflict with other BIOS "value add" code for firmware
> first mode, etc. 
> 
> Yes, they get it for free with the reference/example BIOS code.
> 
> > > So I dropped Qiuxu's code into a blender and ran it on high
> > > for a few minutes.  Below is an RFC patch (compiles, but
> > > otherwise untested) to see what things would looks like with
> > > a built in ACPI block with no registrations to EDAC core, or
> > > other stuff that is specific to the EDAC usage model.
> > 
> > Looks nice and clean to me.
> 
> Thanks!
> 
> > > +int adxl_count;
> > > +EXPORT_SYMBOL_GPL(adxl_count);
> > 
> > That's the length of the address components adxl_component_names array?
> 
> Yes. But see below.
> 
> > > +char **adxl_component_names;
> > > +EXPORT_SYMBOL_GPL(adxl_component_names);
> > 
> > I guess I'm still unclear on how this is going to be used...
> 
> Comments added in new version to save people from mailing list
> archeology.
> 
> > aha, the DSM replies with a bunch of components which map to the names
> > and they'll get filled up by BIOS and the driver simply dumps them.
> > Something along those lines at least.
> > 
> > So you don't need count if you NULL-terminate the names array, right?
> > 
> > So you can simply do
> > 
> > 	kcalloc(cnt + 1, ...
> > 
> > and the last element will be NULL.
> 
> Good idea. Done in new version, and the EXPORT_SYMBOL_GPL(adxl_count) is
> gone.
> 
> Still untested ... Qiuxu, can you try this out please.
> 
> Rafael: Does this look like something you can merge (once we
> clean up any minor style stuff you find).

Well, it looks reasonable, one question though (below).

> 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 wonder if the extra config option is really needed.  I guess it is useful
for the "tinification" people, but then can it simply depend on something
else we already have?

Distros will set it to "Y" anyway.

>  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..01d2b4d06430
> --- /dev/null
> +++ b/drivers/acpi/acpi_adxl.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Address translation interface via ACPI DSM.
> + * Copyright (C) 2018 Intel Corporation
> + */
> +
> +#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"

It would be good to add a comment to say where the ADXL object is
defined.

> +
> +#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 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_debug("Empty obj\n");
> +		goto out;
> +	}
> +
> +	if (obj->package.count != 2) {
> +		pr_debug("Bad pkg cnt %d\n", obj->package.count);
> +		goto err;
> +	}
> +
> +	o = obj->package.elements;
> +	if (o->type != ACPI_TYPE_INTEGER) {
> +		pr_debug("Bad 1st element type %d\n", o->type);
> +		goto err;
> +	}
> +	if (o->integer.value) {
> +		pr_debug("Bad ret val %llu\n", o->integer.value);
> +		goto err;
> +	}
> +
> +	o = obj->package.elements + 1;
> +	if (o->type != ACPI_TYPE_PACKAGE) {
> +		pr_debug("Bad 2nd element type %d\n", o->type);
> +		goto err;
> +	}
> +	return obj;
> +
> +err:
> +	ACPI_FREE(obj);
> +out:
> +	return obj;
> +}
> +
> +/**
> + * 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()
> + */
> +char **adxl_get_component_names(void)
> +{
> +	return 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;
> +	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, cnt;
> +
> +	status = acpi_get_handle(NULL, path, &handle);
> +	if (ACPI_FAILURE(status)) {
> +		pr_debug("No ACPI handle for path %s\n", path);
> +		return false;
> +	}
> +
> +	if (!acpi_has_method(handle, "_DSM")) {
> +		pr_debug("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_debug("No ADXL DSM methods\n");
> +		return false;
> +	}
> +
> +	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
> +	if (!params) {
> +		pr_debug("Failed to get params\n");
> +		return false;
> +	}
> +
> +	p = params->package.elements + 1;
> +	cnt = p->package.count;
> +	p = p->package.elements;
> +
> +	adxl_component_names = kcalloc(cnt + 1, sizeof(char *), GFP_KERNEL);
> +	if (!adxl_component_names) {
> +		ACPI_FREE(params);
> +		return false;
> +	}
> +
> +	for (i = 0; i < cnt; 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..96d356a06e8b
> --- /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
> +char **adxl_get_component_names(void);
> +int adxl_decode(u64 addr, u64 component_values[]);
> +#else
> +static inline char **adxl_get_component_names(void)
> +{
> +	return NULL;
> +}
> +
> +static inline int adxl_decode(u64 addr, u64 component_values[])
> +{
> +	return -EOPNOTSUPP;
> +}
> +#endif
> +
> +#endif /* _LINUX_ADXL_H */
>

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

Added linux-acpi list to Cc:

On Sat, Oct 06, 2018 at 10:44:56PM +0200, Borislav Petkov wrote:
> On Fri, Oct 05, 2018 at 03:25:57PM -0700, Luck, Tony wrote:
> > It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
> > to error reporting.  No, we aren't abandoning extlog, but it seems
> > that fewer OEMs than we hoped are adopting eMCA and implementing
> > the extended log in their BIOS.
> 
> Ok, but what is the difference with this DSM thing and why would OEMs
> use that? They get it for free from you? Or?

This shouldn't conflict with other BIOS "value add" code for firmware
first mode, etc. 

Yes, they get it for free with the reference/example BIOS code.

> > So I dropped Qiuxu's code into a blender and ran it on high
> > for a few minutes.  Below is an RFC patch (compiles, but
> > otherwise untested) to see what things would looks like with
> > a built in ACPI block with no registrations to EDAC core, or
> > other stuff that is specific to the EDAC usage model.
> 
> Looks nice and clean to me.

Thanks!

> > +int adxl_count;
> > +EXPORT_SYMBOL_GPL(adxl_count);
> 
> That's the length of the address components adxl_component_names array?

Yes. But see below.

> > +char **adxl_component_names;
> > +EXPORT_SYMBOL_GPL(adxl_component_names);
> 
> I guess I'm still unclear on how this is going to be used...

Comments added in new version to save people from mailing list
archeology.

> aha, the DSM replies with a bunch of components which map to the names
> and they'll get filled up by BIOS and the driver simply dumps them.
> Something along those lines at least.
> 
> So you don't need count if you NULL-terminate the names array, right?
> 
> So you can simply do
> 
> 	kcalloc(cnt + 1, ...
> 
> and the last element will be NULL.

Good idea. Done in new version, and the EXPORT_SYMBOL_GPL(adxl_count) is
gone.

Still untested ... Qiuxu, can you try this out please.

Rafael: Does this look like something you can merge (once we
clean up any minor style stuff you find).

-Tony

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..01d2b4d06430
--- /dev/null
+++ b/drivers/acpi/acpi_adxl.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Address translation interface via ACPI DSM.
+ * Copyright (C) 2018 Intel Corporation
+ */
+
+#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"
+
+#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 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_debug("Empty obj\n");
+		goto out;
+	}
+
+	if (obj->package.count != 2) {
+		pr_debug("Bad pkg cnt %d\n", obj->package.count);
+		goto err;
+	}
+
+	o = obj->package.elements;
+	if (o->type != ACPI_TYPE_INTEGER) {
+		pr_debug("Bad 1st element type %d\n", o->type);
+		goto err;
+	}
+	if (o->integer.value) {
+		pr_debug("Bad ret val %llu\n", o->integer.value);
+		goto err;
+	}
+
+	o = obj->package.elements + 1;
+	if (o->type != ACPI_TYPE_PACKAGE) {
+		pr_debug("Bad 2nd element type %d\n", o->type);
+		goto err;
+	}
+	return obj;
+
+err:
+	ACPI_FREE(obj);
+out:
+	return obj;
+}
+
+/**
+ * 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()
+ */
+char **adxl_get_component_names(void)
+{
+	return 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;
+	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, cnt;
+
+	status = acpi_get_handle(NULL, path, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("No ACPI handle for path %s\n", path);
+		return false;
+	}
+
+	if (!acpi_has_method(handle, "_DSM")) {
+		pr_debug("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_debug("No ADXL DSM methods\n");
+		return false;
+	}
+
+	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
+	if (!params) {
+		pr_debug("Failed to get params\n");
+		return false;
+	}
+
+	p = params->package.elements + 1;
+	cnt = p->package.count;
+	p = p->package.elements;
+
+	adxl_component_names = kcalloc(cnt + 1, sizeof(char *), GFP_KERNEL);
+	if (!adxl_component_names) {
+		ACPI_FREE(params);
+		return false;
+	}
+
+	for (i = 0; i < cnt; 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..96d356a06e8b
--- /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
+char **adxl_get_component_names(void);
+int adxl_decode(u64 addr, u64 component_values[]);
+#else
+static inline char **adxl_get_component_names(void)
+{
+	return NULL;
+}
+
+static inline int adxl_decode(u64 addr, u64 component_values[])
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
+#endif /* _LINUX_ADXL_H */

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

Rafael: You'd become the deciding maintainer if we follow this
new direction.

On Thu, Oct 04, 2018 at 11:31:01AM +0200, Borislav Petkov wrote:
> So this sounds like drivers/acpi/acpi_extlog.c which you guys did. Is
> that thing being abandoned now too, for yet another thing?

It's like acpi_extlog.c in that it uses an ACPI DSM. Also related
to error reporting.  No, we aren't abandoning extlog, but it seems
that fewer OEMs than we hoped are adopting eMCA and implementing
the extended log in their BIOS.

> And that extlog thing is under drivers/acpi/. So even if the DSM stuff
> is *maybe* going to be part of the ACPI spec, I believe drivers/acpi/ is
> where it should go. As in: preparing to become part of ACPI :-)

That's a good argment for moving it there.

> But, there's another aspect: is this DSM interface going to be used only
> by EDAC or some other subsystems? (It seems so.)

Unclear who else might want to use this.  I spoke to an internal
validation guy (who is the person who mostly came up with this
DSM idea). He has some out-of-tree driver that can call into any
DSM. So it is unclear whether he'd want to use whatever we write.
But if there are going to be other users someday, then the interface
should be less EDAC focussed.

> Because if it is going to be used by EDAC only and EDAC drivers will
> reg/unreg with it, then I guess making it part of the EDAC core, would
> probably be more fitting. Something like drivers/edac/dsm.c and then you
> don't have to have another module which needs to sync with the *actual*
> EDAC module...

The module syning part is painful/ugly.  Making this built-in would
be far prettier (more maintainable).

> Practically, this is going to be enabled on 99.9% of the boxes so making
> it part of the core is, I guess, just as well.

So I dropped Qiuxu's code into a blender and ran it on high
for a few minutes.  Below is an RFC patch (compiles, but
otherwise untested) to see what things would looks like with
a built in ACPI block with no registrations to EDAC core, or
other stuff that is specific to the EDAC usage model.

When it finds the BIOS support this, it provides three EXPORT
objects:

1) A count of how many address components are provided.
2) A "char **" pointer to the strings for those components
3) A function that takes a system address, and fills in an
   array of component values.

The final patch to update skx_edac.c would have to be updated
for these new interface options. But that shouldn't be too hard.

Here's the patch for the new ACPI ADXL code:
---

 drivers/acpi/Kconfig     |  10 +++
 drivers/acpi/Makefile    |   3 +
 drivers/acpi/acpi_adxl.c | 156 +++++++++++++++++++++++++++++++++++++++
 include/linux/adxl.h     |  18 +++++
 4 files changed, 187 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..a0e026680c3d
--- /dev/null
+++ b/drivers/acpi/acpi_adxl.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Address translation interface via ACPI DSM.
+ * Copyright (C) 2018 Intel Corporation
+ */
+
+#ifdef CONFIG_ACPI_ADXL
+#include <linux/acpi.h>
+#include <linux/adxl.h>
+
+#define ADXL_REV				0x1
+#define ADXL_IDX_GET_ADDR_PARAMS		0x1
+#define ADXL_IDX_FORWARD_TRANSLATE	0x2
+#define ACPI_ADXL_PATH			"\\_SB.ADXL"
+
+#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);
+
+int adxl_count;
+EXPORT_SYMBOL_GPL(adxl_count);
+
+char **adxl_component_names;
+EXPORT_SYMBOL_GPL(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_REV,
+				      cmd, argv, ACPI_TYPE_PACKAGE);
+	if (!obj) {
+		pr_debug("Empty obj\n");
+		goto out;
+	}
+
+	if (obj->package.count != 2) {
+		pr_debug("Bad pkg cnt %d\n", obj->package.count);
+		goto err;
+	}
+
+	o = obj->package.elements;
+	if (o->type != ACPI_TYPE_INTEGER) {
+		pr_debug("Bad 1st element type %d\n", o->type);
+		goto err;
+	}
+	if (o->integer.value) {
+		pr_debug("Bad ret val %llu\n", o->integer.value);
+		goto err;
+	}
+
+	o = obj->package.elements + 1;
+	if (o->type != ACPI_TYPE_PACKAGE) {
+		pr_debug("Bad 2nd element type %d\n", o->type);
+		goto err;
+	}
+	return obj;
+
+err:
+	ACPI_FREE(obj);
+out:
+	return obj;
+}
+
+int adxl_decode(u64 addr, u64 component_values[])
+{
+	union acpi_object argv4[2], *results, *r;
+	int i, cnt;
+
+	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;
+	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, cnt;
+
+	status = acpi_get_handle(NULL, path, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_debug("No ACPI handle for path %s\n", path);
+		return false;
+	}
+
+	if (!acpi_has_method(handle, "_DSM")) {
+		pr_debug("No DSM method\n");
+		return false;
+	}
+
+	if (!acpi_check_dsm(handle, &adxl_guid, ADXL_REV,
+			    ADXL_IDX_GET_ADDR_PARAMS |
+			    ADXL_IDX_FORWARD_TRANSLATE)) {
+		pr_debug("No ADXL DSM methods\n");
+		return false;
+	}
+
+	params = adxl_dsm(ADXL_IDX_GET_ADDR_PARAMS, NULL);
+	if (!params) {
+		pr_debug("Failed to get params\n");
+		return false;
+	}
+
+	p = params->package.elements + 1;
+	cnt = p->package.count;
+	p = p->package.elements;
+
+	adxl_component_names = kmalloc_array(cnt, sizeof(char *), GFP_KERNEL);
+	if (!adxl_component_names) {
+		ACPI_FREE(params);
+		return false;
+	}
+
+	for (i = 0; i < cnt; i++)
+		adxl_component_names[i] = p[i].string.pointer;
+
+	adxl_count = cnt;
+
+	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..11d62bd9ca3c
--- /dev/null
+++ b/include/linux/adxl.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Address translation interface via ACPI DSM.
+ * Copyright (C) 2018 Intel Corporation
+ */
+
+#ifdef CONFIG_ACPI_ADXL
+int adxl_count;
+char **adxl_component_names;
+int adxl_decode(u64 addr, u64 component_values[]);
+#else
+#define adxl_count 0
+#define adxl_component_names NULL
+static inline int adxl_decode(u64 addr, u64 component_values[])
+{
+	return -EOPNOTSUPP;
+}
+#endif

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov]

On Wed, Sep 26, 2018 at 11:22:25AM -0700, Luck, Tony wrote:
> The plan going forward is that EDAC drivers can be much smaller. They
> just need to discover memory controller and DIMM topology and register
> a handler on the mce_decode_chain.  All the work in turning system
> addresses into DIMM addresses can be handled by a call to the BIOS
> (which knows how to do this, and the people writing it have resources
> to validate on way more configurations than I can).

So this sounds like drivers/acpi/acpi_extlog.c which you guys did. Is
that thing being abandoned now too, for yet another thing?

And that extlog thing is under drivers/acpi/. So even if the DSM stuff
is *maybe* going to be part of the ACPI spec, I believe drivers/acpi/ is
where it should go. As in: preparing to become part of ACPI :-)

But, there's another aspect: is this DSM interface going to be used only
by EDAC or some other subsystems? (It seems so.)

Because if it is going to be used by EDAC only and EDAC drivers will
reg/unreg with it, then I guess making it part of the EDAC core, would
probably be more fitting. Something like drivers/edac/dsm.c and then you
don't have to have another module which needs to sync with the *actual*
EDAC module...

Practically, this is going to be enabled on 99.9% of the boxes so making
it part of the core is, I guess, just as well.

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

> We can put it someplace else if you have a better suggestion.

Any thoughts on this?

I'll note that other users of ACPI DSM calls are scattered around the kernel in the drivers that use them:

$ git grep -l acpi_evaluate_dsm
drivers/acpi/acpi_extlog.c
drivers/acpi/nfit/core.c
drivers/acpi/sleep.c
drivers/acpi/utils.c
drivers/acpi/x86/apple.c
drivers/char/tpm/tpm_crb.c
drivers/char/tpm/tpm_ppi.c
drivers/gpu/drm/i915/intel_acpi.c
drivers/gpu/drm/nouveau/nouveau_acpi.c
drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
drivers/hid/i2c-hid/i2c-hid.c
drivers/iommu/dmar.c
drivers/mmc/host/sdhci-acpi.c
drivers/mmc/host/sdhci-pci-core.c
drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c
drivers/pci/pci-acpi.c
drivers/pci/pci-label.c
drivers/platform/x86/intel-hid.c
drivers/usb/dwc3/dwc3-pci.c
drivers/usb/host/xhci-pci.c
drivers/usb/typec/typec_wcove.c
drivers/usb/typec/ucsi/ucsi_acpi.c
include/acpi/acpi_bus.h
include/linux/acpi.h
sound/soc/intel/skylake/skl-nhlt.c

We originally coded the DSM bits directly into the driver. But during
internal review it was obvious that we'd be copy/pasting that code
into future drivers, so better to somehow separate it from the driver.

The registration/callback methods we employed in this patch series
aren't required. Happy to adopt a cleaner/simpler solution.

-Tony

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

On Wed, Sep 26, 2018 at 07:33:14PM +0200, Borislav Petkov wrote:
> + Rafael.
> 
> On Mon, Sep 24, 2018 at 01:16:12PM -0700, Tony Luck wrote:
> > From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > 
> > Some machines provide BIOS ACPI DSM (Device Specific Method)
> > to translate memory system address to DRAM address (socket, memory
> > controller, channel, rank, bank, row, column). This patch wraps
> > the DSM methods, and registers them to EDAC core.
> 
> Why?
> 
> Why isn't this part of the ACPI core and why isn't it exporting
> proper interfaces for EDAC to use?

This isn't part of ACPI spec. Just a device specific method available
to the OS. Someday, after we clean up the rough edges found by using it,
we might submit it for inclusion in the ACPI spec. But we are a long way
from that point.

> This looks like a bunch of ACPI code outside of ACPI. And it is grafted
> on the EDAC core with an ops reg/dereg thing. Ewww.

We can put it someplace else if you have a better suggestion.

> Can we please design this properly instead of just slapping it where it
> sticks?

Sure.  I should have posted it as [RFC] rather than [PATCH].

The plan going forward is that EDAC drivers can be much smaller. They
just need to discover memory controller and DIMM topology and register
a handler on the mce_decode_chain.  All the work in turning system
addresses into DIMM addresses can be handled by a call to the BIOS
(which knows how to do this, and the people writing it have resources
to validate on way more configurations than I can).


-Tony

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Borislav Petkov]

+ Rafael.

On Mon, Sep 24, 2018 at 01:16:12PM -0700, Tony Luck wrote:
> From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> 
> Some machines provide BIOS ACPI DSM (Device Specific Method)
> to translate memory system address to DRAM address (socket, memory
> controller, channel, rank, bank, row, column). This patch wraps
> the DSM methods, and registers them to EDAC core.

Why?

Why isn't this part of the ACPI core and why isn't it exporting
proper interfaces for EDAC to use?

This looks like a bunch of ACPI code outside of ACPI. And it is grafted
on the EDAC core with an ops reg/dereg thing. Ewww.

Can we please design this properly instead of just slapping it where it
sticks?

Thx.

[4/5] EDAC, dsm_edac: Wrap ACPI DSM methods for address translation

[Luck, Tony]

From: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Some machines provide BIOS ACPI DSM (Device Specific Method)
to translate memory system address to DRAM address (socket, memory
controller, channel, rank, bank, row, column). This patch wraps
the DSM methods, and registers them to EDAC core.

Specification for this translation method is at:

	https://cdrdv2.intel.com/v1/dl/getContent/603354

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

The specification is currently behind a click-through license. I'll see
if I can get that fixed.

 drivers/edac/Kconfig    |   9 ++
 drivers/edac/Makefile   |   1 +
 drivers/edac/dsm_edac.c | 223 ++++++++++++++++++++++++++++++++++++++++
 drivers/edac/edac_mc.c  |  36 +++++++
 drivers/edac/edac_mc.h  |  64 ++++++++++++
 5 files changed, 333 insertions(+)
 create mode 100644 drivers/edac/dsm_edac.c

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 57304b2e989f..1b2e0ae6158f 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -222,6 +222,15 @@ config EDAC_I7300
 	  Support for error detection and correction the Intel
 	  Clarksboro MCH (Intel 7300 chipset).
 
+config EDAC_DSM
+	tristate "Address translation interface via ACPI DSM"
+	depends on X86_64 && ACPI
+	help
+	  Some of machines provide BIOS ACPI DSM (Device Specific Method)
+	  to translate memory system address to DRAM address (socket, memory
+	  controller, channel, rank, bank, row, column). By enabling this
+	  option, the wrapped DSM methods are registered to EDAC core.
+
 config EDAC_SBRIDGE
 	tristate "Intel Sandy-Bridge/Ivy-Bridge/Haswell Integrated MC"
 	depends on PCI && X86_64 && X86_MCE_INTEL && PCI_MMCONFIG
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 02b43a7d8c3e..0238c9127730 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_EDAC_I5100)		+= i5100_edac.o
 obj-$(CONFIG_EDAC_I5400)		+= i5400_edac.o
 obj-$(CONFIG_EDAC_I7300)		+= i7300_edac.o
 obj-$(CONFIG_EDAC_I7CORE)		+= i7core_edac.o
+obj-$(CONFIG_EDAC_DSM)			+= dsm_edac.o
 obj-$(CONFIG_EDAC_SBRIDGE)		+= sb_edac.o
 obj-$(CONFIG_EDAC_SKX)			+= skx_edac.o
 obj-$(CONFIG_EDAC_PND2)			+= pnd2_edac.o
diff --git a/drivers/edac/dsm_edac.c b/drivers/edac/dsm_edac.c
new file mode 100644
index 000000000000..ae18d3d1c438
--- /dev/null
+++ b/drivers/edac/dsm_edac.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Address translation interface via ACPI DSM.
+ * Copyright (C) 2018 Intel Corporation
+ */
+
+#include <linux/acpi.h>
+#include "edac_module.h"
+#include "edac_mc.h"
+
+#define DSM_REV				0x1
+#define DSM_IDX_GET_ADDR_PARAMS		0x1
+#define DSM_IDX_FORWARD_TRANSLATE	0x2
+#define ACPI_ADXL_PATH			"\\_SB.ADXL"
+#define MAX_PARAMS			31
+
+#define dsm_printk(level, fmt, arg...)	\
+	edac_printk(level, "dsm", fmt, ##arg)
+
+static acpi_handle handle;
+static union acpi_object *params;
+static const guid_t dsm_guid = GUID_INIT(0xAA3C050A, 0x7EA4, 0x4C1F, 0xAF, 0xDA,
+					 0x12, 0x67, 0xDF, 0xD3, 0xD4, 0x8D);
+
+static int dsm_adxl_check_obj(union acpi_object *obj)
+{
+	union acpi_object *o;
+
+	if (!obj) {
+		dsm_printk(KERN_ERR, "Empty obj\n");
+		return -ENODEV;
+	}
+
+	if (obj->package.count != 2) {
+		dsm_printk(KERN_ERR, "Bad pkg cnt %d\n", obj->package.count);
+		return -EINVAL;
+	}
+
+	o = obj->package.elements;
+	if (o->type != ACPI_TYPE_INTEGER) {
+		dsm_printk(KERN_ERR, "Bad 1st element type %d\n", o->type);
+		return -EINVAL;
+	}
+	if (o->integer.value) {
+		dsm_printk(KERN_ERR, "Bad ret val %llu\n", o->integer.value);
+		return -EINVAL;
+	}
+
+	o = obj->package.elements + 1;
+	if (o->type != ACPI_TYPE_PACKAGE) {
+		dsm_printk(KERN_ERR, "Bad 2nd element type %d\n", o->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static union acpi_object *_dsm_adxl_get_params(void)
+{
+	union acpi_object *obj;
+
+	obj = acpi_evaluate_dsm_typed(handle, &dsm_guid, DSM_REV,
+				      DSM_IDX_GET_ADDR_PARAMS,
+				      NULL, ACPI_TYPE_PACKAGE);
+
+	if (dsm_adxl_check_obj(obj)) {
+		ACPI_FREE(obj);
+		return NULL;
+	}
+
+	return obj;
+}
+
+static int dsm_adxl_check(char component_names[][32],
+			  int component_indices[])
+{
+	union acpi_object *p;
+	int i, j, cnt;
+
+	p = params->package.elements + 1;
+	cnt = min_t(u32, p->package.count, MAX_PARAMS);
+	p = p->package.elements;
+
+	for (i = 0; i < MAX_PARAMS && component_names[i][0]; i++) {
+		for (j = 0; j < cnt; j++) {
+			if (!strcmp(component_names[i], p[j].string.pointer)) {
+				component_indices[i] = j;
+				break;
+			}
+		}
+		if (j == cnt)
+			goto err;
+	}
+	component_indices[i] = -1;
+
+	return 0;
+err:
+	dsm_printk(KERN_ERR, "'%s' is not matched from DSM parameters:",
+		   component_names[i]);
+	for (j = 0; j < cnt; j++)
+		dsm_printk(KERN_CONT, " %s", p[j].string.pointer);
+	dsm_printk(KERN_CONT, "\n");
+
+	return -ENODEV;
+}
+
+static int dsm_adxl_decode(u64 addr, char *msg, int msglen,
+			   int component_indices[], ...)
+
+{
+	union acpi_object argv4[2], *results, *r, *p;
+	int i, rc, cnt, len = 0;
+	va_list ap;
+	u64 *valp;
+
+	va_start(ap, component_indices);
+	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 = acpi_evaluate_dsm_typed(handle, &dsm_guid, DSM_REV,
+					  DSM_IDX_FORWARD_TRANSLATE, argv4,
+					  ACPI_TYPE_PACKAGE);
+
+	rc = dsm_adxl_check_obj(results);
+	if (rc)
+		goto end;
+
+	r = results->package.elements + 1;
+	p = params->package.elements + 1;
+	cnt = min_t(u32, min(p->package.count, r->package.count), MAX_PARAMS);
+	r = r->package.elements;
+	p = p->package.elements;
+
+	for (i = 0; i < MAX_PARAMS && component_indices[i] != -1; i++) {
+		valp = va_arg(ap, u64 *);
+		*valp = r[component_indices[i]].integer.value;
+	}
+
+	for (i = 0; i < cnt; i++) {
+		if (r[i].integer.value == -1)
+			continue;
+
+		len += snprintf(msg + len, msglen - len, " %s:0x%llx",
+				p[i].string.pointer, r[i].integer.value);
+
+		if (msglen - len <= 0)
+			break;
+	}
+
+end:
+	ACPI_FREE(results);
+	va_end(ap);
+	return rc;
+}
+
+static bool dsm_adxl_detect(void)
+{
+	char *path = ACPI_ADXL_PATH;
+	acpi_status status;
+
+	status = acpi_get_handle(NULL, path, &handle);
+	if (ACPI_FAILURE(status)) {
+		dsm_printk(KERN_ERR, "No ACPI handle for path %s\n", path);
+		return false;
+	}
+
+	if (!acpi_has_method(handle, "_DSM")) {
+		dsm_printk(KERN_ERR, "No DSM method\n");
+		return false;
+	}
+
+	if (!acpi_check_dsm(handle, &dsm_guid, DSM_REV,
+			    DSM_IDX_GET_ADDR_PARAMS |
+			    DSM_IDX_FORWARD_TRANSLATE)) {
+		dsm_printk(KERN_ERR, "No ADXL DSM methods\n");
+		return false;
+	}
+
+	params = _dsm_adxl_get_params();
+	if (!params) {
+		dsm_printk(KERN_ERR, "Failed to get params\n");
+		return false;
+	}
+
+	return true;
+}
+
+static const struct edac_dsm_operations dsm_ops = {
+	.check	= dsm_adxl_check,
+	.decode	= dsm_adxl_decode,
+	.owner	= THIS_MODULE,
+};
+
+static int __init dsm_init(void)
+{
+	edac_dbg(2, "\n");
+
+	if (!dsm_adxl_detect())
+		return -ENODEV;
+
+	edac_dsm_register_ops(&dsm_ops);
+
+	return 0;
+}
+
+static void __exit dsm_exit(void)
+{
+	edac_dbg(2, "\n");
+
+	edac_dsm_unregister_ops(&dsm_ops);
+
+	ACPI_FREE(params);
+}
+
+module_init(dsm_init);
+module_exit(dsm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Qiuxu Zhuo");
+MODULE_DESCRIPTION("Address translation interface via ACPI DSM");
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 7d3edd713932..ba27feafc976 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -57,6 +57,8 @@ static const char *edac_mc_owner;
 
 static struct bus_type mc_bus[EDAC_MAX_MCS];
 
+static const struct edac_dsm_operations *edac_dsm_ops;
+
 int edac_get_report_status(void)
 {
 	return edac_report;
@@ -1259,3 +1261,37 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 	edac_raw_mc_handle_error(type, mci, e);
 }
 EXPORT_SYMBOL_GPL(edac_mc_handle_error);
+
+const struct edac_dsm_operations *edac_dsm_get_ops(void)
+{
+	if (edac_dsm_ops && try_module_get(edac_dsm_ops->owner))
+		return edac_dsm_ops;
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(edac_dsm_get_ops);
+
+void edac_dsm_put_ops(const struct edac_dsm_operations *ops)
+{
+	if (ops)
+		module_put(ops->owner);
+}
+EXPORT_SYMBOL_GPL(edac_dsm_put_ops);
+
+int edac_dsm_register_ops(const struct edac_dsm_operations *ops)
+{
+	if (edac_dsm_ops)
+		return -EEXIST;
+
+	edac_dsm_ops = ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(edac_dsm_register_ops);
+
+void edac_dsm_unregister_ops(const struct edac_dsm_operations *ops)
+{
+	if (edac_dsm_ops == ops)
+		edac_dsm_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(edac_dsm_unregister_ops);
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 4165e15995ad..f2183d06de77 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -254,6 +254,70 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
 			  const char *msg,
 			  const char *other_detail);
 
+/*
+ * edac DSM APIs
+ */
+
+struct edac_dsm_operations {
+	struct module *owner;
+	/*
+	 * check() - Check whether the speficified address component names
+	 *           are supported.
+	 *
+	 * @component_names:	List of address component name
+	 *			strings to be checked.
+	 * @component_indices:  Array of address component name indices
+	 *			to be filled.
+	 *
+	 * Returns: 0 on success, or an error code on failure.
+	 */
+	int (*check)(char component_names[][32], int component_indices[]);
+	/*
+	 * decode() - Decode memory system address to DRAM address
+	 *	      (socket, MC, channel, rank, bank, row, column).
+	 * @addr:		Memory system address
+	 * @component_indices:	Array of address component name
+	 *			indices from check()
+	 * @msg:		String holds detail of decoded DRAM addresses
+	 * @msglen:		Length of the string
+	 *
+	 * Returns: 0 on success, or an error code on failure.
+	 */
+	int (*decode)(u64 addr, char *msg, int msglen,
+		      int component_indices[], ...);
+};
+
+/**
+ * edac_dsm_get_ops() - Get EDAC DSM operations and increase the
+ *			refcount of module.
+ *
+ * Returns: On success, return a pointer to struct edac_dsm_operations,
+ *	%NULL otherwise
+ */
+const struct edac_dsm_operations *edac_dsm_get_ops(void);
+
+/**
+ * edac_dsm_put_ops() - Put EDAC DSM operations and decrease the
+ *			refcount of module.
+ */
+void edac_dsm_put_ops(const struct edac_dsm_operations *ops);
+
+/**
+ * edac_dsm_register_ops() - Register EDAC DSM operations to EDAC core.
+ *
+ * @ops: A struct edac_dsm_operations pointer
+ *
+ * Returns: On success, return 0, an error code otherwise
+ */
+int edac_dsm_register_ops(const struct edac_dsm_operations *ops);
+
+/**
+ * edac_dsm_unregister_ops() - Unregister EDAC DSM operations from EDAC core.
+ *
+ * @ops: A struct edac_dsm_operations pointer
+ */
+void edac_dsm_unregister_ops(const struct edac_dsm_operations *ops);
+
 /*
  * edac misc APIs
  */