| Message ID | 20221206173607.1185907-3-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mce: Support extended MCA_ADDR address on SMCA systems | expand |
On Tue, Dec 06, 2022 at 11:36:05AM -0600, Yazen Ghannam wrote: > + mce_flags.overflow_recov = !!cpu_has(c, X86_FEATURE_OVERFLOW_RECOV); > + mce_flags.succor = !!cpu_has(c, X86_FEATURE_SUCCOR); > + mce_flags.smca = !!cpu_has(c, X86_FEATURE_SMCA); > + mce_flags.amd_threshold = 1; > > for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) { > if (mce_flags.smca) > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 5f406d135d32..9efd6d010e2d 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -1906,19 +1906,6 @@ static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c) > return 0; > } > > -/* > - * Init basic CPU features needed for early decoding of MCEs. > - */ > -static void __mcheck_cpu_init_early(struct cpuinfo_x86 *c) > -{ > - if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor == X86_VENDOR_HYGON) { > - mce_flags.overflow_recov = !!cpu_has(c, X86_FEATURE_OVERFLOW_RECOV); > - mce_flags.succor = !!cpu_has(c, X86_FEATURE_SUCCOR); > - mce_flags.smca = !!cpu_has(c, X86_FEATURE_SMCA); > - mce_flags.amd_threshold = 1; > - } Yeah, looking at this, before and after the change, what we are and were doing here is silly. Those flags are global for the whole system but we do set them on each CPU - unnecessarily, ofc ;-\ - because we don't have a BSP MCE init call. That above happens on the mcheck_cpu_init() path which is per-CPU. However, if we had to be precise and correct, this flags setup should happen in a function called mcheck_bsp_init() or so which gets called at the end of identify_boot_cpu() and which does all the *once* actions there like allocate the gen pool, run the quirks which need to run only once on the BSP and so on. So that we don't have to do unnecessary work on every CPU. Tony, thoughts? I think we should start working towards this - doesn't have to be done immediately but I think a proper separation of what runs where - once on the BSP or on every CPU - is needed here. Unless I'm missing an important angle, which is entirely possible. Hmmm.
> Yeah, looking at this, before and after the change, what we are and were > doing here is silly. Those flags are global for the whole system but we > do set them on each CPU - unnecessarily, ofc ;-\ - because we don't have > a BSP MCE init call. > > That above happens on the mcheck_cpu_init() path which is per-CPU. > > However, if we had to be precise and correct, this flags setup should > happen in a function called > > mcheck_bsp_init() > > or so which gets called at the end of identify_boot_cpu() and which does > all the *once* actions there like allocate the gen pool, run the quirks > which need to run only once on the BSP and so on. > > So that we don't have to do unnecessary work on every CPU. > > Tony, thoughts? > > I think we should start working towards this - doesn't have to be done > immediately but I think a proper separation of what runs where - once > on the BSP or on every CPU - is needed here. Unless I'm missing an > important angle, which is entirely possible. Cleanup sounds good. But do we need a new mcheck_bsp_init() function? Can the "only once" stuff be done from mcheck_init()? Or does it rely on things that aren't set up that early? -Tony
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index 10fb5b5c9efa..b80472a52ad8 100644 --- a/arch/x86/kernel/cpu/mce/amd.c +++ b/arch/x86/kernel/cpu/mce/amd.c @@ -681,6 +681,10 @@ void mce_amd_feature_init(struct cpuinfo_x86 *c) u32 low = 0, high = 0, address = 0; int offset = -1; + mce_flags.overflow_recov = !!cpu_has(c, X86_FEATURE_OVERFLOW_RECOV); + mce_flags.succor = !!cpu_has(c, X86_FEATURE_SUCCOR); + mce_flags.smca = !!cpu_has(c, X86_FEATURE_SMCA); + mce_flags.amd_threshold = 1; for (bank = 0; bank < this_cpu_read(mce_num_banks); ++bank) { if (mce_flags.smca) diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 5f406d135d32..9efd6d010e2d 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -1906,19 +1906,6 @@ static int __mcheck_cpu_ancient_init(struct cpuinfo_x86 *c) return 0; } -/* - * Init basic CPU features needed for early decoding of MCEs. - */ -static void __mcheck_cpu_init_early(struct cpuinfo_x86 *c) -{ - if (c->x86_vendor == X86_VENDOR_AMD || c->x86_vendor == X86_VENDOR_HYGON) { - mce_flags.overflow_recov = !!cpu_has(c, X86_FEATURE_OVERFLOW_RECOV); - mce_flags.succor = !!cpu_has(c, X86_FEATURE_SUCCOR); - mce_flags.smca = !!cpu_has(c, X86_FEATURE_SMCA); - mce_flags.amd_threshold = 1; - } -} - static void mce_centaur_feature_init(struct cpuinfo_x86 *c) { struct mca_config *cfg = &mca_cfg; @@ -2139,10 +2126,9 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c) mca_cfg.initialized = 1; - __mcheck_cpu_init_early(c); - __mcheck_cpu_init_generic(); __mcheck_cpu_init_vendor(c); __mcheck_cpu_init_prepare_banks(); + __mcheck_cpu_init_generic(); __mcheck_cpu_setup_timer(); } @@ -2308,9 +2294,9 @@ static void mce_syscore_shutdown(void) */ static void mce_syscore_resume(void) { - __mcheck_cpu_init_generic(); __mcheck_cpu_init_vendor(raw_cpu_ptr(&cpu_info)); __mcheck_cpu_init_prepare_banks(); + __mcheck_cpu_init_generic(); } static struct syscore_ops mce_syscore_ops = { @@ -2327,8 +2313,8 @@ static void mce_cpu_restart(void *data) { if (!mce_available(raw_cpu_ptr(&cpu_info))) return; - __mcheck_cpu_init_generic(); __mcheck_cpu_init_prepare_banks(); + __mcheck_cpu_init_generic(); __mcheck_cpu_init_timer(); }
The __mcheck_cpu_init_early() function was introduced so that some vendor-specific features are detected before the first MCA polling event done in __mcheck_cpu_init_generic(). Currently, __mcheck_cpu_init_early() is only used on AMD-based systems and additional code will be needed to support various system configurations. However, the current and future vendor-specific code should be done during vendor init. This keeps all the vendor code in a common location and simplifies the generic init flow. Move all the __mcheck_cpu_init_early() code into mce_amd_feature_init(). Also, move __mcheck_cpu_init_generic() after __mcheck_cpu_init_prepare_banks() so that MCA is enabled after the first MCA polling event. Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> --- Link: https://lore.kernel.org/r/YqJHwXkg3Ny9fI3s@yaz-fattaah v6: New. arch/x86/kernel/cpu/mce/amd.c | 4 ++++ arch/x86/kernel/cpu/mce/core.c | 20 +++----------------- 2 files changed, 7 insertions(+), 17 deletions(-)