On Mon, Apr 02, 2018 at 02:57:07PM -0500, Yazen Ghannam wrote: > From: Yazen Ghannam <[email protected]> > > The Intel SDM and AMD APM both state that the auxiliary MCA registers > should be read if their respective valid bits are set in MCA_STATUS. > > The Processor Programming Reference for AMD Fam17h systems has a new > recommendation that the auxiliary registers should be saved > unconditionally. This recommendation can be retroactively applied to > older AMD systems. However, we only need to apply this to SMCA systems > to avoid modifying behavior on older systems.
Applying the logic of that recommendation on older systems: wouldn't it be prudent to save them there too, if it helps debugging an MCE? > Define a separate function to save all auxiliary registers on SMCA > systems. Call this function from both the MCE handlers and the AMD LVT > interrupt handlers so that we don't duplicate code. > > Print all auxiliary registers in EDAC/mce_amd. Don't restrict this to > SMCA systems in order to save a conditional and keep the format similar > between SMCA and non-SMCA systems. > > Signed-off-by: Yazen Ghannam <[email protected]> ... > diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c > b/arch/x86/kernel/cpu/mcheck/mce_amd.c > index f7666eef4a87..b00d5fff1848 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c > +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c > @@ -244,6 +244,47 @@ static void smca_configure(unsigned int bank, unsigned > int cpu) > } > } > > + > +static bool _smca_read_aux(struct mce *m, int bank, bool read_addr) > +{ > + if (!mce_flags.smca) > + return false; > + > + rdmsrl(MSR_AMD64_SMCA_MCx_IPID(bank), m->ipid); > + rdmsrl(MSR_AMD64_SMCA_MCx_SYND(bank), m->synd); > + > + /* > + * We should already have a value if we're coming from the Threshold LVT > + * interrupt handler. Otherwise, read it now. > + */ > + if (!m->misc) > + rdmsrl(msr_ops.misc(bank), m->misc); > + > + /* > + * Read MCA_ADDR if we don't have it already. We should already have it > + * if we're coming from the interrupt handlers. > + */ > + if (read_addr) Why not if (!m->addr) ? And yeah, if it has been read to 0 already, reading it again won't change anything. And thinking about it more, you don't really need those if-tests, I'd say. So what, you'll read one or two MSRs once more. It is not such a hot path that we can't stomach the perf penalty of reading the MSRs. > + rdmsrl(msr_ops.addr(bank), m->addr); > + > + /* > + * Extract [55:<lsb>] where lsb is the least significant > + * *valid* bit of the address bits. > + */ > + if (m->addr) { And that test is probably not needed either: if m->addr is 0, the below would be 0 anyway. > + u8 lsb = (m->addr >> 56) & 0x3f; > + > + m->addr &= GENMASK_ULL(55, lsb); > + } > + > + return true; > +} IOW, those tests are probably ok but getting rid of them would make the code more readable and I think we can afford that here. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
