[PATCH v2 0/8] AMD MCA Address Translation Updates

[PATCH v2 0/8] AMD MCA Address Translation Updates

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

This patchset includes updates for the MCA Address Translation process
on recent AMD systems.

Patches 1 & 3:
Fixes an input to the address translation function. The translation
requires a physical Die ID (NodeId in AMD documentation) rather than a
logicial NUMA node ID. This is because the physical and logical nodes
may not always match.

Patch 2:
Removes a function that is no longer needed with Patch 1.

Patches 4-7:
Code cleanup in preparation for Patch 8.

Patch 8:
Add translation support for new memory interleaving options available in
Rome systems. The patch is based on the latest AMD reference code for
the address translation.

Patches 6-8 have checkpatch warnings about long lines, but I kept the
long lines for readability.

Thanks,
Yazen

Link:
https://lkml.kernel.org/r/20200814191449.183998-1-Yazen.Ghannam@amd.com

v1 -> v2:
* Save the AMD NodeId value in struct cpuinfo_x86 rather than use a
  local value in MCA code.
* Include code cleanup for AMD MCA Address Translation function before
  adding new functionality.

Muralidhara M K (1):
  x86/MCE/AMD Support new memory interleaving modes during address
    translation

Yazen Ghannam (7):
  x86/CPU/AMD: Save NodeId on AMD-based systems
  x86/CPU/AMD: Remove amd_get_nb_id()
  EDAC/mce_amd: Use struct cpuinfo_x86.node_id for NodeId
  x86/MCE/AMD: Use defines for register addresses in translation code
  x86/MCE/AMD: Use macros to get bitfields in translation code
  x86/MCE/AMD: Drop tmp variable in translation code
  x86/MCE/AMD: Group register reads in translation code

 arch/x86/events/amd/core.c       |   2 +-
 arch/x86/include/asm/cacheinfo.h |   4 +-
 arch/x86/include/asm/processor.h |   3 +-
 arch/x86/kernel/amd_nb.c         |   4 +-
 arch/x86/kernel/cpu/amd.c        |  17 +-
 arch/x86/kernel/cpu/cacheinfo.c  |   8 +-
 arch/x86/kernel/cpu/hygon.c      |  11 +-
 arch/x86/kernel/cpu/mce/amd.c    | 284 ++++++++++++++++++++++---------
 arch/x86/kernel/cpu/mce/inject.c |   4 +-
 drivers/edac/amd64_edac.c        |   4 +-
 drivers/edac/mce_amd.c           |   4 +-
 11 files changed, 233 insertions(+), 112 deletions(-)

-- 
2.25.1

[PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

AMD systems provide a "NodeId" value that represents a global ID
indicating to which "Node" a logical CPU belongs. The "Node" is a
physical structure equivalent to a Die, and it should not be confused
with logical structures like NUMA node. Logical nodes can be adjusted
based on firmware or other settings whereas the physical nodes/dies are
fixed based on hardware topology.

The NodeId value can be used when a physical ID is needed by software.

Save the AMD NodeId to struct cpuinfo_x86. Use the value from CPUID or
MSR as appropriate. Default to phys_proc_id otherwise. Do so for both
AMD and Hygon systems.

Drop the node_id parameter from cacheinfo_*_init_llc_id() as it is no
longer needed.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-2-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on review comment to save value to struct cpuinfo_x86.

 arch/x86/include/asm/cacheinfo.h |  4 ++--
 arch/x86/include/asm/processor.h |  1 +
 arch/x86/kernel/cpu/amd.c        | 11 +++++------
 arch/x86/kernel/cpu/cacheinfo.c  |  6 +++---
 arch/x86/kernel/cpu/hygon.c      | 11 +++++------
 5 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/cacheinfo.h b/arch/x86/include/asm/cacheinfo.h
index 86b63c7feab7..86b2e0dcc4bf 100644
--- a/arch/x86/include/asm/cacheinfo.h
+++ b/arch/x86/include/asm/cacheinfo.h
@@ -2,7 +2,7 @@
 #ifndef _ASM_X86_CACHEINFO_H
 #define _ASM_X86_CACHEINFO_H
 
-void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
-void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id);
+void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu);
+void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu);
 
 #endif /* _ASM_X86_CACHEINFO_H */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 97143d87994c..a776b7886ec0 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -95,6 +95,7 @@ struct cpuinfo_x86 {
 	/* CPUID returned core id bits: */
 	__u8			x86_coreid_bits;
 	__u8			cu_id;
+	__u8			node_id;
 	/* Max extended CPUID function supported: */
 	__u32			extended_cpuid_level;
 	/* Maximum supported CPUID level, -1=no CPUID: */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index dcc3d943c68f..5eef4cc1e5b7 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -330,7 +330,6 @@ static void legacy_fixup_core_id(struct cpuinfo_x86 *c)
  */
 static void amd_get_topology(struct cpuinfo_x86 *c)
 {
-	u8 node_id;
 	int cpu = smp_processor_id();
 
 	/* get information required for multi-node processors */
@@ -340,7 +339,7 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 
 		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
 
-		node_id  = ecx & 0xff;
+		c->node_id  = ecx & 0xff;
 
 		if (c->x86 == 0x15)
 			c->cu_id = ebx & 0xff;
@@ -360,15 +359,15 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 		if (!err)
 			c->x86_coreid_bits = get_count_order(c->x86_max_cores);
 
-		cacheinfo_amd_init_llc_id(c, cpu, node_id);
+		cacheinfo_amd_init_llc_id(c, cpu);
 
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
-		node_id = value & 7;
+		c->node_id = value & 7;
 
-		per_cpu(cpu_llc_id, cpu) = node_id;
+		per_cpu(cpu_llc_id, cpu) = c->node_id;
 	} else
 		return;
 
@@ -393,7 +392,7 @@ static void amd_detect_cmp(struct cpuinfo_x86 *c)
 	/* Convert the initial APIC ID into the socket ID */
 	c->phys_proc_id = c->initial_apicid >> bits;
 	/* use socket ID also for last level cache */
-	per_cpu(cpu_llc_id, cpu) = c->phys_proc_id;
+	per_cpu(cpu_llc_id, cpu) = c->node_id = c->phys_proc_id;
 }
 
 static void amd_detect_ppin(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 57074cf3ad7c..81dfddae4470 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -646,7 +646,7 @@ static int find_num_cache_leaves(struct cpuinfo_x86 *c)
 	return i;
 }
 
-void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id)
+void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu)
 {
 	/*
 	 * We may have multiple LLCs if L3 caches exist, so check if we
@@ -657,7 +657,7 @@ void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id)
 
 	if (c->x86 < 0x17) {
 		/* LLC is at the node level. */
-		per_cpu(cpu_llc_id, cpu) = node_id;
+		per_cpu(cpu_llc_id, cpu) = c->node_id;
 	} else if (c->x86 == 0x17 && c->x86_model <= 0x1F) {
 		/*
 		 * LLC is at the core complex level.
@@ -684,7 +684,7 @@ void cacheinfo_amd_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id)
 	}
 }
 
-void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu, u8 node_id)
+void cacheinfo_hygon_init_llc_id(struct cpuinfo_x86 *c, int cpu)
 {
 	/*
 	 * We may have multiple LLCs if L3 caches exist, so check if we
diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
index ac6c30e5801d..604b0315211a 100644
--- a/arch/x86/kernel/cpu/hygon.c
+++ b/arch/x86/kernel/cpu/hygon.c
@@ -65,7 +65,6 @@ static void hygon_get_topology_early(struct cpuinfo_x86 *c)
  */
 static void hygon_get_topology(struct cpuinfo_x86 *c)
 {
-	u8 node_id;
 	int cpu = smp_processor_id();
 
 	/* get information required for multi-node processors */
@@ -75,7 +74,7 @@ static void hygon_get_topology(struct cpuinfo_x86 *c)
 
 		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
 
-		node_id  = ecx & 0xff;
+		c->node_id  = ecx & 0xff;
 
 		c->cpu_core_id = ebx & 0xff;
 
@@ -93,14 +92,14 @@ static void hygon_get_topology(struct cpuinfo_x86 *c)
 		/* Socket ID is ApicId[6] for these processors. */
 		c->phys_proc_id = c->apicid >> APICID_SOCKET_ID_BIT;
 
-		cacheinfo_hygon_init_llc_id(c, cpu, node_id);
+		cacheinfo_hygon_init_llc_id(c, cpu);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
-		node_id = value & 7;
+		c->node_id = value & 7;
 
-		per_cpu(cpu_llc_id, cpu) = node_id;
+		per_cpu(cpu_llc_id, cpu) = c->node_id;
 	} else
 		return;
 
@@ -123,7 +122,7 @@ static void hygon_detect_cmp(struct cpuinfo_x86 *c)
 	/* Convert the initial APIC ID into the socket ID */
 	c->phys_proc_id = c->initial_apicid >> bits;
 	/* use socket ID also for last level cache */
-	per_cpu(cpu_llc_id, cpu) = c->phys_proc_id;
+	per_cpu(cpu_llc_id, cpu) = c->node_id = c->phys_proc_id;
 }
 
 static void srat_detect_node(struct cpuinfo_x86 *c)
-- 
2.25.1

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Thu, Sep 03, 2020 at 08:01:37PM +0000, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> AMD systems provide a "NodeId" value that represents a global ID
> indicating to which "Node" a logical CPU belongs. The "Node" is a
> physical structure equivalent to a Die, and it should not be confused
> with logical structures like NUMA node.

So we said in Documentation/x86/topology.rst that:

"The kernel does not care about the concept of physical sockets because
a socket has no relevance to software. It's an electromechanical
component."

Now, you're talking, AFAIU, about physical components. Why do you need
them?

What is then:

  - cpuinfo_x86.phys_proc_id:

    The physical ID of the package. This information is retrieved via CPUID
    and deduced from the APIC IDs of the cores in the package.

supposed to mean?

Why isn't phys_proc_id != node_id?

And so on and so on.

Please get the nomenclature straight first and then we can talk changes.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

On Wed, Sep 09, 2020 at 08:06:47PM +0200, Borislav Petkov wrote:
> On Thu, Sep 03, 2020 at 08:01:37PM +0000, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > 
> > AMD systems provide a "NodeId" value that represents a global ID
> > indicating to which "Node" a logical CPU belongs. The "Node" is a
> > physical structure equivalent to a Die, and it should not be confused
> > with logical structures like NUMA node.
> 
> So we said in Documentation/x86/topology.rst that:
> 
> "The kernel does not care about the concept of physical sockets because
> a socket has no relevance to software. It's an electromechanical
> component."
> 

Yes, I agree with this.

> Now, you're talking, AFAIU, about physical components. Why do you need
> them?
> 

We need to access specific instances of hardware registers in the
Northbridge or Data Fabric. The code in arch/x86/kernel/amd_nb.c does
this.

> What is then:
> 
>   - cpuinfo_x86.phys_proc_id:
> 
>     The physical ID of the package. This information is retrieved via CPUID
>     and deduced from the APIC IDs of the cores in the package.
> 
> supposed to mean?
> 

Package = Socket, i.e. a field replaceable unit. Socket may not be
useful for software, but I think it helps users identify the hardware.

I think the following could be changed in the documentation:

"In the past a socket always contained a single package (see below), but
with the advent of Multi Chip Modules (MCM) a socket can hold more than one
package."

Replace "package" with "die".

You take multiple dies from the foundry and you "package" them together
into a single unit.

> Why isn't phys_proc_id != node_id?
> 

They could be equal depending on the system. The values are different on
MCM systems like Bulldozer and Naples though.

The functions and structures in amd_nb.c are indexed by the node_id.
This is done implicitly right now by using amd_get_nb_id()/cpu_llc_id.
But the LLC isn't always equal to the Node/Die like in Naples. So the
patches in this set save and explicitly use the node_id when needed.

What do you think?

Thanks,
Yazen

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Wed, Sep 09, 2020 at 03:17:55PM -0500, Yazen Ghannam wrote:
> We need to access specific instances of hardware registers in the
> Northbridge or Data Fabric. The code in arch/x86/kernel/amd_nb.c does
> this.

So you don't need the node_id - you need the northbridge/data fabric ID?
I'm guessing NB == DF, i.e., it was NB before Zen and it is DF now.

Yes?

> Package = Socket, i.e. a field replaceable unit. Socket may not be
> useful for software, but I think it helps users identify the hardware.
> 
> I think the following could be changed in the documentation:
> 
> "In the past a socket always contained a single package (see below), but
> with the advent of Multi Chip Modules (MCM) a socket can hold more than one
> package."
> 
> Replace "package" with "die".

So first of all, we have:

"AMD nomenclature for package is 'Node'."

so we either change that because as you explain, node != package on AMD.

What you need is the ID of that northbridge or data fabric instance,
AFAIU.

> You take multiple dies from the foundry and you "package" them together
> into a single unit.

I think you're overloading the word "package" here and that leads to
more confusion. Package in our definition - Linux' - is:

"Packages contain a number of cores plus shared resources, e.g. DRAM
controller, shared caches etc." If you glue several packages together,
you get an MCM.

> They could be equal depending on the system. The values are different on
> MCM systems like Bulldozer and Naples though.
> 
> The functions and structures in amd_nb.c are indexed by the node_id.
> This is done implicitly right now by using amd_get_nb_id()/cpu_llc_id.
> But the LLC isn't always equal to the Node/Die like in Naples. So the
> patches in this set save and explicitly use the node_id when needed.
> 
> What do you think?

Sounds to me that you want to ID that data fabric instance which
logically belongs to one or multiple packages. Or can a DF a single
package?

So let's start simple: how does a DF instance map to a logical NUMA
node or package? Can a DF serve multiple packages?

You could use the examples at the end of Documentation/x86/topology.rst
to explain how those things play together. And remember to not think
about the physical aspect of the hardware structure because it doesn't
mean anything to software. All you wanna do is address the proper DF
instance so this needs to be enumerable and properly represented by sw.

Confused?

I am.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

On Thu, Sep 10, 2020 at 12:14:43PM +0200, Borislav Petkov wrote:
> On Wed, Sep 09, 2020 at 03:17:55PM -0500, Yazen Ghannam wrote:
> > We need to access specific instances of hardware registers in the
> > Northbridge or Data Fabric. The code in arch/x86/kernel/amd_nb.c does
> > this.
> 
> So you don't need the node_id - you need the northbridge/data fabric ID?
> I'm guessing NB == DF, i.e., it was NB before Zen and it is DF now.
> 
> Yes?
>

Yes, that's right.

I called it "node_id" based on the AMD documentation and what it's
called today in the Linux code. It's called other things like nb_id and
nid too.

I think we can call it something else to avoid confusion with NUMA nodes
if that'll help.

> > Package = Socket, i.e. a field replaceable unit. Socket may not be
> > useful for software, but I think it helps users identify the hardware.
> > 
> > I think the following could be changed in the documentation:
> > 
> > "In the past a socket always contained a single package (see below), but
> > with the advent of Multi Chip Modules (MCM) a socket can hold more than one
> > package."
> > 
> > Replace "package" with "die".
> 
> So first of all, we have:
> 
> "AMD nomenclature for package is 'Node'."
> 
> so we either change that because as you explain, node != package on AMD.
> 
> What you need is the ID of that northbridge or data fabric instance,
> AFAIU.
> 
> > You take multiple dies from the foundry and you "package" them together
> > into a single unit.
> 
> I think you're overloading the word "package" here and that leads to
> more confusion. Package in our definition - Linux' - is:
> 
> "Packages contain a number of cores plus shared resources, e.g. DRAM
> controller, shared caches etc." If you glue several packages together,
> you get an MCM.
> 

Yes, you're right. The AMD documentation is different, so I'll try to
stick with the Linux documentation and qualify names with "AMD" when
noting the usage by the AMD docs.

> > They could be equal depending on the system. The values are different on
> > MCM systems like Bulldozer and Naples though.
> > 
> > The functions and structures in amd_nb.c are indexed by the node_id.
> > This is done implicitly right now by using amd_get_nb_id()/cpu_llc_id.
> > But the LLC isn't always equal to the Node/Die like in Naples. So the
> > patches in this set save and explicitly use the node_id when needed.
> > 
> > What do you think?
> 
> Sounds to me that you want to ID that data fabric instance which
> logically belongs to one or multiple packages. Or can a DF a single
> package?
> 
> So let's start simple: how does a DF instance map to a logical NUMA
> node or package? Can a DF serve multiple packages?
> 

There's one DF/NB per package and it's a fixed value, i.e. it shouldn't
change based on the NUMA configuration.

Here's an example of a 2 socket Naples system with 4 packages per socket
and setup to have 1 NUMA node. The "node_id" value is the AMD NodeId
from CPUID.

CPU=0 phys_proc_id=0 node_id=0 cpu_to_node()=0
CPU=8 phys_proc_id=0 node_id=1 cpu_to_node()=0
CPU=16 phys_proc_id=0 node_id=2 cpu_to_node()=0
CPU=24 phys_proc_id=0 node_id=3 cpu_to_node()=0
CPU=32 phys_proc_id=1 node_id=4 cpu_to_node()=0
CPU=40 phys_proc_id=1 node_id=5 cpu_to_node()=0
CPU=48 phys_proc_id=1 node_id=6 cpu_to_node()=0
CPU=56 phys_proc_id=1 node_id=7 cpu_to_node()=0

> You could use the examples at the end of Documentation/x86/topology.rst
> to explain how those things play together. And remember to not think
> about the physical aspect of the hardware structure because it doesn't
> mean anything to software. All you wanna do is address the proper DF
> instance so this needs to be enumerable and properly represented by sw.
>

Yeah, I think example 4b works here. The mismatch though is with
phys_proc_id and package on AMD systems. You can see above that
phys_proc_id gives a socket number, and the AMD NodeId gives a package
number.

Should we add a note under cpuinfo_x86.phys_proc_id to make this
distinction?

> Confused?
> 
> I am.
> 
> :-)
>

Yeah, me too. :)

Thanks,
Yazen

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Mon, Sep 14, 2020 at 02:20:39PM -0500, Yazen Ghannam wrote:
> Yes, that's right.
> 
> I called it "node_id" based on the AMD documentation and what it's
> called today in the Linux code. It's called other things like nb_id and
> nid too.
> 
> I think we can call it something else to avoid confusion with NUMA nodes
> if that'll help.

Yes, whatever we end up calling it, it should be added to that
documentation file I pointed you at. Because months and years from now,
it'll be the only place we look first, before changing the topology
again.

> Yes, you're right. The AMD documentation is different, so I'll try to
> stick with the Linux documentation and qualify names with "AMD" when
> noting the usage by the AMD docs.

Thanks, yes, because Linux is trying to map its view of the topology to
the vendor's and model all vendors properly, if possible.

> There's one DF/NB per package and it's a fixed value, i.e. it shouldn't
> change based on the NUMA configuration.

Aha, so the NB kinda serves the package and is part of it. That makes a
lot of sense and clears some confusion.

> Here's an example of a 2 socket Naples system with 4 packages per socket
> and setup to have 1 NUMA node. The "node_id" value is the AMD NodeId
> from CPUID.
> 
> CPU=0 phys_proc_id=0 node_id=0 cpu_to_node()=0
> CPU=8 phys_proc_id=0 node_id=1 cpu_to_node()=0
> CPU=16 phys_proc_id=0 node_id=2 cpu_to_node()=0
> CPU=24 phys_proc_id=0 node_id=3 cpu_to_node()=0
> CPU=32 phys_proc_id=1 node_id=4 cpu_to_node()=0
> CPU=40 phys_proc_id=1 node_id=5 cpu_to_node()=0
> CPU=48 phys_proc_id=1 node_id=6 cpu_to_node()=0
> CPU=56 phys_proc_id=1 node_id=7 cpu_to_node()=0

Ok, node_id is the DF instance number in this case.

> Yeah, I think example 4b works here. The mismatch though is with
> phys_proc_id and package on AMD systems. You can see above that
> phys_proc_id gives a socket number, and the AMD NodeId gives a package
> number.

Ok, now looka here:

"  - cpuinfo_x86.logical_proc_id:

    The logical ID of the package. As we do not trust BIOSes to enumerate the
    packages in a consistent way, we introduced the concept of logical package
    ID so we can sanely calculate the number of maximum possible packages in
    the system and have the packages enumerated linearly."

Doesn't that sound like exactly what you need?

Because that DF ID *is* practically the package ID as there's 1:1
mapping between DF and a package, as you say above.

Right?

Now, it says

[    7.670791] smpboot: Max logical packages: 2

on my Rome box but what you want sounds very much like the logical
package ID and if we define that on AMD to be that and document it this
way, I guess that should work too, provided there are no caveats like
sched is using this info for proper task placement and so on. That would
need code audit, of course...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

On Tue, Sep 15, 2020 at 10:35:15AM +0200, Borislav Petkov wrote:
...
> > Yeah, I think example 4b works here. The mismatch though is with
> > phys_proc_id and package on AMD systems. You can see above that
> > phys_proc_id gives a socket number, and the AMD NodeId gives a package
> > number.
> 
> Ok, now looka here:
> 
> "  - cpuinfo_x86.logical_proc_id:
> 
>     The logical ID of the package. As we do not trust BIOSes to enumerate the
>     packages in a consistent way, we introduced the concept of logical package
>     ID so we can sanely calculate the number of maximum possible packages in
>     the system and have the packages enumerated linearly."
> 
> Doesn't that sound like exactly what you need?
> 
> Because that DF ID *is* practically the package ID as there's 1:1
> mapping between DF and a package, as you say above.
> 
> Right?
> 
> Now, it says
> 
> [    7.670791] smpboot: Max logical packages: 2
> 
> on my Rome box but what you want sounds very much like the logical
> package ID and if we define that on AMD to be that and document it this
> way, I guess that should work too, provided there are no caveats like
> sched is using this info for proper task placement and so on. That would
> need code audit, of course...
>

The only use of logical_proc_id seems to be in hswep_uncore_cpu_init().
So I think maybe we can use this.

However, I think there are two issues.

1) The logical_proc_id seems like it should refer to the same type of
structure as phys_proc_id. In our case, this won't be true as
phys_proc_id would refer to the "socket" on AMD and logical_proc_id
would refer to the package/AMD NodeId.

2) The AMD NodeId is read during c_init()/init_amd(), so logical_proc_id
can be set here. But then logical_proc_id will get overwritten later in 
topology_update_package_map(). I don't know if it'd be good to modify
the generic flow to support this vendor-specific behavior.

What do you think?

Thanks,
Yazen

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Wed, Sep 16, 2020 at 02:51:52PM -0500, Yazen Ghannam wrote:
> What do you think?

Yeah, forget logical_proc_id - the galactic senate of x86 maintainers
said that we're keeping that for when BIOS vendors f*ck up with the
phys_proc_id enumeration on AMD. Then we'll need that as a workaround.

Look instead at:

struct cpuinfo_x86 {

	...

        u16                     cpu_die_id;
        u16                     logical_die_id;

and

7745f03eb395 ("x86/topology: Add CPUID.1F multi-die/package support")

"Some new systems have multiple software-visible die within each
package."

and you could map the AMD packages to those dies. And if you guys
implement CPUID.1F to enumerate those packages the same way, then all
should just work (famous last words).

Because Intel dies is basically AMD packages consisting of a CCX, caches
and DF.

We would have to update the documentation in the end to denote that but
let's see if this should work for you too first. Because the concepts
sound very similar, if not identical...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

On Thu, Sep 17, 2020 at 12:37:20PM +0200, Borislav Petkov wrote:
> On Wed, Sep 16, 2020 at 02:51:52PM -0500, Yazen Ghannam wrote:
> > What do you think?
> 
> Yeah, forget logical_proc_id - the galactic senate of x86 maintainers
> said that we're keeping that for when BIOS vendors f*ck up with the
> phys_proc_id enumeration on AMD. Then we'll need that as a workaround.
> 
> Look instead at:
> 
> struct cpuinfo_x86 {
> 
> 	...
> 
>         u16                     cpu_die_id;
>         u16                     logical_die_id;
> 
> and
> 
> 7745f03eb395 ("x86/topology: Add CPUID.1F multi-die/package support")
> 
> "Some new systems have multiple software-visible die within each
> package."
> 
> and you could map the AMD packages to those dies. And if you guys
> implement CPUID.1F to enumerate those packages the same way, then all
> should just work (famous last words).
>
> Because Intel dies is basically AMD packages consisting of a CCX, caches
> and DF.
> 
> We would have to update the documentation in the end to denote that but
> let's see if this should work for you too first. Because the concepts
> sound very similar, if not identical...
>

Yep, we could ask the hardware folks to implement CPUID Leaf 0x1F, but
that'll be in some future products. 

I actually tried using cpu_die_id, but I ran into an issue on newer
systems.

On older systems, there is no CPUID Leaf 0xB or 0x1F, and cpu_die_id
doesn't get explicitly set. So setting cpu_die_id equal to AMD NodeId
would work. But newer systems support CPUID Leaf 0xB, so cpu_die_id
will get explicitly set by detect_extended_topology(). The value set is
different from the AMD NodeId. And at that point I shied away from
doing any override or fixup.

Thanks,
Yazen

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Thu, Sep 17, 2020 at 11:20:53AM -0500, Yazen Ghannam wrote:
> But newer systems support CPUID Leaf 0xB, so cpu_die_id will get
> explicitly set by detect_extended_topology(). The value set is
> different from the AMD NodeId. And at that point I shied away from
> doing any override or fixup.

Well, different how? Can you extract the node_id you need
from CPUID(0xb)? If yes, we can do an AMD-specific branch in
detect_extended_topology() but that better be future proof.

IOW, is information from CPUID(0xb) ever going to be needed in the
kernel?

Also, and independently, if its definition do not give you the
node_id you need, then you can just as well overwrite ->cpu_die_id in
detect_extended_topology() because that value - whatever that is, could
be garbage, just as well - is wrong on AMD anyway.

So it would be a fix for the leaf parsing, regardless of whether you
need it or not.

Makes sense?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Yazen Ghannam]

On Thu, Sep 17, 2020 at 06:40:48PM +0200, Borislav Petkov wrote:
> On Thu, Sep 17, 2020 at 11:20:53AM -0500, Yazen Ghannam wrote:
> > But newer systems support CPUID Leaf 0xB, so cpu_die_id will get
> > explicitly set by detect_extended_topology(). The value set is
> > different from the AMD NodeId. And at that point I shied away from
> > doing any override or fixup.
> 
> Well, different how? Can you extract the node_id you need
> from CPUID(0xb)? If yes, we can do an AMD-specific branch in
> detect_extended_topology() but that better be future proof.
> 
> IOW, is information from CPUID(0xb) ever going to be needed in the
> kernel?
> 
> Also, and independently, if its definition do not give you the
> node_id you need, then you can just as well overwrite ->cpu_die_id in
> detect_extended_topology() because that value - whatever that is, could
> be garbage, just as well - is wrong on AMD anyway.
> 
> So it would be a fix for the leaf parsing, regardless of whether you
> need it or not.
> 
> Makes sense?
>

Yes, I think so. "Die" is not defined in CPUID(0xb), only SMT and Core,
so the cpu_die_id value is not valid. In which case, we can overwrite
it. CPUID(0xb) doesn't have anything equivalent to AMD NodeId. So on
systems with CPUID < 0x1F, we should be okay with using cpu_die_id equal
to AMD NodeId.

I have an idea on what to do, so I'll send another rev if that's okay.
Do you have any comments on the other patches in the set?

Thanks,
Yazen

Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems

[Borislav Petkov]

On Thu, Sep 17, 2020 at 02:44:25PM -0500, Yazen Ghannam wrote:
> Yes, I think so. "Die" is not defined in CPUID(0xb), only SMT and Core,
> so the cpu_die_id value is not valid.

Right.

> In which case, we can overwrite it. CPUID(0xb) doesn't have anything
> equivalent to AMD NodeId. So on systems with CPUID < 0x1F, we should
> be okay with using cpu_die_id equal to AMD NodeId.

Right.

> I have an idea on what to do, so I'll send another rev if that's okay.
> Do you have any comments on the other patches in the set?

Lemme go through them these days.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 2/8] x86/CPU/AMD: Remove amd_get_nb_id()

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The Last Level Cache ID is returned by amd_get_nb_id(). In practice,
this value is the same as the AMD NodeId for callers of this function.
The NodeId is saved in struct cpuinfo_x86.node_id.

Replace calls to amd_get_nb_id() with the logical CPU's node_id and
remove the function.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-2-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch.

 arch/x86/events/amd/core.c       | 2 +-
 arch/x86/include/asm/processor.h | 2 --
 arch/x86/kernel/amd_nb.c         | 4 ++--
 arch/x86/kernel/cpu/amd.c        | 6 ------
 arch/x86/kernel/cpu/cacheinfo.c  | 2 +-
 arch/x86/kernel/cpu/mce/amd.c    | 4 ++--
 arch/x86/kernel/cpu/mce/inject.c | 4 ++--
 drivers/edac/amd64_edac.c        | 4 ++--
 drivers/edac/mce_amd.c           | 2 +-
 9 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 39eb276d0277..01b9b943dcf4 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -538,7 +538,7 @@ static void amd_pmu_cpu_starting(int cpu)
 	if (!x86_pmu.amd_nb_constraints)
 		return;
 
-	nb_id = amd_get_nb_id(cpu);
+	nb_id = cpu_data(cpu).node_id;
 	WARN_ON_ONCE(nb_id == BAD_APICID);
 
 	for_each_online_cpu(i) {
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index a776b7886ec0..408977a323d3 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -871,10 +871,8 @@ extern int set_tsc_mode(unsigned int val);
 DECLARE_PER_CPU(u64, msr_misc_features_shadow);
 
 #ifdef CONFIG_CPU_SUP_AMD
-extern u16 amd_get_nb_id(int cpu);
 extern u32 amd_get_nodes_per_socket(void);
 #else
-static inline u16 amd_get_nb_id(int cpu)		{ return 0; }
 static inline u32 amd_get_nodes_per_socket(void)	{ return 0; }
 #endif
 
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 18f6b7c4bd79..2bd8abdbed8e 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -384,7 +384,7 @@ struct resource *amd_get_mmconfig_range(struct resource *res)
 
 int amd_get_subcaches(int cpu)
 {
-	struct pci_dev *link = node_to_amd_nb(amd_get_nb_id(cpu))->link;
+	struct pci_dev *link = node_to_amd_nb(cpu_data(cpu).node_id)->link;
 	unsigned int mask;
 
 	if (!amd_nb_has_feature(AMD_NB_L3_PARTITIONING))
@@ -398,7 +398,7 @@ int amd_get_subcaches(int cpu)
 int amd_set_subcaches(int cpu, unsigned long mask)
 {
 	static unsigned int reset, ban;
-	struct amd_northbridge *nb = node_to_amd_nb(amd_get_nb_id(cpu));
+	struct amd_northbridge *nb = node_to_amd_nb(cpu_data(cpu).node_id);
 	unsigned int reg;
 	int cuid;
 
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 5eef4cc1e5b7..846367a69c4a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -424,12 +424,6 @@ static void amd_detect_ppin(struct cpuinfo_x86 *c)
 	clear_cpu_cap(c, X86_FEATURE_AMD_PPIN);
 }
 
-u16 amd_get_nb_id(int cpu)
-{
-	return per_cpu(cpu_llc_id, cpu);
-}
-EXPORT_SYMBOL_GPL(amd_get_nb_id);
-
 u32 amd_get_nodes_per_socket(void)
 {
 	return nodes_per_socket;
diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
index 81dfddae4470..8e34e90bb872 100644
--- a/arch/x86/kernel/cpu/cacheinfo.c
+++ b/arch/x86/kernel/cpu/cacheinfo.c
@@ -580,7 +580,7 @@ static void amd_init_l3_cache(struct _cpuid4_info_regs *this_leaf, int index)
 	if (index < 3)
 		return;
 
-	node = amd_get_nb_id(smp_processor_id());
+	node = cpu_data(smp_processor_id()).node_id;
 	this_leaf->nb = node_to_amd_nb(node);
 	if (this_leaf->nb && !this_leaf->nb->l3_cache.indices)
 		amd_calc_l3_indices(this_leaf->nb);
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 0c6b02dd744c..be96f77004ad 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -1341,7 +1341,7 @@ static int threshold_create_bank(struct threshold_bank **bp, unsigned int cpu,
 		return -ENODEV;
 
 	if (is_shared_bank(bank)) {
-		nb = node_to_amd_nb(amd_get_nb_id(cpu));
+		nb = node_to_amd_nb(cpu_data(cpu).node_id);
 
 		/* threshold descriptor already initialized on this node? */
 		if (nb && nb->bank4) {
@@ -1445,7 +1445,7 @@ static void threshold_remove_bank(struct threshold_bank *bank)
 		 * The last CPU on this node using the shared bank is going
 		 * away, remove that bank now.
 		 */
-		nb = node_to_amd_nb(amd_get_nb_id(smp_processor_id()));
+		nb = node_to_amd_nb(cpu_data(smp_processor_id()).node_id);
 		nb->bank4 = NULL;
 	}
 
diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
index 3a44346f2276..ba491134c326 100644
--- a/arch/x86/kernel/cpu/mce/inject.c
+++ b/arch/x86/kernel/cpu/mce/inject.c
@@ -522,8 +522,8 @@ static void do_inject(void)
 	if (boot_cpu_has(X86_FEATURE_AMD_DCM) &&
 	    b == 4 &&
 	    boot_cpu_data.x86 < 0x17) {
-		toggle_nb_mca_mst_cpu(amd_get_nb_id(cpu));
-		cpu = get_nbc_for_node(amd_get_nb_id(cpu));
+		toggle_nb_mca_mst_cpu(cpu_data(cpu).node_id);
+		cpu = get_nbc_for_node(cpu_data(cpu).node_id);
 	}
 
 	get_online_cpus();
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index fcc08bbf6945..3f91cac00fb2 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1133,7 +1133,7 @@ static int k8_early_channel_count(struct amd64_pvt *pvt)
 /* On F10h and later ErrAddr is MC4_ADDR[47:1] */
 static u64 get_error_address(struct amd64_pvt *pvt, struct mce *m)
 {
-	u16 mce_nid = amd_get_nb_id(m->extcpu);
+	u16 mce_nid = cpu_data(m->extcpu).node_id;
 	struct mem_ctl_info *mci;
 	u8 start_bit = 1;
 	u8 end_bit   = 47;
@@ -3046,7 +3046,7 @@ static void get_cpus_on_this_dct_cpumask(struct cpumask *mask, u16 nid)
 	int cpu;
 
 	for_each_online_cpu(cpu)
-		if (amd_get_nb_id(cpu) == nid)
+		if (cpu_data(cpu).node_id == nid)
 			cpumask_set_cpu(cpu, mask);
 }
 
diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index 7f28edb070bd..ac9bd74c92cd 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -869,7 +869,7 @@ static void decode_mc3_mce(struct mce *m)
 static void decode_mc4_mce(struct mce *m)
 {
 	unsigned int fam = x86_family(m->cpuid);
-	int node_id = amd_get_nb_id(m->extcpu);
+	int node_id = cpu_data(m->extcpu).node_id;
 	u16 ec = EC(m->status);
 	u8 xec = XEC(m->status, 0x1f);
 	u8 offset = 0;
-- 
2.25.1

[PATCH v2 3/8] EDAC/mce_amd: Use struct cpuinfo_x86.node_id for NodeId

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The edac_mce_amd module calls decode_dram_ecc() on AMD Family17h and
later systems. This function is used in amd64_edac_mod to do
system-specific decoding for DRAM ECC errors. The function takes a
"NodeId" as a parameter.

In AMD documentation, NodeId is used to identify a physical die in a
system. This can be used to identify a node in the AMD_NB code and also
it is used with umc_normaddr_to_sysaddr().

However, the input used for decode_dram_ecc() is currently the NUMA node
of a logical CPU. In the default configuration, the NUMA node and
physical die will be equivalent, so this doesn't have an impact. But the
NUMA node configuration can be adjusted with optional memory
interleaving modes. This will cause the NUMA node enumeration to not
match the physical die enumeration. The mismatch will cause the address
translation function to fail or report incorrect results.

Use struct cpuinfo_x86.node_id for the node_id parameter to ensure the
physical ID is used.

Fixes: fbe63acf62f5 ("EDAC, mce_amd: Use cpu_to_node() to find the node ID")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-2-Yazen.Ghannam@amd.com

v1 -> v2:
* Redo based on change in Patch 1.

 drivers/edac/mce_amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/mce_amd.c b/drivers/edac/mce_amd.c
index ac9bd74c92cd..91b5e3e0744e 100644
--- a/drivers/edac/mce_amd.c
+++ b/drivers/edac/mce_amd.c
@@ -1003,7 +1003,7 @@ static void decode_smca_error(struct mce *m)
 		pr_cont(", %s.\n", smca_mce_descs[bank_type].descs[xec]);
 
 	if (bank_type == SMCA_UMC && xec == 0 && decode_dram_ecc)
-		decode_dram_ecc(cpu_to_node(m->extcpu), m);
+		decode_dram_ecc(cpu_data(m->extcpu).node_id, m);
 }
 
 static inline void amd_decode_err_code(u16 ec)
-- 
2.25.1

[PATCH v2 4/8] x86/MCE/AMD: Use defines for register addresses in translation code

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Replace raw register offset values in the AMD address translation code
with named definitions.

Also, drop comments that only note the register names.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on comments for v1 Patch 2.

 arch/x86/kernel/cpu/mce/amd.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index be96f77004ad..1e0510fd5afc 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -675,6 +675,14 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 		deferred_error_interrupt_enable(c);
 }
 
+#define DF_F0_FABRICINSTINFO3	0x50
+#define DF_F0_MMIOHOLE		0x104
+#define DF_F0_DRAMBASEADDR	0x110
+#define DF_F0_DRAMLIMITADDR	0x114
+#define DF_F0_DRAMOFFSET	0x1B4
+
+#define DF_F1_SYSFABRICID	0x208
+
 int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 {
 	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
@@ -691,22 +699,21 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	u8 cs_mask, cs_id = 0;
 	bool hash_enabled = false;
 
-	/* Read D18F0x1B4 (DramOffset), check if base 1 is used. */
-	if (amd_df_indirect_read(nid, 0, 0x1B4, umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &tmp))
 		goto out_err;
 
 	/* Remove HiAddrOffset from normalized address, if enabled: */
 	if (tmp & BIT(0)) {
 		u64 hi_addr_offset = (tmp & GENMASK_ULL(31, 20)) << 8;
 
+		/* Check if base 1 is used. */
 		if (norm_addr >= hi_addr_offset) {
 			ret_addr -= hi_addr_offset;
 			base = 1;
 		}
 	}
 
-	/* Read D18F0x110 (DramBaseAddress). */
-	if (amd_df_indirect_read(nid, 0, 0x110 + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMBASEADDR + (8 * base), umc, &tmp))
 		goto out_err;
 
 	/* Check if address range is valid. */
@@ -728,8 +735,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
-	/* Read D18F0x114 (DramLimitAddress). */
-	if (amd_df_indirect_read(nid, 0, 0x114 + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &tmp))
 		goto out_err;
 
 	intlv_num_sockets = (tmp >> 8) & 0x1;
@@ -780,12 +786,11 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		u8 die_id_bit, sock_id_bit, cs_fabric_id;
 
 		/*
-		 * Read FabricBlockInstanceInformation3_CS[BlockFabricID].
 		 * This is the fabric id for this coherent slave. Use
 		 * umc/channel# as instance id of the coherent slave
 		 * for FICAA.
 		 */
-		if (amd_df_indirect_read(nid, 0, 0x50, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &tmp))
 			goto out_err;
 
 		cs_fabric_id = (tmp >> 8) & 0xFF;
@@ -800,9 +805,8 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 		sock_id_bit = die_id_bit;
 
-		/* Read D18F1x208 (SystemFabricIdMask). */
 		if (intlv_num_dies || intlv_num_sockets)
-			if (amd_df_indirect_read(nid, 1, 0x208, umc, &tmp))
+			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &tmp))
 				goto out_err;
 
 		/* If interleaved over more than 1 die. */
@@ -841,7 +845,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	/* If legacy MMIO hole enabled */
 	if (lgcy_mmio_hole_en) {
-		if (amd_df_indirect_read(nid, 0, 0x104, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_MMIOHOLE, umc, &tmp))
 			goto out_err;
 
 		dram_hole_base = tmp & GENMASK(31, 24);
-- 
2.25.1

[PATCH v2 5/8] x86/MCE/AMD: Use macros to get bitfields in translation code

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Define macros to get individual bits and bitfields. Use these to make
the code more readable.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on comments for v1 Patch 2.

 arch/x86/kernel/cpu/mce/amd.c | 46 +++++++++++++++++------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 1e0510fd5afc..90c3ad61ae19 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -675,6 +675,9 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 		deferred_error_interrupt_enable(c);
 }
 
+#define get_bits(x, msb, lsb)	((x & GENMASK_ULL(msb, lsb)) >> lsb)
+#define get_bit(x, bit)		((x >> bit) & BIT(0))
+
 #define DF_F0_FABRICINSTINFO3	0x50
 #define DF_F0_MMIOHOLE		0x104
 #define DF_F0_DRAMBASEADDR	0x110
@@ -704,7 +707,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	/* Remove HiAddrOffset from normalized address, if enabled: */
 	if (tmp & BIT(0)) {
-		u64 hi_addr_offset = (tmp & GENMASK_ULL(31, 20)) << 8;
+		u64 hi_addr_offset = get_bits(tmp, 31, 20) << 28;
 
 		/* Check if base 1 is used. */
 		if (norm_addr >= hi_addr_offset) {
@@ -723,10 +726,10 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
-	lgcy_mmio_hole_en = tmp & BIT(1);
-	intlv_num_chan	  = (tmp >> 4) & 0xF;
-	intlv_addr_sel	  = (tmp >> 8) & 0x7;
-	dram_base_addr	  = (tmp & GENMASK_ULL(31, 12)) << 16;
+	lgcy_mmio_hole_en = get_bit(tmp, 1);
+	intlv_num_chan	  = get_bits(tmp, 7, 4);
+	intlv_addr_sel	  = get_bits(tmp, 10, 8);
+	dram_base_addr	  = get_bits(tmp, 31, 12) << 28;
 
 	/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
 	if (intlv_addr_sel > 3) {
@@ -738,9 +741,9 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &tmp))
 		goto out_err;
 
-	intlv_num_sockets = (tmp >> 8) & 0x1;
-	intlv_num_dies	  = (tmp >> 10) & 0x3;
-	dram_limit_addr	  = ((tmp & GENMASK_ULL(31, 12)) << 16) | GENMASK_ULL(27, 0);
+	intlv_num_sockets = get_bit(tmp, 8);
+	intlv_num_dies	  = get_bits(tmp, 11, 10);
+	dram_limit_addr	  = (get_bits(tmp, 31, 12) << 28) | GENMASK_ULL(27, 0);
 
 	intlv_addr_bit = intlv_addr_sel + 8;
 
@@ -793,7 +796,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &tmp))
 			goto out_err;
 
-		cs_fabric_id = (tmp >> 8) & 0xFF;
+		cs_fabric_id = get_bits(tmp, 15, 8);
 		die_id_bit   = 0;
 
 		/* If interleaved over more than 1 channel: */
@@ -812,16 +815,16 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		/* If interleaved over more than 1 die. */
 		if (intlv_num_dies) {
 			sock_id_bit  = die_id_bit + intlv_num_dies;
-			die_id_shift = (tmp >> 24) & 0xF;
-			die_id_mask  = (tmp >> 8) & 0xFF;
+			die_id_shift = get_bits(tmp, 27, 24);
+			die_id_mask  = get_bits(tmp, 15, 8);
 
 			cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
 		}
 
 		/* If interleaved over more than 1 socket. */
 		if (intlv_num_sockets) {
-			socket_id_shift	= (tmp >> 28) & 0xF;
-			socket_id_mask	= (tmp >> 16) & 0xFF;
+			socket_id_shift	= get_bits(tmp, 31, 28);
+			socket_id_mask	= get_bits(tmp, 23, 16);
 
 			cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
 		}
@@ -834,7 +837,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		 * bits there are. "intlv_addr_bit" tells us how many "Y" bits
 		 * there are (where "I" starts).
 		 */
-		temp_addr_y = ret_addr & GENMASK_ULL(intlv_addr_bit-1, 0);
+		temp_addr_y = get_bits(ret_addr, intlv_addr_bit-1, 0);
 		temp_addr_i = (cs_id << intlv_addr_bit);
 		temp_addr_x = (ret_addr & GENMASK_ULL(63, intlv_addr_bit)) << num_intlv_bits;
 		ret_addr    = temp_addr_x | temp_addr_i | temp_addr_y;
@@ -854,16 +857,13 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	}
 
 	if (hash_enabled) {
-		/* Save some parentheses and grab ls-bit at the end. */
-		hashed_bit =	(ret_addr >> 12) ^
-				(ret_addr >> 18) ^
-				(ret_addr >> 21) ^
-				(ret_addr >> 30) ^
-				cs_id;
-
-		hashed_bit &= BIT(0);
+		hashed_bit =	get_bit(ret_addr, 12) ^
+				get_bit(ret_addr, 18) ^
+				get_bit(ret_addr, 21) ^
+				get_bit(ret_addr, 30) ^
+				get_bit(cs_id, 0);
 
-		if (hashed_bit != ((ret_addr >> intlv_addr_bit) & BIT(0)))
+		if (hashed_bit != get_bit(ret_addr, intlv_addr_bit))
 			ret_addr ^= BIT(intlv_addr_bit);
 	}
 
-- 
2.25.1

Re: [PATCH v2 5/8] x86/MCE/AMD: Use macros to get bitfields in translation code

[Borislav Petkov]

On Thu, Sep 03, 2020 at 08:01:41PM +0000, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Define macros to get individual bits and bitfields. Use these to make
> the code more readable.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
> Link:
> https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com
> 
> v1 -> v2:
> * New patch based on comments for v1 Patch 2.
> 
>  arch/x86/kernel/cpu/mce/amd.c | 46 +++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
> index 1e0510fd5afc..90c3ad61ae19 100644
> --- a/arch/x86/kernel/cpu/mce/amd.c
> +++ b/arch/x86/kernel/cpu/mce/amd.c
> @@ -675,6 +675,9 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
>  		deferred_error_interrupt_enable(c);
>  }
>  
> +#define get_bits(x, msb, lsb)	((x & GENMASK_ULL(msb, lsb)) >> lsb)
> +#define get_bit(x, bit)		((x >> bit) & BIT(0))
> +
>  #define DF_F0_FABRICINSTINFO3	0x50
>  #define DF_F0_MMIOHOLE		0x104
>  #define DF_F0_DRAMBASEADDR	0x110
> @@ -704,7 +707,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  
>  	/* Remove HiAddrOffset from normalized address, if enabled: */
>  	if (tmp & BIT(0)) {
> -		u64 hi_addr_offset = (tmp & GENMASK_ULL(31, 20)) << 8;
> +		u64 hi_addr_offset = get_bits(tmp, 31, 20) << 28;
>  
>  		/* Check if base 1 is used. */
>  		if (norm_addr >= hi_addr_offset) {
> @@ -723,10 +726,10 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
>  		goto out_err;
>  	}
>  
> -	lgcy_mmio_hole_en = tmp & BIT(1);
> -	intlv_num_chan	  = (tmp >> 4) & 0xF;
> -	intlv_addr_sel	  = (tmp >> 8) & 0x7;
> -	dram_base_addr	  = (tmp & GENMASK_ULL(31, 12)) << 16;
> +	lgcy_mmio_hole_en = get_bit(tmp, 1);
> +	intlv_num_chan	  = get_bits(tmp, 7, 4);
> +	intlv_addr_sel	  = get_bits(tmp, 10, 8);
> +	dram_base_addr	  = get_bits(tmp, 31, 12) << 28;

I can't say that those macros make it more readable. Now I have to go
lookup what the arguments are. I guess I can imagine what the msb and
lsb is but meh...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 6/8] x86/MCE/AMD: Drop tmp variable in translation code

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Remove the "tmp" variable used to save register values. Save the values
in existing variables, if possible.

The register values are 32 bits. Use separate "reg_" variables to hold
the register values if the existing variable sizes doesn't match, or if
no bitfields in a register share the same name as the register.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on comments for v1 Patch 2.

 arch/x86/kernel/cpu/mce/amd.c | 56 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 90c3ad61ae19..5a18937ff7cd 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -688,11 +688,14 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 {
-	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
 	/* We start from the normalized address */
 	u64 ret_addr = norm_addr;
 
-	u32 tmp;
+	u64 dram_base_addr, dram_limit_addr;
+	u32 dram_hole_base;
+
+	u32 reg_dram_base_addr, reg_dram_limit_addr;
+	u32 reg_dram_offset;
 
 	u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
 	u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
@@ -702,12 +705,12 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	u8 cs_mask, cs_id = 0;
 	bool hash_enabled = false;
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &reg_dram_offset))
 		goto out_err;
 
 	/* Remove HiAddrOffset from normalized address, if enabled: */
-	if (tmp & BIT(0)) {
-		u64 hi_addr_offset = get_bits(tmp, 31, 20) << 28;
+	if (reg_dram_offset & BIT(0)) {
+		u64 hi_addr_offset = get_bits(reg_dram_offset, 31, 20) << 28;
 
 		/* Check if base 1 is used. */
 		if (norm_addr >= hi_addr_offset) {
@@ -716,20 +719,20 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		}
 	}
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMBASEADDR + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMBASEADDR + (8 * base), umc, &reg_dram_base_addr))
 		goto out_err;
 
 	/* Check if address range is valid. */
-	if (!(tmp & BIT(0))) {
+	if (!(reg_dram_base_addr & BIT(0))) {
 		pr_err("%s: Invalid DramBaseAddress range: 0x%x.\n",
-			__func__, tmp);
+			__func__, reg_dram_base_addr);
 		goto out_err;
 	}
 
-	lgcy_mmio_hole_en = get_bit(tmp, 1);
-	intlv_num_chan	  = get_bits(tmp, 7, 4);
-	intlv_addr_sel	  = get_bits(tmp, 10, 8);
-	dram_base_addr	  = get_bits(tmp, 31, 12) << 28;
+	lgcy_mmio_hole_en = get_bit(reg_dram_base_addr, 1);
+	intlv_num_chan	  = get_bits(reg_dram_base_addr, 7, 4);
+	intlv_addr_sel	  = get_bits(reg_dram_base_addr, 10, 8);
+	dram_base_addr	  = get_bits(reg_dram_base_addr, 31, 12) << 28;
 
 	/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
 	if (intlv_addr_sel > 3) {
@@ -738,12 +741,12 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &reg_dram_limit_addr))
 		goto out_err;
 
-	intlv_num_sockets = get_bit(tmp, 8);
-	intlv_num_dies	  = get_bits(tmp, 11, 10);
-	dram_limit_addr	  = (get_bits(tmp, 31, 12) << 28) | GENMASK_ULL(27, 0);
+	intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
+	intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
+	dram_limit_addr	  = (get_bits(reg_dram_limit_addr, 31, 12) << 28) | GENMASK_ULL(27, 0);
 
 	intlv_addr_bit = intlv_addr_sel + 8;
 
@@ -786,17 +789,18 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	if (num_intlv_bits > 0) {
 		u64 temp_addr_x, temp_addr_i, temp_addr_y;
-		u8 die_id_bit, sock_id_bit, cs_fabric_id;
+		u32 reg_sys_fabric_id, cs_fabric_id;
+		u8 die_id_bit, sock_id_bit;
 
 		/*
 		 * This is the fabric id for this coherent slave. Use
 		 * umc/channel# as instance id of the coherent slave
 		 * for FICAA.
 		 */
-		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &cs_fabric_id))
 			goto out_err;
 
-		cs_fabric_id = get_bits(tmp, 15, 8);
+		cs_fabric_id = get_bits(cs_fabric_id, 15, 8);
 		die_id_bit   = 0;
 
 		/* If interleaved over more than 1 channel: */
@@ -809,22 +813,22 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		sock_id_bit = die_id_bit;
 
 		if (intlv_num_dies || intlv_num_sockets)
-			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &tmp))
+			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &reg_sys_fabric_id))
 				goto out_err;
 
 		/* If interleaved over more than 1 die. */
 		if (intlv_num_dies) {
 			sock_id_bit  = die_id_bit + intlv_num_dies;
-			die_id_shift = get_bits(tmp, 27, 24);
-			die_id_mask  = get_bits(tmp, 15, 8);
+			die_id_shift = get_bits(reg_sys_fabric_id, 27, 24);
+			die_id_mask  = get_bits(reg_sys_fabric_id, 15, 8);
 
 			cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
 		}
 
 		/* If interleaved over more than 1 socket. */
 		if (intlv_num_sockets) {
-			socket_id_shift	= get_bits(tmp, 31, 28);
-			socket_id_mask	= get_bits(tmp, 23, 16);
+			socket_id_shift	= get_bits(reg_sys_fabric_id, 31, 28);
+			socket_id_mask	= get_bits(reg_sys_fabric_id, 23, 16);
 
 			cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
 		}
@@ -848,10 +852,10 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	/* If legacy MMIO hole enabled */
 	if (lgcy_mmio_hole_en) {
-		if (amd_df_indirect_read(nid, 0, DF_F0_MMIOHOLE, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_MMIOHOLE, umc, &dram_hole_base))
 			goto out_err;
 
-		dram_hole_base = tmp & GENMASK(31, 24);
+		dram_hole_base &= GENMASK(31, 24);
 		if (ret_addr >= dram_hole_base)
 			ret_addr += (BIT_ULL(32) - dram_hole_base);
 	}
-- 
2.25.1

Re: [PATCH v2 6/8] x86/MCE/AMD: Drop tmp variable in translation code

[Borislav Petkov]

On Thu, Sep 03, 2020 at 08:01:42PM +0000, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Remove the "tmp" variable used to save register values. Save the values
> in existing variables, if possible.
> 
> The register values are 32 bits. Use separate "reg_" variables to hold
> the register values if the existing variable sizes doesn't match, or if
> no bitfields in a register share the same name as the register.

So I'm missing the "why" in the commit message. Why are you doing this?

Is there some reason which I'll find out later? If not, then this is
just unnecessary churn.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 6/8] x86/MCE/AMD: Drop tmp variable in translation code

[Yazen Ghannam]

On Wed, Sep 23, 2020 at 10:05:56AM +0200, Borislav Petkov wrote:
> On Thu, Sep 03, 2020 at 08:01:42PM +0000, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > 
> > Remove the "tmp" variable used to save register values. Save the values
> > in existing variables, if possible.
> > 
> > The register values are 32 bits. Use separate "reg_" variables to hold
> > the register values if the existing variable sizes doesn't match, or if
> > no bitfields in a register share the same name as the register.
> 
> So I'm missing the "why" in the commit message. Why are you doing this?
> 
> Is there some reason which I'll find out later? If not, then this is
> just unnecessary churn.
>

I don't have a strong reason other than trying to address a comment in
the first version. I can drop this patch if you prefer.

Thanks,
Yazen

[PATCH v2 7/8] x86/MCE/AMD: Group register reads in translation code

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

...so that bitfield extraction can be done together to simplify future
patches.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on comments for v1 Patch 2.

 arch/x86/kernel/cpu/mce/amd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 5a18937ff7cd..f5440f8000e9 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -729,11 +729,18 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &reg_dram_limit_addr))
+		goto out_err;
+
 	lgcy_mmio_hole_en = get_bit(reg_dram_base_addr, 1);
 	intlv_num_chan	  = get_bits(reg_dram_base_addr, 7, 4);
 	intlv_addr_sel	  = get_bits(reg_dram_base_addr, 10, 8);
 	dram_base_addr	  = get_bits(reg_dram_base_addr, 31, 12) << 28;
 
+	intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
+	intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
+	dram_limit_addr	  = (get_bits(reg_dram_limit_addr, 31, 12) << 28) | GENMASK_ULL(27, 0);
+
 	/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
 	if (intlv_addr_sel > 3) {
 		pr_err("%s: Invalid interleave address select %d.\n",
@@ -741,13 +748,6 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &reg_dram_limit_addr))
-		goto out_err;
-
-	intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
-	intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
-	dram_limit_addr	  = (get_bits(reg_dram_limit_addr, 31, 12) << 28) | GENMASK_ULL(27, 0);
-
 	intlv_addr_bit = intlv_addr_sel + 8;
 
 	/* Re-use intlv_num_chan by setting it equal to log2(#channels) */
-- 
2.25.1

[PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Yazen Ghannam]

From: Muralidhara M K <muralidhara.mk@amd.com>

Add support for new memory interleaving modes used in current AMD systems.

Check if the system is using a current Data Fabric version or a legacy
version as some bit and register definitions have changed.

Tested on AMD reference platforms with the following memory interleaving
options.

Naples
- None
- Channel
- Die
- Socket

Rome (NPS = Nodes per Socket)
- None
- NPS0
- NPS1
- NPS2
- NPS4

The fixes tag refers to the commit that allows amd64_edac_mod to load on
Rome systems. The module may report an incorrect system addresses on
Rome systems depending on the interleaving option used.

Fixes: 6e846239e548 ("EDAC/amd64: Add Family 17h Model 30h PCI IDs")
Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
Co-developed-by: Naveen Krishna Chtradhi <naveenkrishna.chatradhi@amd.com>
Signed-off-by: Naveen Krishna Chtradhi <naveenkrishna.chatradhi@amd.com>
Co-developed-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* Rebased on cleanup patches.
* Save and use the Data Fabric version.
* Reorder code to execute non-legacy flows first. This change wasn't
  made to the section with the "hashed_bit" calculation, since the
  current flow reads easier IMHO.

 arch/x86/kernel/cpu/mce/amd.c | 222 ++++++++++++++++++++++++++--------
 1 file changed, 172 insertions(+), 50 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index f5440f8000e9..c14076bcabf2 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -683,8 +683,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c)
 #define DF_F0_DRAMBASEADDR	0x110
 #define DF_F0_DRAMLIMITADDR	0x114
 #define DF_F0_DRAMOFFSET	0x1B4
+#define DF_F0_DFGLOBALCTRL	0x3F8
 
 #define DF_F1_SYSFABRICID	0x208
+#define DF_F1_SYSFABRICID1	0x20C
 
 int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 {
@@ -695,22 +697,30 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	u32 dram_hole_base;
 
 	u32 reg_dram_base_addr, reg_dram_limit_addr;
-	u32 reg_dram_offset;
+	u32 reg_dram_offset, reg_sys_fabric_id;
+
+	bool hash_enabled = false, split_normalized = false;
 
-	u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
 	u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
-	u8 intlv_addr_sel, intlv_addr_bit;
-	u8 num_intlv_bits, hashed_bit;
+	u8 intlv_addr_sel, intlv_addr_bit, num_intlv_bits;
+	u8 cs_mask, cs_id = 0, dst_fabric_id = 0;
 	u8 lgcy_mmio_hole_en, base = 0;
-	u8 cs_mask, cs_id = 0;
-	bool hash_enabled = false;
+	u8 df_version;
+
+	if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &reg_sys_fabric_id))
+		goto out_err;
+
+	df_version = (reg_sys_fabric_id & 0xFF) ? 3 : 2;
 
 	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &reg_dram_offset))
 		goto out_err;
 
 	/* Remove HiAddrOffset from normalized address, if enabled: */
 	if (reg_dram_offset & BIT(0)) {
-		u64 hi_addr_offset = get_bits(reg_dram_offset, 31, 20) << 28;
+		u8 hi_addr_offset_lsb = (df_version >= 3) ? 12 : 20;
+		u64 hi_addr_offset = get_bits(reg_dram_offset, 31, hi_addr_offset_lsb);
+
+		hi_addr_offset <<= 28;
 
 		/* Check if base 1 is used. */
 		if (norm_addr >= hi_addr_offset) {
@@ -733,19 +743,23 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 
 	lgcy_mmio_hole_en = get_bit(reg_dram_base_addr, 1);
-	intlv_num_chan	  = get_bits(reg_dram_base_addr, 7, 4);
-	intlv_addr_sel	  = get_bits(reg_dram_base_addr, 10, 8);
 	dram_base_addr	  = get_bits(reg_dram_base_addr, 31, 12) << 28;
-
-	intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
-	intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
 	dram_limit_addr	  = (get_bits(reg_dram_limit_addr, 31, 12) << 28) | GENMASK_ULL(27, 0);
 
-	/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
-	if (intlv_addr_sel > 3) {
-		pr_err("%s: Invalid interleave address select %d.\n",
-			__func__, intlv_addr_sel);
-		goto out_err;
+	if (df_version >= 3) {
+		intlv_num_chan    = get_bits(reg_dram_base_addr, 5, 2);
+		intlv_num_dies    = get_bits(reg_dram_base_addr, 7, 6);
+		intlv_num_sockets = get_bit(reg_dram_base_addr, 8);
+		intlv_addr_sel    = get_bits(reg_dram_base_addr, 11, 9);
+
+		dst_fabric_id	  = get_bits(reg_dram_limit_addr, 9, 0);
+	} else {
+		intlv_num_chan	  = get_bits(reg_dram_base_addr, 7, 4);
+		intlv_addr_sel	  = get_bits(reg_dram_base_addr, 10, 8);
+
+		dst_fabric_id	  = get_bits(reg_dram_limit_addr, 7, 0);
+		intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
+		intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
 	}
 
 	intlv_addr_bit = intlv_addr_sel + 8;
@@ -758,27 +772,42 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	case 5:	intlv_num_chan = 3; break;
 	case 7:	intlv_num_chan = 4; break;
 
-	case 8: intlv_num_chan = 1;
+	case 8:
+		if (df_version >= 3) {
+			intlv_num_chan = 5;
+		} else {
+			intlv_num_chan = 1;
+			hash_enabled = true;
+		}
+		break;
+	case 12:
+		intlv_num_chan = 1;
 		hash_enabled = true;
 		break;
+	case 13:
+		intlv_num_chan = 2;
+		hash_enabled = true;
+		split_normalized = true;
+		break;
+	case 14:
+		intlv_num_chan = 3;
+		hash_enabled = true;
+		split_normalized = true;
+		break;
 	default:
 		pr_err("%s: Invalid number of interleaved channels %d.\n",
 			__func__, intlv_num_chan);
 		goto out_err;
 	}
 
-	num_intlv_bits = intlv_num_chan;
-
-	if (intlv_num_dies > 2) {
-		pr_err("%s: Invalid number of interleaved nodes/dies %d.\n",
-			__func__, intlv_num_dies);
+	/* Assert interleave address bit is 8 or 9 for hashing cases. */
+	if (hash_enabled && intlv_addr_bit != 8 && intlv_addr_bit != 9) {
+		pr_err("%s: Invalid interleave address bit for hashing %d.\n",
+			__func__, intlv_addr_bit);
 		goto out_err;
 	}
 
-	num_intlv_bits += intlv_num_dies;
-
-	/* Add a bit if sockets are interleaved. */
-	num_intlv_bits += intlv_num_sockets;
+	num_intlv_bits = intlv_num_chan + intlv_num_dies + intlv_num_sockets;
 
 	/* Assert num_intlv_bits <= 4 */
 	if (num_intlv_bits > 4) {
@@ -788,9 +817,11 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	}
 
 	if (num_intlv_bits > 0) {
-		u64 temp_addr_x, temp_addr_i, temp_addr_y;
-		u32 reg_sys_fabric_id, cs_fabric_id;
+		u8 cs_fabric_id_msb = (df_version >= 3) ? 13 : 15;
 		u8 die_id_bit, sock_id_bit;
+		u64 addr_x, addr_y, addr_z;
+		u8 node_id_shift = 0;
+		u32 cs_fabric_id;
 
 		/*
 		 * This is the fabric id for this coherent slave. Use
@@ -800,7 +831,7 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &cs_fabric_id))
 			goto out_err;
 
-		cs_fabric_id = get_bits(cs_fabric_id, 15, 8);
+		cs_fabric_id = get_bits(cs_fabric_id, cs_fabric_id_msb, 8);
 		die_id_bit   = 0;
 
 		/* If interleaved over more than 1 channel: */
@@ -808,43 +839,83 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 			die_id_bit = intlv_num_chan;
 			cs_mask	   = (1 << die_id_bit) - 1;
 			cs_id	   = cs_fabric_id & cs_mask;
+			cs_id	  -= dst_fabric_id & cs_mask;
 		}
 
 		sock_id_bit = die_id_bit;
 
-		if (intlv_num_dies || intlv_num_sockets)
-			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &reg_sys_fabric_id))
+		if ((intlv_num_dies || intlv_num_sockets) && df_version >= 3) {
+			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID1, umc, &reg_sys_fabric_id))
 				goto out_err;
 
+			node_id_shift = get_bits(reg_sys_fabric_id, 3, 0);
+		}
+
 		/* If interleaved over more than 1 die. */
 		if (intlv_num_dies) {
+			u8 die_id_shift, die_id_mask;
+
 			sock_id_bit  = die_id_bit + intlv_num_dies;
-			die_id_shift = get_bits(reg_sys_fabric_id, 27, 24);
-			die_id_mask  = get_bits(reg_sys_fabric_id, 15, 8);
+
+			if (df_version >= 3) {
+				die_id_shift = get_bits(reg_sys_fabric_id, 3, 0) + node_id_shift;
+
+				die_id_mask  = get_bits(reg_sys_fabric_id, 18, 16);
+				die_id_mask <<= node_id_shift;
+			} else {
+				die_id_shift = get_bits(reg_sys_fabric_id, 27, 24);
+				die_id_mask  = get_bits(reg_sys_fabric_id, 15, 8);
+			}
 
 			cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
 		}
 
 		/* If interleaved over more than 1 socket. */
 		if (intlv_num_sockets) {
-			socket_id_shift	= get_bits(reg_sys_fabric_id, 31, 28);
-			socket_id_mask	= get_bits(reg_sys_fabric_id, 23, 16);
+			u8 socket_id_shift, socket_id_mask;
+
+			if (df_version >= 3) {
+				socket_id_shift	= get_bits(reg_sys_fabric_id, 10, 8);
+				socket_id_shift += node_id_shift;
+
+				socket_id_mask	= get_bits(reg_sys_fabric_id, 26, 24);
+				socket_id_mask <<= node_id_shift;
+			} else {
+				socket_id_shift	= get_bits(reg_sys_fabric_id, 31, 28);
+				socket_id_mask	= get_bits(reg_sys_fabric_id, 23, 16);
+			}
 
 			cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
 		}
 
 		/*
 		 * The pre-interleaved address consists of XXXXXXIIIYYYYY
-		 * where III is the ID for this CS, and XXXXXXYYYYY are the
-		 * address bits from the post-interleaved address.
-		 * "num_intlv_bits" has been calculated to tell us how many "I"
-		 * bits there are. "intlv_addr_bit" tells us how many "Y" bits
-		 * there are (where "I" starts).
+		 * or XXXXXXIIZZZIYYY where III is the ID for this CS, and
+		 * XXXXXXZZZYYYYY are the address bits from the post-interleaved
+		 * address. "num_intlv_bits" has been calculated to tell us how
+		 * many "I" bits there are. "intlv_addr_bit" tells us how many
+		 * "Y" bits there are (where "I" starts).
+		 *
+		 * The "split" III is only used in the COD modes, where there
+		 * is one bit I at "intlv_addr_bit", and the remaining CS bits
+		 * are higher up starting at bit 12.
 		 */
-		temp_addr_y = get_bits(ret_addr, intlv_addr_bit-1, 0);
-		temp_addr_i = (cs_id << intlv_addr_bit);
-		temp_addr_x = (ret_addr & GENMASK_ULL(63, intlv_addr_bit)) << num_intlv_bits;
-		ret_addr    = temp_addr_x | temp_addr_i | temp_addr_y;
+		addr_y = get_bits(ret_addr, intlv_addr_bit - 1, 0);
+
+		if (split_normalized) {
+			addr_x = ret_addr & GENMASK_ULL(63, 11);
+			addr_x <<= num_intlv_bits;
+
+			addr_z = ret_addr & GENMASK_ULL(10, intlv_addr_bit);
+			addr_z <<= 1;
+		} else {
+			addr_x = ret_addr & GENMASK_ULL(63, intlv_addr_bit);
+			addr_x <<= num_intlv_bits;
+
+			addr_z = 0;
+		}
+
+		ret_addr = addr_x | addr_z | addr_y;
 	}
 
 	/* Add dram base address */
@@ -860,18 +931,69 @@ int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 			ret_addr += (BIT_ULL(32) - dram_hole_base);
 	}
 
-	if (hash_enabled) {
-		hashed_bit =	get_bit(ret_addr, 12) ^
+	/*
+	 * There are three cases for hashing:
+	 * 1) No Hashing
+	 * 2) Legacy Hashing
+	 * 3) Cluster-on-Die (COD) Hashing
+	 */
+	if (!hash_enabled) {
+		/* Fill in the interleave bit. */
+		if (intlv_num_chan)
+			ret_addr |= (cs_id << intlv_addr_bit);
+	} else if (df_version == 2) {
+		/* Legacy 2ch hash. */
+		u8 hashed_bit =	get_bit(ret_addr, 12) ^
 				get_bit(ret_addr, 18) ^
 				get_bit(ret_addr, 21) ^
 				get_bit(ret_addr, 30) ^
 				get_bit(cs_id, 0);
 
-		if (hashed_bit != get_bit(ret_addr, intlv_addr_bit))
-			ret_addr ^= BIT(intlv_addr_bit);
+		ret_addr ^= hashed_bit << intlv_addr_bit;
+	} else {
+		u8 hashed_bit, hash_ctl_64K, hash_ctl_2M, hash_ctl_1G;
+		u32 reg_df_global_ctrl;
+
+		if (amd_df_indirect_read(nid, 0, DF_F0_DFGLOBALCTRL, umc, &reg_df_global_ctrl))
+			goto out_err;
+
+		hash_ctl_64K = get_bit(reg_df_global_ctrl, 20);
+		hash_ctl_2M  = get_bit(reg_df_global_ctrl, 21);
+		hash_ctl_1G  = get_bit(reg_df_global_ctrl, 22);
+
+		/* COD with 2ch, 4ch, or 8ch hash. */
+		hashed_bit =	get_bit(ret_addr, 14) ^
+				(get_bit(ret_addr, 18) & hash_ctl_64K) ^
+				(get_bit(ret_addr, 23) & hash_ctl_2M) ^
+				(get_bit(ret_addr, 32) & hash_ctl_1G) ^
+				get_bit(cs_id, 0);
+
+		ret_addr ^= hashed_bit << intlv_addr_bit;
+
+		/* COD with 4ch or 8ch hash. */
+		if ((intlv_num_chan == 2) || (intlv_num_chan == 3)) {
+			hashed_bit =	get_bit(ret_addr, 12) ^
+					(get_bit(ret_addr, 16) & hash_ctl_64K) ^
+					(get_bit(ret_addr, 21) & hash_ctl_2M) ^
+					(get_bit(ret_addr, 30) & hash_ctl_1G) ^
+					get_bit(cs_id, 1);
+
+			ret_addr ^= hashed_bit << 12;
+		}
+
+		/* COD with 8ch hash. */
+		if (intlv_num_chan == 3) {
+			hashed_bit =	get_bit(ret_addr, 13) ^
+					(get_bit(ret_addr, 17) & hash_ctl_64K) ^
+					(get_bit(ret_addr, 22) & hash_ctl_2M) ^
+					(get_bit(ret_addr, 31) & hash_ctl_1G) ^
+					get_bit(cs_id, 2);
+
+			ret_addr ^= hashed_bit << 13;
+		}
 	}
 
-	/* Is calculated system address is above DRAM limit address? */
+	/* Is calculated system address above DRAM limit address? */
 	if (ret_addr > dram_limit_addr)
 		goto out_err;
 
-- 
2.25.1

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Borislav Petkov]

On Thu, Sep 03, 2020 at 08:01:44PM +0000, Yazen Ghannam wrote:
> From: Muralidhara M K <muralidhara.mk@amd.com>
> 
> Add support for new memory interleaving modes used in current AMD systems.
>
> Check if the system is using a current Data Fabric version or a legacy
> version as some bit and register definitions have changed.
> 
> Tested on AMD reference platforms with the following memory interleaving
> options.
> 
> Naples
> - None
> - Channel
> - Die
> - Socket
> 
> Rome (NPS = Nodes per Socket)
> - None
> - NPS0
> - NPS1
> - NPS2
> - NPS4
> 
> The fixes tag refers to the commit that allows amd64_edac_mod to load on
> Rome systems.

Err, why? This is adding new stuff to an address translation function.
How does that fix amd64_edac loading on Rome?

> The module may report an incorrect system addresses on
> Rome systems depending on the interleaving option used.

That doesn't stop it from loading, sorry.

Now, before you guys do any new features, I'd like you to split this
humongous function umc_normaddr_to_sysaddr() logically into separate
helpers and each helper does exactly one thing and one thing only.

Then use a verb in its name: umc_translate_normaddr_to_sysaddr() or so.

Also, Yazen, remind me again pls why isn't this function in
drivers/edac/amd64_edac.c, where it is needed?

If the reason is not valid anymore, let's move it there before splitting
so that it doesn't bloat the core code.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Yazen Ghannam]

On Wed, Sep 23, 2020 at 10:20:39AM +0200, Borislav Petkov wrote:
> On Thu, Sep 03, 2020 at 08:01:44PM +0000, Yazen Ghannam wrote:
> > From: Muralidhara M K <muralidhara.mk@amd.com>
> > 
> > Add support for new memory interleaving modes used in current AMD systems.
> >
> > Check if the system is using a current Data Fabric version or a legacy
> > version as some bit and register definitions have changed.
> > 
> > Tested on AMD reference platforms with the following memory interleaving
> > options.
> > 
> > Naples
> > - None
> > - Channel
> > - Die
> > - Socket
> > 
> > Rome (NPS = Nodes per Socket)
> > - None
> > - NPS0
> > - NPS1
> > - NPS2
> > - NPS4
> > 
> > The fixes tag refers to the commit that allows amd64_edac_mod to load on
> > Rome systems.
> 
> Err, why? This is adding new stuff to an address translation function.
> How does that fix amd64_edac loading on Rome?
> 
> > The module may report an incorrect system addresses on
> > Rome systems depending on the interleaving option used.
> 
> That doesn't stop it from loading, sorry.
>

Okay, no problem.

> Now, before you guys do any new features, I'd like you to split this
> humongous function umc_normaddr_to_sysaddr() logically into separate
> helpers and each helper does exactly one thing and one thing only.
> 
> Then use a verb in its name: umc_translate_normaddr_to_sysaddr() or so.
>

Okay, will do.

> Also, Yazen, remind me again pls why isn't this function in
> drivers/edac/amd64_edac.c, where it is needed?
> 
> If the reason is not valid anymore, let's move it there before splitting
> so that it doesn't bloat the core code.
>

I don't remember the original reason, and I was recently asked about
this code living in a module. I did some looking after this ask, and I
found that we should be using this translation to get a proper value for
the memory error notifiers to use. So I think we still need to use this
function some way with the core code even if the EDAC interface isn't
used.

I think this set can be split up.

1) Set with patches 1-3 fixed up to use cpu_die_id.
2) Set with the address translation updates.
   a) Move umc_normaddr_to_sysaddr() into a new module under EDAC.
   b) Hook the new module into amd64_edac.c where it's used today.
   c) Refactor the code as you suggested above.
   d) Add the new features.
3) New set that sets up a proper notifier for the address translation.
   a) Unhook the new module from amd64_edac.c.
   b) Register a notifer that runs before any notifiers that operate on
      memory errors.
   c) Find a way to pass the translated address through the chain
      without losing the original value.

What do you think?

Thanks,
Yazen

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Borislav Petkov]

On Wed, Sep 23, 2020 at 11:25:10AM -0500, Yazen Ghannam wrote:
> I don't remember the original reason, and I was recently asked about
> this code living in a module. I did some looking after this ask, and I
> found that we should be using this translation to get a proper value for
> the memory error notifiers to use. So I think we still need to use this
> function some way with the core code even if the EDAC interface isn't
> used.

You'd need to be more specific here, you want to bypass amd64_edac to
decode errors? Judging by the current RAS activity coming from you guys,
I'm thinking firmware. But then wouldn't the firmware do the decoding
for us and then this function is not even needed?

> What do you think?

I think you should explain what the use case is first. :)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Yazen Ghannam]

On Fri, Sep 25, 2020 at 09:22:31AM +0200, Borislav Petkov wrote:
> On Wed, Sep 23, 2020 at 11:25:10AM -0500, Yazen Ghannam wrote:
> > I don't remember the original reason, and I was recently asked about
> > this code living in a module. I did some looking after this ask, and I
> > found that we should be using this translation to get a proper value for
> > the memory error notifiers to use. So I think we still need to use this
> > function some way with the core code even if the EDAC interface isn't
> > used.
> 
> You'd need to be more specific here, you want to bypass amd64_edac to
> decode errors? Judging by the current RAS activity coming from you guys,
> I'm thinking firmware. But then wouldn't the firmware do the decoding
> for us and then this function is not even needed?
>

The UC, NFIT, and CEC notifiers all operate on system physical
addresses. The address in the MCE record is checked by
mce_usable_address() to see if it can be used by the kernel, i.e. the
address is a system physical address. Right now, this check passes on
AMD systems if MCA_STATUS[AddrV] is set. This works for memory errors on
legacy AMD systems, since the NB MCA bank logs a physical address for
DRAM ECC errors. But this won't work on newer systems, because the UMC
MCA bank does not log a system physical address for DRAM ECC errors. So
the address provided by the hardware will need to be translated to a
physical address before the notifiers in the MCE chain can use it.

We can add support to get the physical address from firmware in some
cases. But it looks to me that we'll still need to keep updating the
translation code in the kernel to cover some platform/user
configurations. So it makes sense to me to move the functionality into a
module to make it easier to update.

The address translation needs to be done before the notfiers that need
it, and EDAC comes after all of them. There's also the case where the
EDAC interface isn't wanted, so amd64_edac will be unloaded. But the
functionality in the other notifiers are still expected to be available.
So it's more than just decoding the error like we do now with amd64_edac.
That's why I think the translation code can be in a separate module with
a notfier that runs before the others. This can do the translation once
then pass the result down to the CEC, UC, NFIT, and EDAC notifiers to
use as needed.

Thanks,
Yazen

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Borislav Petkov]

On Fri, Sep 25, 2020 at 02:51:27PM -0500, Yazen Ghannam wrote:
> We can add support to get the physical address from firmware in some
> cases. But it looks to me that we'll still need to keep updating the
> translation code in the kernel to cover some platform/user
> configurations.

Unfortunately, that is correct. :-\

> The address translation needs to be done before the notfiers that need
> it, and EDAC comes after all of them. There's also the case where the
> EDAC interface isn't wanted, so amd64_edac will be unloaded.

I'd be interested as to why. Because decoding addresses is amd64_edac
*core* functionality. We can stick it in drivers/edac/mce_amd.c but I'd
like to hear what those valid reasons are, not to use the driver which
is supposed to do that anyway.

Thanks for explaining - it is all clear now.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Yazen Ghannam]

On Mon, Sep 28, 2020 at 11:47:59AM +0200, Borislav Petkov wrote:
> On Fri, Sep 25, 2020 at 02:51:27PM -0500, Yazen Ghannam wrote:
> 
> > The address translation needs to be done before the notfiers that need
> > it, and EDAC comes after all of them. There's also the case where the
> > EDAC interface isn't wanted, so amd64_edac will be unloaded.
> 
> I'd be interested as to why. Because decoding addresses is amd64_edac
> *core* functionality. We can stick it in drivers/edac/mce_amd.c but I'd
> like to hear what those valid reasons are, not to use the driver which
> is supposed to do that anyway.
>

I don't have any clear reasons. I just get vague use cases sometimes
about not using EDAC and relying on other things. But it shouldn't hurt
to have the module load anyway. The EDAC messages can be suppressed, and
the sysfs interface can be ignored. So, after a bit more thought, this
doesn't seem like a good reason.

I agree that the translation code is implementation-specific and applies
only to DRAM ECC errors, so it make sense to have it in amd64_edac. The
only issue is getting the address translation to earlier notifiers. I
think we can add a new one in amd64_edac to run before others. Maybe this
can be a new priority class like MCE_PRIO_PREPROCESS, or something like
that for notifiers that fixup the MCE data.

I can start by moving the address translation to amd64_edac and doing
the code cleanup.

Thanks,
Yazen

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Borislav Petkov]

On Mon, Sep 28, 2020 at 10:53:50AM -0500, Yazen Ghannam wrote:
> I don't have any clear reasons. I just get vague use cases sometimes
> about not using EDAC and relying on other things. But it shouldn't hurt
> to have the module load anyway. The EDAC messages can be suppressed, and
> the sysfs interface can be ignored. So, after a bit more thought, this
> doesn't seem like a good reason.

Ok. We can always carve it out if someone comes up with a valid reason
later.

> I agree that the translation code is implementation-specific and applies
> only to DRAM ECC errors, so it make sense to have it in amd64_edac. The
> only issue is getting the address translation to earlier notifiers. I
> think we can add a new one in amd64_edac to run before others. Maybe this
> can be a new priority class like MCE_PRIO_PREPROCESS, or something like
> that for notifiers that fixup the MCE data.

Well, I'm not sure you need notifiers here - you wanna call
mce_usable_address() and in it, it should do the address conversion
calculation to give you a physical address which you can feed to
memory_failure etc.

Now, mce_usable_address() is core code and we can make core code call
into a module but that is yucky. So *that* is your reason for keeping it
where it is.

Looking at its size:

$ readelf -s vmlinux | grep umc_normaddr_to
  2864: ffffffff817d8ae5   168 FUNC    LOCAL  DEFAULT    1 umc_normaddr_to_[...]
 91866: ffffffff81030e00  1127 FUNC    GLOBAL DEFAULT    1 umc_normaddr_to_[...]

that's something like ~1.3K and if you split it and do some
experimenting, you might get it even slimmer. Not that ~1.3K is that
huge for current standards but we should always aim at not bloating the
fat guy our kernel already is.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation

[Yazen Ghannam]

On Mon, Sep 28, 2020 at 08:14:07PM +0200, Borislav Petkov wrote:
> On Mon, Sep 28, 2020 at 10:53:50AM -0500, Yazen Ghannam wrote:
> 
> > I agree that the translation code is implementation-specific and applies
> > only to DRAM ECC errors, so it make sense to have it in amd64_edac. The
> > only issue is getting the address translation to earlier notifiers. I
> > think we can add a new one in amd64_edac to run before others. Maybe this
> > can be a new priority class like MCE_PRIO_PREPROCESS, or something like
> > that for notifiers that fixup the MCE data.
> 
> Well, I'm not sure you need notifiers here - you wanna call
> mce_usable_address() and in it, it should do the address conversion
> calculation to give you a physical address which you can feed to
> memory_failure etc.
> 
> Now, mce_usable_address() is core code and we can make core code call
> into a module but that is yucky. So *that* is your reason for keeping it
> where it is.
>

Okay, we'll keep the code where it is. I'll work on another set to call
the address translation with mce_usable_address().

> Looking at its size:
> 
> $ readelf -s vmlinux | grep umc_normaddr_to
>   2864: ffffffff817d8ae5   168 FUNC    LOCAL  DEFAULT    1 umc_normaddr_to_[...]
>  91866: ffffffff81030e00  1127 FUNC    GLOBAL DEFAULT    1 umc_normaddr_to_[...]
> 
> that's something like ~1.3K and if you split it and do some
> experimenting, you might get it even slimmer. Not that ~1.3K is that
> huge for current standards but we should always aim at not bloating the
> fat guy our kernel already is.
>

Okay, I'll keep an eye on this and try to slim it down.

Thanks,
Yazen