linux-kernel - Re: [PATCH v2 1/2] x86/mce/AMD: Redo use of SMCA MCA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 Apr 2017 18:44:33 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     "Ghannam, Yazen" <Yazen.Ghannam@....com>
Cc:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        Tony Luck <tony.luck@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] x86/mce/AMD: Redo use of SMCA MCA_DE{STAT,ADDR}
 registers

On Wed, Apr 05, 2017 at 02:52:08PM +0000, Ghannam, Yazen wrote:
> Yes, BIOS does and should set this bit. If it doesn't for some reason
> then our handling in the Kernel will still be functional. We just
> won't find Deferred errors in MCA_STATUS.

Ok.

> I'd rather we keep as many checks as possible out of __log_error().

What checks?

> Your suggestion gave me an idea. Let's drop __log_error_deferred() and just
> select the correct registers in the deferred error interrupt handler.
> 
> /*
>  * APIC interrupt handler for deferred errors
>  *
>  * We have three scenarios for checking for Deferred errors.
>  * 1) Non-SMCA systems check MCA_STATUS and log error if found.
>  * 2) SMCA systems check MCA_STATUS. If error is found then log it and also
>  *    clear MCA_DESTAT.
>  * 3) SMCA systems check MCA_DESTAT, if error was not found in MCA_STATUS, and
>  *    log it.
>  */
> static void amd_deferred_error_interrupt(void)
> {
>         unsigned int bank;
>         u64 status;
> 
>         for (bank = 0; bank < mca_cfg.banks; ++bank) {
>                 rdmsrl(msr_ops.status(bank), status);
> 
>                 if (is_deferred_error(status)) {
>                         __log_error(bank, msr_ops.status(bank), msr_ops.addr(bank), 0);

So we're an SMCA box and we land here on a deferred error, we don't have
anything in the standard MSRs...

>                         /* Clear MCA_DESTAT even if we used MCA_STATUS. */
>                         if (mce_flags.smca)
>                                 wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), 0);

... and here we clear the info which we wanted to log before we log it!

> 
>                 } else if (mce_flags.smca) {
>                         rdmsrl(MSR_AMD64_SMCA_MCx_DESTAT(bank), status);
> 
>                         if (is_deferred_error(status))
>                                 __log_error(bank, MSR_AMD64_SMCA_MCx_DESTAT(bank), MSR_AMD64_SMCA_MCx_DEADDR(bank), 0);

So we execute __log_error() twice on an SMCA box for a deferred error.

So no, I'd like to see clear code flow which does what the guidance is.
And if you feel it is not readable enough then you restructure more. And
I gave you a perfectly fine __log_error_deferred() which actually does
what the guidance is.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.