LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Yazen Ghannam <Yazen.Ghannam@amd.com>
Cc: linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
	tony.luck@intel.com, x86@kernel.org
Subject: Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems
Date: Tue, 17 Apr 2018 19:21:02 +0200	[thread overview]
Message-ID: <20180417172102.GA3633@pd.tnic> (raw)
In-Reply-To: <20180402195707.42875-1-Yazen.Ghannam@amd.com>

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.

  reply	other threads:[~2018-04-17 17:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-02 19:57 Yazen Ghannam
2018-04-17 17:21 ` Borislav Petkov [this message]
2018-04-17 18:30   ` Ghannam, Yazen
2018-04-18 17:13     ` Borislav Petkov
2018-04-20 13:05       ` Ghannam, Yazen
2018-04-20 18:03         ` Borislav Petkov

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=20180417172102.GA3633@pd.tnic \
    --to=bp@alien8.de \
    --cc=Yazen.Ghannam@amd.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] x86/MCE, EDAC/mce_amd: Save all aux registers on SMCA systems' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).