* [PATCH v9 1/6] ACPI: Support Generic Initiator only domains
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains Jonathan Cameron
` (4 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
Generic Initiators are a new ACPI concept that allows for the
description of proximity domains that contain a device which
performs memory access (such as a network card) but neither
host CPU nor Memory.
This patch has the parsing code and provides the infrastructure
for an architecture to associate these new domains with their
nearest memory processing node.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/acpi/numa/srat.c | 69 +++++++++++++++++++++++++++++++++++++++-
drivers/base/node.c | 3 ++
include/linux/nodemask.h | 1 +
3 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 15bbaab8500b..d27e8585132d 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -130,6 +130,36 @@ acpi_table_print_srat_entry(struct acpi_subtable_header *header)
}
break;
+ case ACPI_SRAT_TYPE_GENERIC_AFFINITY:
+ {
+ struct acpi_srat_generic_affinity *p =
+ (struct acpi_srat_generic_affinity *)header;
+
+ if (p->device_handle_type == 0) {
+ /*
+ * For pci devices this may be the only place they
+ * are assigned a proximity domain
+ */
+ pr_debug("SRAT Generic Initiator(Seg:%u BDF:%u) in proximity domain %d %s\n",
+ *(u16 *)(&p->device_handle[0]),
+ *(u16 *)(&p->device_handle[2]),
+ p->proximity_domain,
+ (p->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED) ?
+ "enabled" : "disabled");
+ } else {
+ /*
+ * In this case we can rely on the device having a
+ * proximity domain reference
+ */
+ pr_debug("SRAT Generic Initiator(HID=%.8s UID=%.4s) in proximity domain %d %s\n",
+ (char *)(&p->device_handle[0]),
+ (char *)(&p->device_handle[8]),
+ p->proximity_domain,
+ (p->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED) ?
+ "enabled" : "disabled");
+ }
+ }
+ break;
default:
pr_warn("Found unsupported SRAT entry (type = 0x%x)\n",
header->type);
@@ -332,6 +362,41 @@ acpi_parse_gicc_affinity(union acpi_subtable_headers *header,
return 0;
}
+#if defined(CONFIG_X86) || defined(CONFIG_ARM64)
+static int __init
+acpi_parse_gi_affinity(union acpi_subtable_headers *header,
+ const unsigned long end)
+{
+ struct acpi_srat_generic_affinity *gi_affinity;
+ int node;
+
+ gi_affinity = (struct acpi_srat_generic_affinity *)header;
+ if (!gi_affinity)
+ return -EINVAL;
+ acpi_table_print_srat_entry(&header->common);
+
+ if (!(gi_affinity->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED))
+ return -EINVAL;
+
+ node = acpi_map_pxm_to_node(gi_affinity->proximity_domain);
+ if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
+ pr_err("SRAT: Too many proximity domains.\n");
+ return -EINVAL;
+ }
+ node_set(node, numa_nodes_parsed);
+ node_set_state(node, N_GENERIC_INITIATOR);
+
+ return 0;
+}
+#else
+static int __init
+acpi_parse_gi_affinity(union acpi_subtable_headers *header,
+ const unsigned long end)
+{
+ return 0;
+}
+#endif /* defined(CONFIG_X86) || defined (CONFIG_ARM64) */
+
static int __initdata parsed_numa_memblks;
static int __init
@@ -385,7 +450,7 @@ int __init acpi_numa_init(void)
/* SRAT: System Resource Affinity Table */
if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
- struct acpi_subtable_proc srat_proc[3];
+ struct acpi_subtable_proc srat_proc[4];
memset(srat_proc, 0, sizeof(srat_proc));
srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY;
@@ -394,6 +459,8 @@ int __init acpi_numa_init(void)
srat_proc[1].handler = acpi_parse_x2apic_affinity;
srat_proc[2].id = ACPI_SRAT_TYPE_GICC_AFFINITY;
srat_proc[2].handler = acpi_parse_gicc_affinity;
+ srat_proc[3].id = ACPI_SRAT_TYPE_GENERIC_AFFINITY;
+ srat_proc[3].handler = acpi_parse_gi_affinity;
acpi_table_parse_entries_array(ACPI_SIG_SRAT,
sizeof(struct acpi_table_srat),
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 508b80f6329b..53383f1f683c 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -980,6 +980,8 @@ static struct node_attr node_state_attr[] = {
#endif
[N_MEMORY] = _NODE_ATTR(has_memory, N_MEMORY),
[N_CPU] = _NODE_ATTR(has_cpu, N_CPU),
+ [N_GENERIC_INITIATOR] = _NODE_ATTR(has_generic_initiator,
+ N_GENERIC_INITIATOR),
};
static struct attribute *node_state_attrs[] = {
@@ -991,6 +993,7 @@ static struct attribute *node_state_attrs[] = {
#endif
&node_state_attr[N_MEMORY].attr.attr,
&node_state_attr[N_CPU].attr.attr,
+ &node_state_attr[N_GENERIC_INITIATOR].attr.attr,
NULL
};
diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 27e7fa36f707..3334ce056335 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h
@@ -399,6 +399,7 @@ enum node_states {
#endif
N_MEMORY, /* The node has memory(regular, high, movable) */
N_CPU, /* The node has one or more cpus */
+ N_GENERIC_INITIATOR, /* The node has one or more Generic Initiators */
NR_NODE_STATES
};
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 1/6] ACPI: Support Generic Initiator only domains Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
2020-08-20 22:24 ` Bjorn Helgaas
2020-08-19 14:51 ` [PATCH v9 3/6] ACPI: Let ACPI know we support Generic Initiator Affinity Structures Jonathan Cameron
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
In common with memoryless domains we only register GI domains
if the proximity node is not online. If a domain is already
a memory containing domain, or a memoryless domain there is
nothing to do just because it also contains a Generic Initiator.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
arch/x86/include/asm/numa.h | 2 ++
arch/x86/kernel/setup.c | 1 +
arch/x86/mm/numa.c | 14 ++++++++++++++
3 files changed, 17 insertions(+)
diff --git a/arch/x86/include/asm/numa.h b/arch/x86/include/asm/numa.h
index bbfde3d2662f..f631467272a3 100644
--- a/arch/x86/include/asm/numa.h
+++ b/arch/x86/include/asm/numa.h
@@ -62,12 +62,14 @@ extern void numa_clear_node(int cpu);
extern void __init init_cpu_to_node(void);
extern void numa_add_cpu(int cpu);
extern void numa_remove_cpu(int cpu);
+extern void init_gi_nodes(void);
#else /* CONFIG_NUMA */
static inline void numa_set_node(int cpu, int node) { }
static inline void numa_clear_node(int cpu) { }
static inline void init_cpu_to_node(void) { }
static inline void numa_add_cpu(int cpu) { }
static inline void numa_remove_cpu(int cpu) { }
+static inline void init_gi_nodes(void) { }
#endif /* CONFIG_NUMA */
#ifdef CONFIG_DEBUG_PER_CPU_MAPS
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 3511736fbc74..9062c146f03a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1218,6 +1218,7 @@ void __init setup_arch(char **cmdline_p)
prefill_possible_map();
init_cpu_to_node();
+ init_gi_nodes();
io_apic_init_mappings();
diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index aa76ec2d359b..fc630dc6764e 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -747,6 +747,20 @@ static void __init init_memory_less_node(int nid)
*/
}
+/*
+ * Generic Initiator Nodes may have neither CPU nor Memory.
+ * At this stage if either of the others were present we would
+ * already be online.
+ */
+void __init init_gi_nodes(void)
+{
+ int nid;
+
+ for_each_node_state(nid, N_GENERIC_INITIATOR)
+ if (!node_online(nid))
+ init_memory_less_node(nid);
+}
+
/*
* Setup early cpu_to_node.
*
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains
2020-08-19 14:51 ` [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains Jonathan Cameron
@ 2020-08-20 22:24 ` Bjorn Helgaas
2020-08-21 8:54 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-20 22:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api
On Wed, Aug 19, 2020 at 10:51:07PM +0800, Jonathan Cameron wrote:
> In common with memoryless domains we only register GI domains
> if the proximity node is not online. If a domain is already
> a memory containing domain, or a memoryless domain there is
> nothing to do just because it also contains a Generic Initiator.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> arch/x86/include/asm/numa.h | 2 ++
> arch/x86/kernel/setup.c | 1 +
> arch/x86/mm/numa.c | 14 ++++++++++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/include/asm/numa.h b/arch/x86/include/asm/numa.h
> index bbfde3d2662f..f631467272a3 100644
> --- a/arch/x86/include/asm/numa.h
> +++ b/arch/x86/include/asm/numa.h
> @@ -62,12 +62,14 @@ extern void numa_clear_node(int cpu);
> extern void __init init_cpu_to_node(void);
> extern void numa_add_cpu(int cpu);
> extern void numa_remove_cpu(int cpu);
> +extern void init_gi_nodes(void);
> #else /* CONFIG_NUMA */
> static inline void numa_set_node(int cpu, int node) { }
> static inline void numa_clear_node(int cpu) { }
> static inline void init_cpu_to_node(void) { }
> static inline void numa_add_cpu(int cpu) { }
> static inline void numa_remove_cpu(int cpu) { }
> +static inline void init_gi_nodes(void) { }
> #endif /* CONFIG_NUMA */
>
> #ifdef CONFIG_DEBUG_PER_CPU_MAPS
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 3511736fbc74..9062c146f03a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1218,6 +1218,7 @@ void __init setup_arch(char **cmdline_p)
> prefill_possible_map();
>
> init_cpu_to_node();
> + init_gi_nodes();
>
> io_apic_init_mappings();
>
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index aa76ec2d359b..fc630dc6764e 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -747,6 +747,20 @@ static void __init init_memory_less_node(int nid)
> */
> }
>
> +/*
> + * Generic Initiator Nodes may have neither CPU nor Memory.
> + * At this stage if either of the others were present we would
> + * already be online.
> + */
> +void __init init_gi_nodes(void)
> +{
> + int nid;
> +
> + for_each_node_state(nid, N_GENERIC_INITIATOR)
> + if (!node_online(nid))
> + init_memory_less_node(nid);
> +}
This doesn't *look* very x86-specific, and apparently you don't need
any arm64-specific changes? Too bad this can't be unified a little
bit to remove the arch #ifdefs completely.
I do see that init_memory_less_node() is only implemented on x86, but
it just seems like all this might not be inherently be arch-specific.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains
2020-08-20 22:24 ` Bjorn Helgaas
@ 2020-08-21 8:54 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-21 8:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api
On Thu, 20 Aug 2020 17:24:33 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Aug 19, 2020 at 10:51:07PM +0800, Jonathan Cameron wrote:
> > In common with memoryless domains we only register GI domains
> > if the proximity node is not online. If a domain is already
> > a memory containing domain, or a memoryless domain there is
> > nothing to do just because it also contains a Generic Initiator.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > arch/x86/include/asm/numa.h | 2 ++
> > arch/x86/kernel/setup.c | 1 +
> > arch/x86/mm/numa.c | 14 ++++++++++++++
> > 3 files changed, 17 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/numa.h b/arch/x86/include/asm/numa.h
> > index bbfde3d2662f..f631467272a3 100644
> > --- a/arch/x86/include/asm/numa.h
> > +++ b/arch/x86/include/asm/numa.h
> > @@ -62,12 +62,14 @@ extern void numa_clear_node(int cpu);
> > extern void __init init_cpu_to_node(void);
> > extern void numa_add_cpu(int cpu);
> > extern void numa_remove_cpu(int cpu);
> > +extern void init_gi_nodes(void);
> > #else /* CONFIG_NUMA */
> > static inline void numa_set_node(int cpu, int node) { }
> > static inline void numa_clear_node(int cpu) { }
> > static inline void init_cpu_to_node(void) { }
> > static inline void numa_add_cpu(int cpu) { }
> > static inline void numa_remove_cpu(int cpu) { }
> > +static inline void init_gi_nodes(void) { }
> > #endif /* CONFIG_NUMA */
> >
> > #ifdef CONFIG_DEBUG_PER_CPU_MAPS
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 3511736fbc74..9062c146f03a 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -1218,6 +1218,7 @@ void __init setup_arch(char **cmdline_p)
> > prefill_possible_map();
> >
> > init_cpu_to_node();
> > + init_gi_nodes();
> >
> > io_apic_init_mappings();
> >
> > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> > index aa76ec2d359b..fc630dc6764e 100644
> > --- a/arch/x86/mm/numa.c
> > +++ b/arch/x86/mm/numa.c
> > @@ -747,6 +747,20 @@ static void __init init_memory_less_node(int nid)
> > */
> > }
> >
> > +/*
> > + * Generic Initiator Nodes may have neither CPU nor Memory.
> > + * At this stage if either of the others were present we would
> > + * already be online.
> > + */
> > +void __init init_gi_nodes(void)
> > +{
> > + int nid;
> > +
> > + for_each_node_state(nid, N_GENERIC_INITIATOR)
> > + if (!node_online(nid))
> > + init_memory_less_node(nid);
> > +}
>
> This doesn't *look* very x86-specific, and apparently you don't need
> any arm64-specific changes? Too bad this can't be unified a little
> bit to remove the arch #ifdefs completely.
The only ifdefs in the set aren't actually about this (which is entirely
contained in arch code) they are to avoid ia64 issues as has an entirely
different implementation that I doubt anyone wants to touch!
>
> I do see that init_memory_less_node() is only implemented on x86, but
> it just seems like all this might not be inherently be arch-specific.
I absolutely agree with a long term aim to unify the numa setup code
across architectures.
The x86 code is rather more involved than what we have for arm64. On arm64,
memoryless nodes are handled in the same pass as those with memory (and
we get GI nodes for free).
There are some separate patches under discussion that take a few steps
in that direction.
https://lore.kernel.org/linux-arm-kernel/20200814214725.28818-3-atish.patra@wdc.com/T/
https://patchwork.kernel.org/patch/11651437/ (this one is mostly by coincidence rather
than intent!)
Jonathan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v9 3/6] ACPI: Let ACPI know we support Generic Initiator Affinity Structures
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 1/6] ACPI: Support Generic Initiator only domains Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 2/6] x86: Support Generic Initiator only proximity domains Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3 Jonathan Cameron
` (2 subsequent siblings)
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
Until we tell ACPI that we support generic initiators, it will have
to operate in fall back domain mode and all _PXM entries should
be on existing non GI domains.
This patch sets the relevant OSC bit to make that happen.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/acpi/bus.c | 4 ++++
include/linux/acpi.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 54002670cb7a..113c661eb848 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -303,7 +303,11 @@ static void acpi_bus_osc_support(void)
capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
+#ifdef CONFIG_ARM64
+ capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
+#endif
#ifdef CONFIG_X86
+ capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT;
if (boot_cpu_has(X86_FEATURE_HWP)) {
capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1e4cdc6c7ae2..1321518a53d2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -546,6 +546,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
#define OSC_SB_PCLPI_SUPPORT 0x00000080
#define OSC_SB_OSLPI_SUPPORT 0x00000100
#define OSC_SB_CPC_DIVERSE_HIGH_SUPPORT 0x00001000
+#define OSC_SB_GENERIC_INITIATOR_SUPPORT 0x00002000
extern bool osc_sb_apei_support_acked;
extern bool osc_pc_lpi_support_confirmed;
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
` (2 preceding siblings ...)
2020-08-19 14:51 ` [PATCH v9 3/6] ACPI: Let ACPI know we support Generic Initiator Affinity Structures Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
2020-08-20 22:21 ` Bjorn Helgaas
2020-08-19 14:51 ` [PATCH v9 5/6] node: Add access1 class to represent CPU to memory characteristics Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 6/6] docs: mm: numaperf.rst Add brief description for access class 1 Jonathan Cameron
5 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
In ACPI 6.3, the Memory Proximity Domain Attributes Structure
changed substantially. One of those changes was that the flag
for "Memory Proximity Domain field is valid" was deprecated.
This was because the field "Proximity Domain for the Memory"
became a required field and hence having a validity flag makes
no sense.
So the correct logic is to always assume the field is there.
Current code assumes it never is.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/acpi/numa/hmat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 2c32cfb72370..07cfe50136e0 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
p->flags, p->processor_PD, p->memory_PD);
- if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
+ if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
target = find_mem_target(p->memory_PD);
if (!target) {
pr_debug("HMAT: Memory Domain missing from SRAT\n");
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-19 14:51 ` [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3 Jonathan Cameron
@ 2020-08-20 22:21 ` Bjorn Helgaas
2020-08-21 8:42 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-20 22:21 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api
On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> changed substantially. One of those changes was that the flag
> for "Memory Proximity Domain field is valid" was deprecated.
>
> This was because the field "Proximity Domain for the Memory"
> became a required field and hence having a validity flag makes
> no sense.
>
> So the correct logic is to always assume the field is there.
> Current code assumes it never is.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/acpi/numa/hmat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 2c32cfb72370..07cfe50136e0 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> p->flags, p->processor_PD, p->memory_PD);
>
> - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
I hope/assume the spec is written in such a way that p->memory_PD is
required for any revision > 1? So maybe this should be:
if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
hmat_revision > 1) {
> target = find_mem_target(p->memory_PD);
> if (!target) {
> pr_debug("HMAT: Memory Domain missing from SRAT\n");
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-20 22:21 ` Bjorn Helgaas
@ 2020-08-21 8:42 ` Jonathan Cameron
2020-08-21 12:13 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-21 8:42 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api
On Thu, 20 Aug 2020 17:21:29 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > changed substantially. One of those changes was that the flag
> > for "Memory Proximity Domain field is valid" was deprecated.
> >
> > This was because the field "Proximity Domain for the Memory"
> > became a required field and hence having a validity flag makes
> > no sense.
> >
> > So the correct logic is to always assume the field is there.
> > Current code assumes it never is.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > drivers/acpi/numa/hmat.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > index 2c32cfb72370..07cfe50136e0 100644
> > --- a/drivers/acpi/numa/hmat.c
> > +++ b/drivers/acpi/numa/hmat.c
> > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > p->flags, p->processor_PD, p->memory_PD);
> >
> > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
>
Hi Bjorn,
> I hope/assume the spec is written in such a way that p->memory_PD is
> required for any revision > 1? So maybe this should be:
>
> if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> hmat_revision > 1) {
Good point. We have existing protections elsewhere against hmat_revision
being anything other than 1 or 2, so we should aim to keep that in only one place.
I'll tidy this up for v10.
thanks,
Jonathan
>
> > target = find_mem_target(p->memory_PD);
> > if (!target) {
> > pr_debug("HMAT: Memory Domain missing from SRAT\n");
> > --
> > 2.19.1
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 8:42 ` Jonathan Cameron
@ 2020-08-21 12:13 ` Bjorn Helgaas
2020-08-21 12:59 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-21 12:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
heterogeneous memory")]
On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Aug 2020 17:21:29 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > changed substantially. One of those changes was that the flag
> > > for "Memory Proximity Domain field is valid" was deprecated.
> > >
> > > This was because the field "Proximity Domain for the Memory"
> > > became a required field and hence having a validity flag makes
> > > no sense.
> > >
> > > So the correct logic is to always assume the field is there.
> > > Current code assumes it never is.
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > drivers/acpi/numa/hmat.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > index 2c32cfb72370..07cfe50136e0 100644
> > > --- a/drivers/acpi/numa/hmat.c
> > > +++ b/drivers/acpi/numa/hmat.c
> > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > p->flags, p->processor_PD, p->memory_PD);
> > >
> > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> >
> > I hope/assume the spec is written in such a way that p->memory_PD is
> > required for any revision > 1? So maybe this should be:
> >
> > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > hmat_revision > 1) {
I should have said simply:
if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
we already know it's revision 1.
And unless there was a revision 0 of HMAT, there's no need to look for
hmat_revison > 1.
> Good point. We have existing protections elsewhere against
> hmat_revision being anything other than 1 or 2, so we should aim to
> keep that in only one place.
I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
memory"), is a mistake.
And I think hmat_normalize() has a similar mistake in that it tests
explicitly for hmat_revision == 2 when it should accept 2 AND anything
later.
We should assume that future spec revisions will be backwards
compatible. Otherwise we're forced to make kernel changes when we
otherwise would not have to.
Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 12:13 ` Bjorn Helgaas
@ 2020-08-21 12:59 ` Jonathan Cameron
2020-08-21 13:46 ` Bjorn Helgaas
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-21 12:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, 21 Aug 2020 07:13:56 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> heterogeneous memory")]
>
> On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > On Thu, 20 Aug 2020 17:21:29 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > changed substantially. One of those changes was that the flag
> > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > >
> > > > This was because the field "Proximity Domain for the Memory"
> > > > became a required field and hence having a validity flag makes
> > > > no sense.
> > > >
> > > > So the correct logic is to always assume the field is there.
> > > > Current code assumes it never is.
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > --- a/drivers/acpi/numa/hmat.c
> > > > +++ b/drivers/acpi/numa/hmat.c
> > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > p->flags, p->processor_PD, p->memory_PD);
> > > >
> > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > >
> > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > required for any revision > 1? So maybe this should be:
> > >
> > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > hmat_revision > 1) {
>
> I should have said simply:
>
> if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
>
> We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> we already know it's revision 1.
>
> And unless there was a revision 0 of HMAT, there's no need to look for
> hmat_revison > 1.
It needs to stay as an or statement as you had the first time.
The field is always valid for hmat_revision > 1, and valid for
hmat_revision == 1 with the flag set. You could express it as
if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
but that seems more confusing to me.
>
> > Good point. We have existing protections elsewhere against
> > hmat_revision being anything other than 1 or 2, so we should aim to
> > keep that in only one place.
>
> I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> memory"), is a mistake.
>
> And I think hmat_normalize() has a similar mistake in that it tests
> explicitly for hmat_revision == 2 when it should accept 2 AND anything
> later.
>
> We should assume that future spec revisions will be backwards
> compatible. Otherwise we're forced to make kernel changes when we
> otherwise would not have to.
I disagree with this. There is no rule in ACPI about maintaining
backwards compatibility. The assumption is that the version number
will always be checked. The meaning of fields changed between
version 1 and version 2 so it would be bold to assume that won't
happen in the future!
HMAT is an optional table, so if someone boots up an old kernel
they are probably better off failing to use it at all than
misinterpreting it.
Having the sanity check in one place makes sense, but removing it
entirely is a bad idea.
Jonathan
>
> Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 12:59 ` Jonathan Cameron
@ 2020-08-21 13:46 ` Bjorn Helgaas
2020-08-21 14:59 ` Bjorn Helgaas
2020-08-21 16:37 ` Jonathan Cameron
0 siblings, 2 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-21 13:46 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Aug 2020 07:13:56 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> > heterogeneous memory")]
> >
> > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > > On Thu, 20 Aug 2020 17:21:29 -0500
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > > changed substantially. One of those changes was that the flag
> > > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > > >
> > > > > This was because the field "Proximity Domain for the Memory"
> > > > > became a required field and hence having a validity flag makes
> > > > > no sense.
> > > > >
> > > > > So the correct logic is to always assume the field is there.
> > > > > Current code assumes it never is.
> > > > >
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > ---
> > > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > > --- a/drivers/acpi/numa/hmat.c
> > > > > +++ b/drivers/acpi/numa/hmat.c
> > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > > p->flags, p->processor_PD, p->memory_PD);
> > > > >
> > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > > >
> > > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > > required for any revision > 1? So maybe this should be:
> > > >
> > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > > hmat_revision > 1) {
> >
> > I should have said simply:
> >
> > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> >
> > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> > we already know it's revision 1.
> >
> > And unless there was a revision 0 of HMAT, there's no need to look for
> > hmat_revison > 1.
>
> It needs to stay as an or statement as you had the first time.
> The field is always valid for hmat_revision > 1, and valid for
> hmat_revision == 1 with the flag set. You could express it as
>
> if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
>
> but that seems more confusing to me.
Oh, you're right, sorry! There are two questions here:
1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID"
and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined
only when "hmat_revision == 1", so I think we should test the
revision first.
When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved,
so we shouldn't test it, even if we later check the revision and
discard the result of the flag test. This is a tiny thing,
admittedly, but I think it follows the spec more clearly.
2) Do we need to test hmat_revision for anything other than 1? Yes,
you're right, see below.
> > > Good point. We have existing protections elsewhere against
> > > hmat_revision being anything other than 1 or 2, so we should aim to
> > > keep that in only one place.
> >
> > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> > memory"), is a mistake.
> >
> > And I think hmat_normalize() has a similar mistake in that it tests
> > explicitly for hmat_revision == 2 when it should accept 2 AND anything
> > later.
> >
> > We should assume that future spec revisions will be backwards
> > compatible. Otherwise we're forced to make kernel changes when we
> > otherwise would not have to.
>
> I disagree with this. There is no rule in ACPI about maintaining
> backwards compatibility. The assumption is that the version number
> will always be checked. The meaning of fields changed between
> version 1 and version 2 so it would be bold to assume that won't
> happen in the future!
There *is* a rule about maintaining backwards compatibility. ACPI
v6.3, sec 5.2.2, says:
All versions of the ACPI tables must maintain backward
compatibility. To accomplish this, modifications of the tables
consist of redefinition of previously reserved fields and values
plus appending data to the 1.0 tables. Modifications of the ACPI
tables require that the version numbers of the modified tables be
incremented.
> HMAT is an optional table, so if someone boots up an old kernel
> they are probably better off failing to use it at all than
> misinterpreting it.
An old kernel tests:
if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1)
target = find_mem_target(p->memory_PD);
which is fine on old firmware. On new firmware (hmat_revision == 2),
it will ignore p->memory_PD. That is probably a problem, but I think
we should check for that at the place where we need a memory_PD and
don't find one. That's more general than sanity checking a revision.
A new kernel that tests:
if ((hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) ||
hmat_revision > 1)
target = find_mem_target(p->memory_PD);
will do the right thing on both old and new firmware.
Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 13:46 ` Bjorn Helgaas
@ 2020-08-21 14:59 ` Bjorn Helgaas
2020-08-21 16:30 ` Jonathan Cameron
2020-08-21 16:37 ` Jonathan Cameron
1 sibling, 1 reply; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-21 14:59 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, Aug 21, 2020 at 08:46:22AM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Aug 2020 07:13:56 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> > > heterogeneous memory")]
> > >
> > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > > > On Thu, 20 Aug 2020 17:21:29 -0500
> > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > > > changed substantially. One of those changes was that the flag
> > > > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > > > >
> > > > > > This was because the field "Proximity Domain for the Memory"
> > > > > > became a required field and hence having a validity flag makes
> > > > > > no sense.
> > > > > >
> > > > > > So the correct logic is to always assume the field is there.
> > > > > > Current code assumes it never is.
> > > > > >
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > ---
> > > > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > > > --- a/drivers/acpi/numa/hmat.c
> > > > > > +++ b/drivers/acpi/numa/hmat.c
> > > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > > > p->flags, p->processor_PD, p->memory_PD);
> > > > > >
> > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > > > >
> > > > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > > > required for any revision > 1? So maybe this should be:
> > > > >
> > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > > > hmat_revision > 1) {
> > >
> > > I should have said simply:
> > >
> > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> > >
> > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> > > we already know it's revision 1.
> > >
> > > And unless there was a revision 0 of HMAT, there's no need to look for
> > > hmat_revison > 1.
> >
> > It needs to stay as an or statement as you had the first time.
> > The field is always valid for hmat_revision > 1, and valid for
> > hmat_revision == 1 with the flag set. You could express it as
> >
> > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
> >
> > but that seems more confusing to me.
>
> Oh, you're right, sorry! There are two questions here:
>
> 1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID"
> and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined
> only when "hmat_revision == 1", so I think we should test the
> revision first.
>
> When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved,
> so we shouldn't test it, even if we later check the revision and
> discard the result of the flag test. This is a tiny thing,
> admittedly, but I think it follows the spec more clearly.
>
> 2) Do we need to test hmat_revision for anything other than 1? Yes,
> you're right, see below.
>
> > > > Good point. We have existing protections elsewhere against
> > > > hmat_revision being anything other than 1 or 2, so we should aim to
> > > > keep that in only one place.
> > >
> > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> > > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> > > memory"), is a mistake.
> > >
> > > And I think hmat_normalize() has a similar mistake in that it tests
> > > explicitly for hmat_revision == 2 when it should accept 2 AND anything
> > > later.
> > >
> > > We should assume that future spec revisions will be backwards
> > > compatible. Otherwise we're forced to make kernel changes when we
> > > otherwise would not have to.
> >
> > I disagree with this. There is no rule in ACPI about maintaining
> > backwards compatibility. The assumption is that the version number
> > will always be checked. The meaning of fields changed between
> > version 1 and version 2 so it would be bold to assume that won't
> > happen in the future!
>
> There *is* a rule about maintaining backwards compatibility. ACPI
> v6.3, sec 5.2.2, says:
>
> All versions of the ACPI tables must maintain backward
> compatibility. To accomplish this, modifications of the tables
> consist of redefinition of previously reserved fields and values
> plus appending data to the 1.0 tables. Modifications of the ACPI
> tables require that the version numbers of the modified tables be
> incremented.
>
> > HMAT is an optional table, so if someone boots up an old kernel
> > they are probably better off failing to use it at all than
> > misinterpreting it.
>
> An old kernel tests:
>
> if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1)
> target = find_mem_target(p->memory_PD);
>
> which is fine on old firmware. On new firmware (hmat_revision == 2),
> it will ignore p->memory_PD. That is probably a problem, but I think
> we should check for that at the place where we need a memory_PD and
> don't find one. That's more general than sanity checking a revision.
>
> A new kernel that tests:
>
> if ((hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) ||
> hmat_revision > 1)
> target = find_mem_target(p->memory_PD);
>
> will do the right thing on both old and new firmware.
Actually, I think this part of the spec was done incorrectly.
ACPI v6.3 could have made the p->memory_PD field required without
changing the definition of ACPI_HMAT_MEMORY_PD_VALID. What value was
gained by making ACPI_HMAT_MEMORY_PD_VALID a reserved bit in v6.3?
If they had left ACPI_HMAT_MEMORY_PD_VALID alone, the Linux code could
have been simply this, which would work with old firmware and new
firmware, and we wouldn't have to touch this at all:
if (p->flags & ACPI_HMAT_MEMORY_PD_VALID)
target = find_mem_target(p->memory_PD);
Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 14:59 ` Bjorn Helgaas
@ 2020-08-21 16:30 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-21 16:30 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, 21 Aug 2020 09:59:23 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Aug 21, 2020 at 08:46:22AM -0500, Bjorn Helgaas wrote:
> > On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote:
> > > On Fri, 21 Aug 2020 07:13:56 -0500
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> > > > heterogeneous memory")]
> > > >
> > > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > > > > On Thu, 20 Aug 2020 17:21:29 -0500
> > > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > >
> > > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > > > > changed substantially. One of those changes was that the flag
> > > > > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > > > > >
> > > > > > > This was because the field "Proximity Domain for the Memory"
> > > > > > > became a required field and hence having a validity flag makes
> > > > > > > no sense.
> > > > > > >
> > > > > > > So the correct logic is to always assume the field is there.
> > > > > > > Current code assumes it never is.
> > > > > > >
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > ---
> > > > > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > > > > --- a/drivers/acpi/numa/hmat.c
> > > > > > > +++ b/drivers/acpi/numa/hmat.c
> > > > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > > > > p->flags, p->processor_PD, p->memory_PD);
> > > > > > >
> > > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > > > > >
> > > > > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > > > > required for any revision > 1? So maybe this should be:
> > > > > >
> > > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > > > > hmat_revision > 1) {
> > > >
> > > > I should have said simply:
> > > >
> > > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> > > >
> > > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> > > > we already know it's revision 1.
> > > >
> > > > And unless there was a revision 0 of HMAT, there's no need to look for
> > > > hmat_revison > 1.
> > >
> > > It needs to stay as an or statement as you had the first time.
> > > The field is always valid for hmat_revision > 1, and valid for
> > > hmat_revision == 1 with the flag set. You could express it as
> > >
> > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
> > >
> > > but that seems more confusing to me.
> >
> > Oh, you're right, sorry! There are two questions here:
> >
> > 1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID"
> > and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined
> > only when "hmat_revision == 1", so I think we should test the
> > revision first.
> >
> > When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved,
> > so we shouldn't test it, even if we later check the revision and
> > discard the result of the flag test. This is a tiny thing,
> > admittedly, but I think it follows the spec more clearly.
> >
> > 2) Do we need to test hmat_revision for anything other than 1? Yes,
> > you're right, see below.
> >
> > > > > Good point. We have existing protections elsewhere against
> > > > > hmat_revision being anything other than 1 or 2, so we should aim to
> > > > > keep that in only one place.
> > > >
> > > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> > > > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> > > > memory"), is a mistake.
> > > >
> > > > And I think hmat_normalize() has a similar mistake in that it tests
> > > > explicitly for hmat_revision == 2 when it should accept 2 AND anything
> > > > later.
> > > >
> > > > We should assume that future spec revisions will be backwards
> > > > compatible. Otherwise we're forced to make kernel changes when we
> > > > otherwise would not have to.
> > >
> > > I disagree with this. There is no rule in ACPI about maintaining
> > > backwards compatibility. The assumption is that the version number
> > > will always be checked. The meaning of fields changed between
> > > version 1 and version 2 so it would be bold to assume that won't
> > > happen in the future!
> >
> > There *is* a rule about maintaining backwards compatibility. ACPI
> > v6.3, sec 5.2.2, says:
> >
> > All versions of the ACPI tables must maintain backward
> > compatibility. To accomplish this, modifications of the tables
> > consist of redefinition of previously reserved fields and values
> > plus appending data to the 1.0 tables. Modifications of the ACPI
> > tables require that the version numbers of the modified tables be
> > incremented.
> >
> > > HMAT is an optional table, so if someone boots up an old kernel
> > > they are probably better off failing to use it at all than
> > > misinterpreting it.
> >
> > An old kernel tests:
> >
> > if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1)
> > target = find_mem_target(p->memory_PD);
> >
> > which is fine on old firmware. On new firmware (hmat_revision == 2),
> > it will ignore p->memory_PD. That is probably a problem, but I think
> > we should check for that at the place where we need a memory_PD and
> > don't find one. That's more general than sanity checking a revision.
> >
> > A new kernel that tests:
> >
> > if ((hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) ||
> > hmat_revision > 1)
> > target = find_mem_target(p->memory_PD);
> >
> > will do the right thing on both old and new firmware.
>
> Actually, I think this part of the spec was done incorrectly.
>
> ACPI v6.3 could have made the p->memory_PD field required without
> changing the definition of ACPI_HMAT_MEMORY_PD_VALID. What value was
> gained by making ACPI_HMAT_MEMORY_PD_VALID a reserved bit in v6.3?
>
> If they had left ACPI_HMAT_MEMORY_PD_VALID alone, the Linux code could
> have been simply this, which would work with old firmware and new
> firmware, and we wouldn't have to touch this at all:
>
> if (p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> target = find_mem_target(p->memory_PD);
I have a slight recollection that might have been my fault :) Oops.
Jonathan
>
> Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 13:46 ` Bjorn Helgaas
2020-08-21 14:59 ` Bjorn Helgaas
@ 2020-08-21 16:37 ` Jonathan Cameron
2020-08-21 16:54 ` Bjorn Helgaas
1 sibling, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-21 16:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, 21 Aug 2020 08:46:22 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote:
> > On Fri, 21 Aug 2020 07:13:56 -0500
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> > > heterogeneous memory")]
> > >
> > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > > > On Thu, 20 Aug 2020 17:21:29 -0500
> > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > >
> > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > > > changed substantially. One of those changes was that the flag
> > > > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > > > >
> > > > > > This was because the field "Proximity Domain for the Memory"
> > > > > > became a required field and hence having a validity flag makes
> > > > > > no sense.
> > > > > >
> > > > > > So the correct logic is to always assume the field is there.
> > > > > > Current code assumes it never is.
> > > > > >
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > ---
> > > > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > > > --- a/drivers/acpi/numa/hmat.c
> > > > > > +++ b/drivers/acpi/numa/hmat.c
> > > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > > > p->flags, p->processor_PD, p->memory_PD);
> > > > > >
> > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > > > >
> > > > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > > > required for any revision > 1? So maybe this should be:
> > > > >
> > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > > > hmat_revision > 1) {
> > >
> > > I should have said simply:
> > >
> > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> > >
> > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> > > we already know it's revision 1.
> > >
> > > And unless there was a revision 0 of HMAT, there's no need to look for
> > > hmat_revison > 1.
> >
> > It needs to stay as an or statement as you had the first time.
> > The field is always valid for hmat_revision > 1, and valid for
> > hmat_revision == 1 with the flag set. You could express it as
> >
> > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
> >
> > but that seems more confusing to me.
>
> Oh, you're right, sorry! There are two questions here:
>
> 1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID"
> and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined
> only when "hmat_revision == 1", so I think we should test the
> revision first.
>
> When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved,
> so we shouldn't test it, even if we later check the revision and
> discard the result of the flag test. This is a tiny thing,
> admittedly, but I think it follows the spec more clearly.
Agreed.
>
> 2) Do we need to test hmat_revision for anything other than 1? Yes,
> you're right, see below.
>
> > > > Good point. We have existing protections elsewhere against
> > > > hmat_revision being anything other than 1 or 2, so we should aim to
> > > > keep that in only one place.
> > >
> > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> > > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> > > memory"), is a mistake.
> > >
> > > And I think hmat_normalize() has a similar mistake in that it tests
> > > explicitly for hmat_revision == 2 when it should accept 2 AND anything
> > > later.
> > >
> > > We should assume that future spec revisions will be backwards
> > > compatible. Otherwise we're forced to make kernel changes when we
> > > otherwise would not have to.
> >
> > I disagree with this. There is no rule in ACPI about maintaining
> > backwards compatibility. The assumption is that the version number
> > will always be checked. The meaning of fields changed between
> > version 1 and version 2 so it would be bold to assume that won't
> > happen in the future!
>
> There *is* a rule about maintaining backwards compatibility. ACPI
> v6.3, sec 5.2.2, says:
>
> All versions of the ACPI tables must maintain backward
> compatibility. To accomplish this, modifications of the tables
> consist of redefinition of previously reserved fields and values
> plus appending data to the 1.0 tables. Modifications of the ACPI
> tables require that the version numbers of the modified tables be
> incremented.
Fair point. Unfortunately it's not true here... The field we
are talking about here is probably fine, but the latency units
changed between v1 and v2.
>
> > HMAT is an optional table, so if someone boots up an old kernel
> > they are probably better off failing to use it at all than
> > misinterpreting it.
>
> An old kernel tests:
>
> if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1)
> target = find_mem_target(p->memory_PD);
>
> which is fine on old firmware. On new firmware (hmat_revision == 2),
> it will ignore p->memory_PD. That is probably a problem, but I think
> we should check for that at the place where we need a memory_PD and
> don't find one. That's more general than sanity checking a revision.
>
> A new kernel that tests:
>
> if ((hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) ||
> hmat_revision > 1)
> target = find_mem_target(p->memory_PD);
>
> will do the right thing on both old and new firmware.
>
For the case here we are fine, but as mentioned above, it's not the
only version dependent part.
Jonathan
> Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3
2020-08-21 16:37 ` Jonathan Cameron
@ 2020-08-21 16:54 ` Bjorn Helgaas
0 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2020-08-21 16:54 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-mm, linux-acpi, linux-arm-kernel, x86, Lorenzo Pieralisi,
Bjorn Helgaas, rafael, linux-kernel, Ingo Molnar,
Thomas Gleixner, linuxarm, Dan Williams, Brice Goglin,
Sean V Kelley, linux-api, Keith Busch
On Fri, Aug 21, 2020 at 05:37:18PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Aug 2020 08:46:22 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > On Fri, Aug 21, 2020 at 01:59:01PM +0100, Jonathan Cameron wrote:
> > > On Fri, 21 Aug 2020 07:13:56 -0500
> > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report
> > > > heterogeneous memory")]
> > > >
> > > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote:
> > > > > On Thu, 20 Aug 2020 17:21:29 -0500
> > > > > Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > >
> > > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote:
> > > > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure
> > > > > > > changed substantially. One of those changes was that the flag
> > > > > > > for "Memory Proximity Domain field is valid" was deprecated.
> > > > > > >
> > > > > > > This was because the field "Proximity Domain for the Memory"
> > > > > > > became a required field and hence having a validity flag makes
> > > > > > > no sense.
> > > > > > >
> > > > > > > So the correct logic is to always assume the field is there.
> > > > > > > Current code assumes it never is.
> > > > > > >
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > ---
> > > > > > > drivers/acpi/numa/hmat.c | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> > > > > > > index 2c32cfb72370..07cfe50136e0 100644
> > > > > > > --- a/drivers/acpi/numa/hmat.c
> > > > > > > +++ b/drivers/acpi/numa/hmat.c
> > > > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade
> > > > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n",
> > > > > > > p->flags, p->processor_PD, p->memory_PD);
> > > > > > >
> > > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) {
> > > > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) {
> > > > > >
> > > > > > I hope/assume the spec is written in such a way that p->memory_PD is
> > > > > > required for any revision > 1? So maybe this should be:
> > > > > >
> > > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) ||
> > > > > > hmat_revision > 1) {
> > > >
> > > > I should have said simply:
> > > >
> > > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID)
> > > >
> > > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless
> > > > we already know it's revision 1.
> > > >
> > > > And unless there was a revision 0 of HMAT, there's no need to look for
> > > > hmat_revison > 1.
> > >
> > > It needs to stay as an or statement as you had the first time.
> > > The field is always valid for hmat_revision > 1, and valid for
> > > hmat_revision == 1 with the flag set. You could express it as
> > >
> > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1))
> > >
> > > but that seems more confusing to me.
> >
> > Oh, you're right, sorry! There are two questions here:
> >
> > 1) In what order should we test "p->flags & ACPI_HMAT_MEMORY_PD_VALID"
> > and "hmat_revision == 1"? ACPI_HMAT_MEMORY_PD_VALID is defined
> > only when "hmat_revision == 1", so I think we should test the
> > revision first.
> >
> > When "hmat_revision == 2", ACPI_HMAT_MEMORY_PD_VALID is reserved,
> > so we shouldn't test it, even if we later check the revision and
> > discard the result of the flag test. This is a tiny thing,
> > admittedly, but I think it follows the spec more clearly.
>
> Agreed.
>
> >
> > 2) Do we need to test hmat_revision for anything other than 1? Yes,
> > you're right, see below.
> >
> > > > > Good point. We have existing protections elsewhere against
> > > > > hmat_revision being anything other than 1 or 2, so we should aim to
> > > > > keep that in only one place.
> > > >
> > > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(),
> > > > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous
> > > > memory"), is a mistake.
> > > >
> > > > And I think hmat_normalize() has a similar mistake in that it tests
> > > > explicitly for hmat_revision == 2 when it should accept 2 AND anything
> > > > later.
> > > >
> > > > We should assume that future spec revisions will be backwards
> > > > compatible. Otherwise we're forced to make kernel changes when we
> > > > otherwise would not have to.
> > >
> > > I disagree with this. There is no rule in ACPI about maintaining
> > > backwards compatibility. The assumption is that the version number
> > > will always be checked. The meaning of fields changed between
> > > version 1 and version 2 so it would be bold to assume that won't
> > > happen in the future!
> >
> > There *is* a rule about maintaining backwards compatibility. ACPI
> > v6.3, sec 5.2.2, says:
> >
> > All versions of the ACPI tables must maintain backward
> > compatibility. To accomplish this, modifications of the tables
> > consist of redefinition of previously reserved fields and values
> > plus appending data to the 1.0 tables. Modifications of the ACPI
> > tables require that the version numbers of the modified tables be
> > incremented.
>
> Fair point. Unfortunately it's not true here... The field we
> are talking about here is probably fine, but the latency units
> changed between v1 and v2.
Oops. Sounds like this should have been done in a way that didn't
break old kernels reading new tables. It's OK if old kernels can't
use new features, but not OK if things that used to work are broken
by new tables.
Bjorn
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v9 5/6] node: Add access1 class to represent CPU to memory characteristics
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
` (3 preceding siblings ...)
2020-08-19 14:51 ` [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3 Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
2020-08-19 14:51 ` [PATCH v9 6/6] docs: mm: numaperf.rst Add brief description for access class 1 Jonathan Cameron
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
New access1 class is nearly the same as access0, but always provides
characteristics for CPUs to memory. The existing access0 class
provides characteristics to nearest or direct connnect initiator
which may be a Generic Initiator such as a GPU or network adapter.
This new class allows thread placement on CPUs to be performed
so as to give optimal access characteristics to memory, even if that
memory is for example attached to a GPU or similar and only accessible
to the CPU via an appropriate bus.
Suggested-by: Dan Willaims <dan.j.williams@intel.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/acpi/numa/hmat.c | 87 +++++++++++++++++++++++++++++++---------
1 file changed, 68 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 07cfe50136e0..00b4cdbefb5e 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -56,7 +56,7 @@ struct memory_target {
unsigned int memory_pxm;
unsigned int processor_pxm;
struct resource memregions;
- struct node_hmem_attrs hmem_attrs;
+ struct node_hmem_attrs hmem_attrs[2];
struct list_head caches;
struct node_cache_attrs cache_attrs;
bool registered;
@@ -65,6 +65,7 @@ struct memory_target {
struct memory_initiator {
struct list_head node;
unsigned int processor_pxm;
+ bool has_cpu;
};
struct memory_locality {
@@ -108,6 +109,7 @@ static __init void alloc_memory_initiator(unsigned int cpu_pxm)
return;
initiator->processor_pxm = cpu_pxm;
+ initiator->has_cpu = node_state(pxm_to_node(cpu_pxm), N_CPU);
list_add_tail(&initiator->node, &initiators);
}
@@ -215,28 +217,28 @@ static u32 hmat_normalize(u16 entry, u64 base, u8 type)
}
static void hmat_update_target_access(struct memory_target *target,
- u8 type, u32 value)
+ u8 type, u32 value, int access)
{
switch (type) {
case ACPI_HMAT_ACCESS_LATENCY:
- target->hmem_attrs.read_latency = value;
- target->hmem_attrs.write_latency = value;
+ target->hmem_attrs[access].read_latency = value;
+ target->hmem_attrs[access].write_latency = value;
break;
case ACPI_HMAT_READ_LATENCY:
- target->hmem_attrs.read_latency = value;
+ target->hmem_attrs[access].read_latency = value;
break;
case ACPI_HMAT_WRITE_LATENCY:
- target->hmem_attrs.write_latency = value;
+ target->hmem_attrs[access].write_latency = value;
break;
case ACPI_HMAT_ACCESS_BANDWIDTH:
- target->hmem_attrs.read_bandwidth = value;
- target->hmem_attrs.write_bandwidth = value;
+ target->hmem_attrs[access].read_bandwidth = value;
+ target->hmem_attrs[access].write_bandwidth = value;
break;
case ACPI_HMAT_READ_BANDWIDTH:
- target->hmem_attrs.read_bandwidth = value;
+ target->hmem_attrs[access].read_bandwidth = value;
break;
case ACPI_HMAT_WRITE_BANDWIDTH:
- target->hmem_attrs.write_bandwidth = value;
+ target->hmem_attrs[access].write_bandwidth = value;
break;
default:
break;
@@ -329,8 +331,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
if (mem_hier == ACPI_HMAT_MEMORY) {
target = find_mem_target(targs[targ]);
- if (target && target->processor_pxm == inits[init])
- hmat_update_target_access(target, type, value);
+ if (target && target->processor_pxm == inits[init]) {
+ hmat_update_target_access(target, type, value, 0);
+ /* If the node has a CPU, update access 1*/
+ if (node_state(pxm_to_node(inits[init]), N_CPU))
+ hmat_update_target_access(target, type, value, 1);
+ }
}
}
}
@@ -566,6 +572,7 @@ static void hmat_register_target_initiators(struct memory_target *target)
unsigned int mem_nid, cpu_nid;
struct memory_locality *loc = NULL;
u32 best = 0;
+ bool access0done = false;
int i;
mem_nid = pxm_to_node(target->memory_pxm);
@@ -577,7 +584,11 @@ static void hmat_register_target_initiators(struct memory_target *target)
if (target->processor_pxm != PXM_INVAL) {
cpu_nid = pxm_to_node(target->processor_pxm);
register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
- return;
+ access0done = true;
+ if (node_state(cpu_nid, N_CPU)) {
+ register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
+ return;
+ }
}
if (list_empty(&localities))
@@ -591,6 +602,40 @@ static void hmat_register_target_initiators(struct memory_target *target)
*/
bitmap_zero(p_nodes, MAX_NUMNODES);
list_sort(p_nodes, &initiators, initiator_cmp);
+ if (!access0done) {
+ for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) {
+ loc = localities_types[i];
+ if (!loc)
+ continue;
+
+ best = 0;
+ list_for_each_entry(initiator, &initiators, node) {
+ u32 value;
+
+ if (!test_bit(initiator->processor_pxm, p_nodes))
+ continue;
+
+ value = hmat_initiator_perf(target, initiator,
+ loc->hmat_loc);
+ if (hmat_update_best(loc->hmat_loc->data_type, value, &best))
+ bitmap_clear(p_nodes, 0, initiator->processor_pxm);
+ if (value != best)
+ clear_bit(initiator->processor_pxm, p_nodes);
+ }
+ if (best)
+ hmat_update_target_access(target, loc->hmat_loc->data_type, best, 0);
+ }
+
+ for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
+ cpu_nid = pxm_to_node(i);
+ register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
+ }
+ }
+
+ /* Access 1 ignores Generic Initiators */
+ bitmap_zero(p_nodes, MAX_NUMNODES);
+ list_sort(p_nodes, &initiators, initiator_cmp);
+ best = 0;
for (i = WRITE_LATENCY; i <= READ_BANDWIDTH; i++) {
loc = localities_types[i];
if (!loc)
@@ -600,6 +645,10 @@ static void hmat_register_target_initiators(struct memory_target *target)
list_for_each_entry(initiator, &initiators, node) {
u32 value;
+ if (!initiator->has_cpu) {
+ clear_bit(initiator->processor_pxm, p_nodes);
+ continue;
+ }
if (!test_bit(initiator->processor_pxm, p_nodes))
continue;
@@ -610,12 +659,11 @@ static void hmat_register_target_initiators(struct memory_target *target)
clear_bit(initiator->processor_pxm, p_nodes);
}
if (best)
- hmat_update_target_access(target, loc->hmat_loc->data_type, best);
+ hmat_update_target_access(target, loc->hmat_loc->data_type, best, 1);
}
-
for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
cpu_nid = pxm_to_node(i);
- register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
+ register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
}
}
@@ -628,10 +676,10 @@ static void hmat_register_target_cache(struct memory_target *target)
node_add_cache(mem_nid, &tcache->cache_attrs);
}
-static void hmat_register_target_perf(struct memory_target *target)
+static void hmat_register_target_perf(struct memory_target *target, int access)
{
unsigned mem_nid = pxm_to_node(target->memory_pxm);
- node_set_perf_attrs(mem_nid, &target->hmem_attrs, 0);
+ node_set_perf_attrs(mem_nid, &target->hmem_attrs[access], access);
}
static void hmat_register_target_device(struct memory_target *target,
@@ -733,7 +781,8 @@ static void hmat_register_target(struct memory_target *target)
if (!target->registered) {
hmat_register_target_initiators(target);
hmat_register_target_cache(target);
- hmat_register_target_perf(target);
+ hmat_register_target_perf(target, 0);
+ hmat_register_target_perf(target, 1);
target->registered = true;
}
mutex_unlock(&target_lock);
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v9 6/6] docs: mm: numaperf.rst Add brief description for access class 1.
2020-08-19 14:51 [PATCH v9 0/6] ACPI: Support Generic Initiator proximity domains Jonathan Cameron
` (4 preceding siblings ...)
2020-08-19 14:51 ` [PATCH v9 5/6] node: Add access1 class to represent CPU to memory characteristics Jonathan Cameron
@ 2020-08-19 14:51 ` Jonathan Cameron
5 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2020-08-19 14:51 UTC (permalink / raw)
To: linux-mm, linux-acpi, linux-arm-kernel, x86
Cc: Lorenzo Pieralisi, Bjorn Helgaas, rafael, linux-kernel,
Ingo Molnar, Thomas Gleixner, linuxarm, Dan Williams,
Brice Goglin, Sean V Kelley, linux-api, Jonathan Cameron
Try to make minimal changes to the document which already describes
access class 0 in a generic fashion (including IO initiatiors that
are not CPUs).
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Documentation/admin-guide/mm/numaperf.rst | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/admin-guide/mm/numaperf.rst b/Documentation/admin-guide/mm/numaperf.rst
index 4d69ef1de830..b89bb85eac75 100644
--- a/Documentation/admin-guide/mm/numaperf.rst
+++ b/Documentation/admin-guide/mm/numaperf.rst
@@ -56,6 +56,11 @@ nodes' access characteristics share the same performance relative to other
linked initiator nodes. Each target within an initiator's access class,
though, do not necessarily perform the same as each other.
+The access class "1" is used to allow differentiation between initiators
+that are CPUs and hence suitable for generic task scheduling, and
+IO initiators such as GPUs and NICs. Unlike access class 0, only
+nodes containing CPUs are considered.
+
================
NUMA Performance
================
@@ -88,6 +93,9 @@ The latency attributes are provided in nanoseconds.
The values reported here correspond to the rated latency and bandwidth
for the platform.
+Access class 1, takes the same form, but only includes values for CPU to
+memory activity.
+
==========
NUMA Cache
==========
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread