Re: [PATCH 00/25] AMD MCA Address Translation Updates

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Mon, May 17, 2021 at 02:57:04PM +0200, Borislav Petkov wrote:
> On Fri, May 07, 2021 at 03:01:15PM -0400, Yazen Ghannam wrote:
> > Patches 1-24 do the refactor without adding new system support. The goal
> > is to break down the translation algorithm into smaller chunks. There
> > are some simple wrapper functions defined. These will be filled in when
> > supporting newer systems. The intention is that new system support can
> > be added without any major refactor. I tried to make a patch for each
> > logical change. There's a bit of churn so as to not break the build with
> > each change. I think many of these patches can be squashed together, if
> > desired. The top level function was split first, then the next level of
> > functions, etc. in a somewhat breadth-first approach.
> 
> No, that's great what you did and keeping each logical change in a
> single patch is a lot easier on everybody involved.
> 
> Now, looking at this - and I know we've talked about this before - but:
> 
> umc_normaddr_to_sysaddr() is used only in amd64_edac.c.
> amd_df_indirect_read() is used only by this function, so how about
> moving both to amd64_edac, where they're needed and then doing the
> refactoring ontop?
> 
> You can simply reuse your current patches - just change the file they
> patch from
> 
> arch/x86/kernel/cpu/mce/amd.c
> 
> to
> 
> drivers/edac/amd64_edac.c
> 
> I went through te umc_... function and AFAICT, it doesn't need any core
> MCE facilities so it should be just fine in EDAC land.
> 
> Or?
>

I think this is a good idea. The only hang up is that we should be using
the output of this function, i.e. the systeme physical address, when
handling memory errors in the MCE notifier blocks. But I have an idea
where we can handle this. I can send that as a follow up series, if
that's okay.

One other issue is what if a user doesn't want to use amd64_edac_mod?
This is more of a user preference and/or configuration issue. Maybe the
module loads, but an uninterested user can tell EDAC to not log errors,
etc.? Or should the translation code live in its own module?

So for version 2, I have 1) Add a glossary of terms, and 2) Move
everything to EDAC. Any other comments?

Thanks,
Yazen