lkml.org 
[lkml]   [2018]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems
On Mon, Apr 02, 2018 at 02:57:07PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> 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 <yazen.ghannam@amd.com>

...

> 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.

\
 
 \ /
  Last update: 2018-04-17 19:22    [W:0.083 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site