linux-kernel - RE: [PATCH 1/3] EDAC/amd64: Print ECC enabled/disabled for nodes with enabled MCs

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 28 Mar 2018 14:38:11 +0000
From:   "Ghannam, Yazen" <Yazen.Ghannam@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] EDAC/amd64: Print ECC enabled/disabled for nodes with
 enabled MCs

> -----Original Message-----
> From: linux-edac-owner@...r.kernel.org <linux-edac-
> owner@...r.kernel.org> On Behalf Of Borislav Petkov
> Sent: Wednesday, March 28, 2018 9:00 AM
> To: Ghannam, Yazen <Yazen.Ghannam@....com>
> Cc: linux-edac@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 1/3] EDAC/amd64: Print ECC enabled/disabled for nodes
> with enabled MCs
> 
> On Wed, Mar 21, 2018 at 02:13:33PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@....com>
> >
> > It's possible that a system can be used without any DRAM populated on
> > one or more physical Dies on multi-die systems. Firmware will not
> > enable DRAM ECC on Dies without DRAM. Users will then see a message
> > about DRAM ECC disabled on those nodes without DRAM. However, DRAM
> ECC
> > may, in fact, be enabled on the other Dies that have DRAM.
> >
> > Only print ECC enabled/disabled information for nodes that have at least
> > one enabled memory channel.
> 
> So if the only reason for this is make the error messages more precise,
> then let's not make it uglier than it is.
> 
> The right way to do it would be to push those checks down to
> debug_display_dimm_sizes* which looks at the CS rows and the chip select
> enable bits and there to differentiate between
> 
> * memory controller doesn't have DIMMs
> 
> and
> 
> * memory controller has DIMMs but ECC is disabled in the BIOS
> 
> and then print the respective informative error message. But not with a
> yet another boolean which kinda takes care of F17h only and leaves the
> old families as they were.
> 

In either of those cases we won't get to debug_display_dimm_sizes*
because we won't initialize the instance.

We have a message for "memory controller doesn't have DIMMs".
edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);

We can change the wording of this to make it more clear. Also, we can
make this a pr_info and return from here.

So something like this:

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 329cb96f886f..6e211ed6d419 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -3061,10 +3061,12 @@ static bool ecc_enabled(struct pci_dev *F3, u16 nid)
                }
 
                /* Check whether at least one UMC is enabled: */
-               if (umc_en_mask)
+               if (umc_en_mask) {
                        ecc_en = umc_en_mask == ecc_en_mask;
-               else
-                       edac_dbg(0, "Node %d: No enabled UMCs.\n", nid);
+               } else {
+                       amd64_info("Node %d: No DIMMs populated on memory controller.\n", nid);
+                       return false;
+               }
 
                /* Assume UMC MCA banks are enabled. */
                nb_mce_en = true;

This would work for Fam17h. For older systems I think we can look at
D18F2x94_dct[1:0][DisDramInterface]

Or maybe we have a separate function to check for enabled memory controllers
before we check for ECC?

Thanks,
Yazen