From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Keith Busch <keith.busch@intel.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
Linux Memory Management List <linux-mm@kvack.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Rafael Wysocki <rafael@kernel.org>,
Dave Hansen <dave.hansen@intel.com>,
Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCHv4 03/13] acpi/hmat: Parse and report heterogeneous memory
Date: Thu, 17 Jan 2019 12:00:53 +0100 [thread overview]
Message-ID: <CAJZ5v0hEg3V7FoE6arwTTodVQ4uUZNLpwdOpjzh7PjqB3jguGw@mail.gmail.com> (raw)
In-Reply-To: <20190116175804.30196-4-keith.busch@intel.com>
On Wed, Jan 16, 2019 at 6:59 PM Keith Busch <keith.busch@intel.com> wrote:
>
> Systems may provide different memory types and export this information
> in the ACPI Heterogeneous Memory Attribute Table (HMAT). Parse these
> tables provided by the platform and report the memory access and caching
> attributes.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> drivers/acpi/Kconfig | 1 +
> drivers/acpi/Makefile | 1 +
> drivers/acpi/hmat/Kconfig | 8 ++
> drivers/acpi/hmat/Makefile | 1 +
> drivers/acpi/hmat/hmat.c | 180 +++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 191 insertions(+)
> create mode 100644 drivers/acpi/hmat/Kconfig
> create mode 100644 drivers/acpi/hmat/Makefile
> create mode 100644 drivers/acpi/hmat/hmat.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 90ff0a47c12e..b377f970adfd 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -465,6 +465,7 @@ config ACPI_REDUCED_HARDWARE_ONLY
> If you are unsure what to do, do not enable this option.
>
> source "drivers/acpi/nfit/Kconfig"
> +source "drivers/acpi/hmat/Kconfig"
>
> source "drivers/acpi/apei/Kconfig"
> source "drivers/acpi/dptf/Kconfig"
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 7c6afc111d76..bff8fbe5a6ab 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_ACPI_PROCESSOR) += processor.o
> obj-$(CONFIG_ACPI) += container.o
> obj-$(CONFIG_ACPI_THERMAL) += thermal.o
> obj-$(CONFIG_ACPI_NFIT) += nfit/
> +obj-$(CONFIG_ACPI_HMAT) += hmat/
Yes, I prefer it to go into a separate directory.
Who do you want to maintain it, me or Dan?
> obj-$(CONFIG_ACPI) += acpi_memhotplug.o
> obj-$(CONFIG_ACPI_HOTPLUG_IOAPIC) += ioapic.o
> obj-$(CONFIG_ACPI_BATTERY) += battery.o
> diff --git a/drivers/acpi/hmat/Kconfig b/drivers/acpi/hmat/Kconfig
> new file mode 100644
> index 000000000000..a4034d37a311
> --- /dev/null
> +++ b/drivers/acpi/hmat/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config ACPI_HMAT
> + bool "ACPI Heterogeneous Memory Attribute Table Support"
> + depends on ACPI_NUMA
> + help
> + Parses representation of the ACPI Heterogeneous Memory Attributes
> + Table (HMAT) and set the memory node relationships and access
> + attributes.
What about:
"If set, this option causes the kernel to set the memory NUMA node
relationships and access attributes in accordance with ACPI HMAT
(Heterogeneous Memory Attributes Table)."
> diff --git a/drivers/acpi/hmat/Makefile b/drivers/acpi/hmat/Makefile
> new file mode 100644
> index 000000000000..e909051d3d00
> --- /dev/null
> +++ b/drivers/acpi/hmat/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ACPI_HMAT) := hmat.o
> diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
> new file mode 100644
> index 000000000000..833a783868d5
> --- /dev/null
> +++ b/drivers/acpi/hmat/hmat.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Heterogeneous Memory Attributes Table (HMAT) representation
> + *
> + * Copyright (c) 2018, Intel Corporation.
Can you put a comment describing the code somewhat in here?
> + */
> +
> +#include <acpi/acpi_numa.h>
> +#include <linux/acpi.h>
> +#include <linux/bitops.h>
> +#include <linux/cpu.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/node.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
Are all of the headers above really necessary to build the code?
> +
> +static __init const char *hmat_data_type(u8 type)
> +{
> + switch (type) {
> + case ACPI_HMAT_ACCESS_LATENCY:
> + return "Access Latency";
> + case ACPI_HMAT_READ_LATENCY:
> + return "Read Latency";
> + case ACPI_HMAT_WRITE_LATENCY:
> + return "Write Latency";
> + case ACPI_HMAT_ACCESS_BANDWIDTH:
> + return "Access Bandwidth";
> + case ACPI_HMAT_READ_BANDWIDTH:
> + return "Read Bandwidth";
> + case ACPI_HMAT_WRITE_BANDWIDTH:
> + return "Write Bandwidth";
> + default:
> + return "Reserved";
> + };
> +}
> +
> +static __init const char *hmat_data_type_suffix(u8 type)
> +{
> + switch (type) {
> + case ACPI_HMAT_ACCESS_LATENCY:
> + case ACPI_HMAT_READ_LATENCY:
> + case ACPI_HMAT_WRITE_LATENCY:
> + return " nsec";
> + case ACPI_HMAT_ACCESS_BANDWIDTH:
> + case ACPI_HMAT_READ_BANDWIDTH:
> + case ACPI_HMAT_WRITE_BANDWIDTH:
> + return " MB/s";
> + default:
> + return "";
> + };
> +}
> +
> +static __init int hmat_parse_locality(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_hmat_locality *loc = (void *)header;
> + unsigned int init, targ, total_size, ipds, tpds;
> + u32 *inits, *targs, value;
> + u16 *entries;
> + u8 type;
> +
> + if (loc->header.length < sizeof(*loc)) {
> + pr_err("HMAT: Unexpected locality header length: %d\n",
> + loc->header.length);
Why pr_err()? Is the error really high-prio?
Same below.
> + return -EINVAL;
> + }
> +
> + type = loc->data_type;
> + ipds = loc->number_of_initiator_Pds;
> + tpds = loc->number_of_target_Pds;
> + total_size = sizeof(*loc) + sizeof(*entries) * ipds * tpds +
> + sizeof(*inits) * ipds + sizeof(*targs) * tpds;
> + if (loc->header.length < total_size) {
> + pr_err("HMAT: Unexpected locality header length:%d, minimum required:%d\n",
> + loc->header.length, total_size);
> + return -EINVAL;
> + }
> +
> + pr_info("HMAT: Locality: Flags:%02x Type:%s Initiator Domains:%d Target Domains:%d Base:%lld\n",
> + loc->flags, hmat_data_type(type), ipds, tpds,
> + loc->entry_base_unit);
> +
> + inits = (u32 *)(loc + 1);
> + targs = &inits[ipds];
> + entries = (u16 *)(&targs[tpds]);
> + for (targ = 0; targ < tpds; targ++) {
> + for (init = 0; init < ipds; init++) {
> + value = entries[init * tpds + targ];
> + value = (value * loc->entry_base_unit) / 10;
> + pr_info(" Initiator-Target[%d-%d]:%d%s\n",
> + inits[init], targs[targ], value,
> + hmat_data_type_suffix(type));
> + }
> + }
> + return 0;
> +}
The format and meaning of what is printed into the log should be
documented somewhere IMO.
Of course, that applies to the functions below as well.
> +
> +static __init int hmat_parse_cache(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_hmat_cache *cache = (void *)header;
> + u32 attrs;
> +
> + if (cache->header.length < sizeof(*cache)) {
> + pr_err("HMAT: Unexpected cache header length: %d\n",
> + cache->header.length);
> + return -EINVAL;
> + }
> +
> + attrs = cache->cache_attributes;
> + pr_info("HMAT: Cache: Domain:%d Size:%llu Attrs:%08x SMBIOS Handles:%d\n",
> + cache->memory_PD, cache->cache_size, attrs,
> + cache->number_of_SMBIOShandles);
> +
> + return 0;
> +}
> +
> +static int __init hmat_parse_address_range(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_hmat_address_range *spa = (void *)header;
> +
> + if (spa->header.length != sizeof(*spa)) {
> + pr_err("HMAT: Unexpected address range header length: %d\n",
> + spa->header.length);
> + return -EINVAL;
> + }
> + pr_info("HMAT: Memory (%#llx length %#llx) Flags:%04x Processor Domain:%d Memory Domain:%d\n",
> + spa->physical_address_base, spa->physical_address_length,
> + spa->flags, spa->processor_PD, spa->memory_PD);
> + return 0;
> +}
> +
> +static int __init hmat_parse_subtable(union acpi_subtable_headers *header,
> + const unsigned long end)
> +{
> + struct acpi_hmat_structure *hdr = (void *)header;
> +
> + if (!hdr)
> + return -EINVAL;
> +
> + switch (hdr->type) {
> + case ACPI_HMAT_TYPE_ADDRESS_RANGE:
> + return hmat_parse_address_range(header, end);
> + case ACPI_HMAT_TYPE_LOCALITY:
> + return hmat_parse_locality(header, end);
> + case ACPI_HMAT_TYPE_CACHE:
> + return hmat_parse_cache(header, end);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static __init int hmat_init(void)
> +{
> + struct acpi_table_header *tbl;
> + enum acpi_hmat_type i;
> + acpi_status status;
> +
> + if (srat_disabled())
> + return 0;
> +
> + status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + for (i = ACPI_HMAT_TYPE_ADDRESS_RANGE; i < ACPI_HMAT_TYPE_RESERVED; i++) {
> + if (acpi_table_parse_entries(ACPI_SIG_HMAT,
> + sizeof(struct acpi_table_hmat), i,
> + hmat_parse_subtable, 0) < 0)
> + goto out_put;
> + }
> +out_put:
> + acpi_put_table(tbl);
> + return 0;
> +}
> +subsys_initcall(hmat_init);
> --
It looks like this particular patch only causes some extra messages to
be printed into the log, no attributes setting etc yet.
I would like the changelog to mention that.
next prev parent reply other threads:[~2019-01-17 11:01 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-16 17:57 [PATCHv4 00/13] Heterogeneuos memory node attributes Keith Busch
2019-01-16 17:57 ` [PATCHv4 01/13] acpi: Create subtable parsing infrastructure Keith Busch
2019-01-16 17:57 ` [PATCHv4 02/13] acpi: Add HMAT to generic parsing tables Keith Busch
2019-01-16 17:57 ` [PATCHv4 03/13] acpi/hmat: Parse and report heterogeneous memory Keith Busch
2019-01-17 11:00 ` Rafael J. Wysocki [this message]
2019-01-16 17:57 ` [PATCHv4 04/13] node: Link memory nodes to their compute nodes Keith Busch
2019-01-17 11:26 ` Rafael J. Wysocki
2019-01-16 17:57 ` [PATCHv4 05/13] Documentation/ABI: Add new node sysfs attributes Keith Busch
2019-01-17 11:41 ` Rafael J. Wysocki
2019-01-18 20:42 ` Keith Busch
2019-01-18 21:08 ` Dan Williams
2019-01-19 9:01 ` Greg Kroah-Hartman
2019-01-19 16:56 ` Dan Williams
2019-01-20 16:19 ` Rafael J. Wysocki
2019-01-20 17:34 ` Dan Williams
2019-01-21 9:54 ` Rafael J. Wysocki
2019-01-20 16:16 ` Rafael J. Wysocki
2019-01-22 16:36 ` Keith Busch
2019-01-22 16:51 ` Rafael J. Wysocki
2019-01-22 16:54 ` Rafael J. Wysocki
2019-01-18 11:21 ` Jonathan Cameron
2019-01-18 16:35 ` Dan Williams
2019-01-16 17:57 ` [PATCHv4 06/13] acpi/hmat: Register processor domain to its memory Keith Busch
2019-01-17 12:11 ` Rafael J. Wysocki
2019-01-17 17:01 ` Dan Williams
2019-01-16 17:57 ` [PATCHv4 07/13] node: Add heterogenous memory access attributes Keith Busch
2019-01-17 15:03 ` Rafael J. Wysocki
2019-01-17 15:41 ` Greg Kroah-Hartman
2019-01-16 17:57 ` [PATCHv4 08/13] Documentation/ABI: Add node performance attributes Keith Busch
2019-01-17 15:09 ` Rafael J. Wysocki
2019-01-16 17:58 ` [PATCHv4 09/13] acpi/hmat: Register " Keith Busch
2019-01-17 15:21 ` Rafael J. Wysocki
2019-01-16 17:58 ` [PATCHv4 10/13] node: Add memory caching attributes Keith Busch
2019-01-17 16:00 ` Rafael J. Wysocki
2019-02-09 8:20 ` Brice Goglin
2019-02-10 17:19 ` Jonathan Cameron
2019-02-11 15:23 ` Keith Busch
2019-02-12 8:11 ` Brice Goglin
2019-02-12 8:49 ` Jonathan Cameron
2019-02-12 17:31 ` Keith Busch
2019-01-16 17:58 ` [PATCHv4 11/13] Documentation/ABI: Add node cache attributes Keith Busch
2019-01-17 16:25 ` Rafael J. Wysocki
2019-01-16 17:58 ` [PATCHv4 12/13] acpi/hmat: Register memory side " Keith Busch
2019-01-17 17:42 ` Rafael J. Wysocki
2019-01-16 17:58 ` [PATCHv4 13/13] doc/mm: New documentation for memory performance Keith Busch
2019-01-17 12:58 ` [PATCHv4 00/13] Heterogeneuos memory node attributes Balbir Singh
2019-01-17 15:44 ` Keith Busch
2019-01-18 13:16 ` Balbir Singh
2019-01-17 18:18 ` Jonathan Cameron
2019-01-17 19:47 ` Keith Busch
2019-01-18 11:12 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJZ5v0hEg3V7FoE6arwTTodVQ4uUZNLpwdOpjzh7PjqB3jguGw@mail.gmail.com \
--to=rafael@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=keith.busch@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).