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

From: Yazen Ghannam <yazen.ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	tony.luck@intel.com, x86@kernel.org,
	Smita.KoralahalliChannabasappa@amd.com
Subject: Re: [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems
Date: Wed, 9 Sep 2020 15:17:55 -0500	[thread overview]
Message-ID: <20200909201755.GB3014671@yaz-nikka.amd.com> (raw)
In-Reply-To: <20200909180647.GF12237@zn.tnic>

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