| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240422174325.000058ba@Huawei.com>
Date: Mon, 22 Apr 2024 17:43:25 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Richter <rrichter@....com>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, Dave Hansen
<dave.hansen@...ux.intel.com>, Dan Williams <dan.j.williams@...el.com>,
Alison Schofield <alison.schofield@...el.com>, <linux-acpi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Len Brown
<lenb@...nel.org>
Subject: Re: [PATCH v3 2/5] ACPI/NUMA: Print CXL Early Discovery Table
(CEDT)
On Fri, 19 Apr 2024 16:02:00 +0200
Robert Richter <rrichter@....com> wrote:
> The CEDT contains similar entries as the SRAT. For diagnostic reasons
> print the CEDT same style as the SRAT.
>
> Adding also a pr_info() when successfully adding a CFMWS memory range.
>
> Suggested-by: Alison Schofield <alison.schofield@...el.com> # CEDT node info
> Signed-off-by: Robert Richter <rrichter@....com>
Hi Robert,
Comments inline,
Jonathan
> ---
> drivers/acpi/numa/srat.c | 122 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index e45e64993c50..43417b4920da 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -300,6 +300,114 @@ acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma)
> return -EINVAL;
> }
>
> +static int __init
> +__acpi_table_print_cedt_entry(union acpi_subtable_headers *__header,
> + void *arg, const unsigned long table_end)
> +{
> + struct acpi_cedt_header *header = (struct acpi_cedt_header *)__header;
As below. There is very little shred by the different types in here, so
I'd break it up into separate functions and just call the relevant
one.
> +
> + switch (header->type) {
> + case ACPI_CEDT_TYPE_CHBS:
> + {
> + struct acpi_cedt_chbs *p =
> + (struct acpi_cedt_chbs *)header;
> +
> + if (header->length < sizeof(struct acpi_cedt_chbs)) {
sizeof(*p) perhaps?
> + pr_warn("CEDT: unsupported CHBS entry: size %d\n",
> + header->length);
> + break;
> + }
> +
> + pr_debug("CEDT: CHBS (0x%llx length 0x%llx uid %lu) %s (%d)\n",
> + (unsigned long long)p->base,
> + (unsigned long long)p->length,
> + (unsigned long)p->uid,
> + (p->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) ?
> + "cxl11" :
> + (p->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL20) ?
> + "cxl20" :
> + "unsupported version",
> + p->cxl_version);
> + }
> + break;
> + case ACPI_CEDT_TYPE_CFMWS:
> + {
> + struct acpi_cedt_cfmws *p =
> + (struct acpi_cedt_cfmws *)header;
> + int eiw_to_ways[] = {1, 2, 4, 8, 16, 3, 6, 12};
> + int targets = -1;
> +
> + if (header->length < sizeof(struct acpi_cedt_cfmws)) {
sizeof(*p) perhaps?
> + pr_warn("CEDT: unsupported CFMWS entry: size %d\n",
> + header->length);
> + break;
> + }
> +
> + if (p->interleave_ways < ARRAY_SIZE(eiw_to_ways))
> + targets = eiw_to_ways[p->interleave_ways];
> + if (header->length < struct_size(
> + p, interleave_targets, targets))
> + targets = -1;
Not a warning?
> +
> + pr_debug("CEDT: CFMWS (0x%llx length 0x%llx) with %d target%s",
> + (unsigned long long)p->base_hpa,
> + (unsigned long long)p->window_size,
> + targets, targets > 1 ? "s" : "");
> + for (int i = 0; i < targets; i++)
> + pr_cont("%s%lu", i ? ", " : " (",
> + (unsigned long)p->interleave_targets[i]);
This seems odd. I don't think there is a good way to do combined pr_debug() and pr_cont()
There was a discussion about this a few years ago...
> + pr_cont("%s%s%s%s%s%s\n",
> + targets > 0 ? ")" : "",
> + (p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) ?
> + " type2" : "",
> + (p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) ?
> + " type3" : "",
> + (p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) ?
> + " volatile" : "",
> + (p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM) ?
> + " pmem" : "",
> + (p->restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED) ?
> + " fixed" : "");
> + }
> + break;
> + case ACPI_CEDT_TYPE_CXIMS:
> + {
> + struct acpi_cedt_cxims *p =
> + (struct acpi_cedt_cxims *)header;
> +
> + if (header->length < sizeof(struct acpi_cedt_cxims)) {
sizeof(*p) perhaps?
> + pr_warn("CEDT: unsupported CXIMS entry: size %d\n",
> + header->length);
> + break;
I'd go with direct returns in these to make the code flow easier to read.
> + }
> +
> + pr_debug("CEDT: CXIMS (hbig %u nr_xormaps %u)\n",
> + (unsigned int)p->hbig,
> + (unsigned int)p->nr_xormaps);
> + }
> + break;
> + default:
> + pr_warn("CEDT: Found unsupported entry (type = 0x%x)\n",
> + header->type);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void __init acpi_table_print_cedt_entry(enum acpi_cedt_type id)
> +{
> + acpi_table_parse_cedt(id, __acpi_table_print_cedt_entry, NULL);
> +}
> +
> +static void __init acpi_table_print_cedt(void)
> +{
> + /* Print only implemented CEDT types */
> + acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CHBS);
> + acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CFMWS);
> + acpi_table_print_cedt_entry(ACPI_CEDT_TYPE_CXIMS);
Would this be cleaner just breaking these up?
acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, print_chbs_entry, NULL);
acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, print_cfmws_entry, NULL);
acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS, print_cxims_entry, NULL);
> +}
> +
> static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> void *arg, const unsigned long table_end)
> {
> @@ -318,8 +426,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> * found for any portion of the window to cover the entire
> * window.
> */
> - if (!numa_fill_memblks(start, end))
> + if (!numa_fill_memblks(start, end)) {
> + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n",
> + (unsigned long long) start, (unsigned long long) end - 1);
> return 0;
> + }
>
> /* No SRAT description. Create a new node. */
> node = acpi_map_pxm_to_node(*fake_pxm);
> @@ -334,13 +445,19 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
> node, start, end);
> }
> +
Unrelated changed. Best to clean this out to avoid the (really minor) noise.
> node_set(node, numa_nodes_parsed);
>
> + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n",
> + node, *fake_pxm,
> + (unsigned long long) start, (unsigned long long) end - 1);
> +
> /* Set the next available fake_pxm value */
> (*fake_pxm)++;
> return 0;
> }
> #else
> +static inline void acpi_table_print_cedt(void) {}
> static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> void *arg, const unsigned long table_end)
> {
> @@ -526,6 +643,9 @@ int __init acpi_numa_init(void)
> /* SLIT: System Locality Information Table */
> acpi_table_parse(ACPI_SIG_SLIT, acpi_parse_slit);
>
> + /* CEDT: CXL Early Discovery Table */
> + acpi_table_print_cedt();
> +
> /*
> * CXL Fixed Memory Window Structures (CFMWS) must be parsed
> * after the SRAT. Create NUMA Nodes for CXL memory ranges that
Powered by blists - more mailing lists