ACPI: Support Generic Initiator proximity domains

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 <[email protected]>

---
 arch/x86/include/asm/numa.h |  2 ++
 arch/x86/kernel/setup.c     |  1 +
 arch/x86/mm/numa.c          | 14 ++++++++++++++
 3 files changed, 17 insertions(+)

arch/x86/include/asm/numa.h +2 -0

index bbfde3d2662f..f631467272a3 100644
@@ -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

arch/x86/kernel/setup.c +1 -0

index 3511736fbc74..9062c146f03a 100644
@@ -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();
 

arch/x86/mm/numa.c +3 -0

index aa76ec2d359b..fc630dc6764e 100644
@@ -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

Quoted 60 lines · 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 <[email protected]>

---
 arch/x86/include/asm/numa.h |  2 ++
 arch/x86/kernel/setup.c     |  1 +
 arch/x86/mm/numa.c          | 14 ++++++++++++++
 3 files changed, 17 insertions(+)

arch/x86/include/asm/numa.h +2 -0

index bbfde3d2662f..f631467272a3 100644
@@ -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

arch/x86/kernel/setup.c +1 -0

index 3511736fbc74..9062c146f03a 100644
@@ -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();
 

arch/x86/mm/numa.c +9 -0

index aa76ec2d359b..fc630dc6764e 100644
@@ -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;
+

arch/x86/mm/numa.c static void __init init_memory_less_node(int nid) · line 762

Quoted 4 lines

arch/x86/mm/numa.c +4 -0

+	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.

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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644

drivers/acpi/numa/hmat.c +0 -0

 		target = find_mem_target(p->memory_PD);
 		if (!target) {
 			pr_debug("HMAT: Memory Domain missing from SRAT\n");

Signature · 1 line

2.19.1

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

-	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) {

Quoted 7 lines

drivers/acpi/numa/hmat.c +0 -0

 		target = find_mem_target(p->memory_PD);
 		if (!target) {
 			pr_debug("HMAT: Memory Domain missing from SRAT\n");

Signature · 1 line

2.19.1

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

-	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) {

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.

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 4 lines

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

Quoted 43 lines · On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 7 lines

  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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 54 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 7 lines

  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.

Quoted 7 lines

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.

Quoted 16 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 124 lines · Bjorn Helgaas

Quoted 54 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 7 lines

  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.

Quoted 7 lines

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.

Quoted 16 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 54 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 7 lines

  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.

Quoted 7 lines

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.

Quoted 16 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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

Quoted 70 lines · On Fri, 21 Aug 2020 08:46:22 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 54 lines · Jonathan Cameron

Quoted 43 lines · On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous memory")]

Quoted 28 lines · Jonathan Cameron

Quoted 25 lines · On Thu, 20 Aug 2020 17:21:29 -0500 Bjorn Helgaas <[email protected]> wrote:

Quoted 24 lines · 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 <[email protected]>

---
 drivers/acpi/numa/hmat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/acpi/numa/hmat.c +0 -0

index 2c32cfb72370..07cfe50136e0 100644
@@ -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);
 

Quoted 8 lines

Quoted 8 lines

drivers/acpi/numa/hmat.c static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade · line 427

Quoted 2 lines

drivers/acpi/numa/hmat.c +1 -1

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

Quoted 7 lines

  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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 29 lines

2) Do we need to test hmat_revision for anything other than 1? Yes, you're right, see below.

Quoted 16 lines

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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:

Quoted 6 lines

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.