Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
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 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation
Date: Wed, 23 Sep 2020 11:25:10 -0500
Message-ID: <20200923162510.GB1684790@yaz-nikka.amd.com> (raw)
In-Reply-To: <20200923082039.GB28545@zn.tnic>

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

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-03 20:01 [PATCH v2 0/8] AMD MCA Address Translation Updates Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 1/8] x86/CPU/AMD: Save NodeId on AMD-based systems Yazen Ghannam
2020-09-09 18:06   ` Borislav Petkov
2020-09-09 20:17     ` Yazen Ghannam
2020-09-10 10:14       ` Borislav Petkov
2020-09-14 19:20         ` Yazen Ghannam
2020-09-15  8:35           ` Borislav Petkov
2020-09-16 19:51             ` Yazen Ghannam
2020-09-17 10:37               ` Borislav Petkov
2020-09-17 16:20                 ` Yazen Ghannam
2020-09-17 16:40                   ` Borislav Petkov
2020-09-17 19:44                     ` Yazen Ghannam
2020-09-17 20:10                       ` Borislav Petkov
2020-09-03 20:01 ` [PATCH v2 2/8] x86/CPU/AMD: Remove amd_get_nb_id() Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 3/8] EDAC/mce_amd: Use struct cpuinfo_x86.node_id for NodeId Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 4/8] x86/MCE/AMD: Use defines for register addresses in translation code Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 5/8] x86/MCE/AMD: Use macros to get bitfields " Yazen Ghannam
2020-09-21 13:58   ` Borislav Petkov
2020-09-03 20:01 ` [PATCH v2 6/8] x86/MCE/AMD: Drop tmp variable " Yazen Ghannam
2020-09-23  8:05   ` Borislav Petkov
2020-09-23 16:05     ` Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 7/8] x86/MCE/AMD: Group register reads " Yazen Ghannam
2020-09-03 20:01 ` [PATCH v2 8/8] x86/MCE/AMD Support new memory interleaving modes during address translation Yazen Ghannam
2020-09-23  8:20   ` Borislav Petkov
2020-09-23 16:25     ` Yazen Ghannam [this message]
2020-09-25  7:22       ` Borislav Petkov
2020-09-25 19:51         ` Yazen Ghannam
2020-09-28  9:47           ` Borislav Petkov
2020-09-28 15:53             ` Yazen Ghannam
2020-09-28 18:14               ` Borislav Petkov
2020-09-29 13:21                 ` Yazen Ghannam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200923162510.GB1684790@yaz-nikka.amd.com \
    --to=yazen.ghannam@amd.com \
    --cc=Smita.KoralahalliChannabasappa@amd.com \
    --cc=bp@alien8.de \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git