Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Apr 2019 19:25:39 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     "Ghannam, Yazen" <Yazen.Ghannam@amd.com>
Cc:     "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH RESEND 2/5] x86/MCE: Handle MCA controls in a per_cpu way
Message-ID: <20190410172539.GF26580@zn.tnic>
References: <20190408141205.12376-1-Yazen.Ghannam@amd.com>
 <20190408141205.12376-3-Yazen.Ghannam@amd.com>
 <20190408175142.GK15689@zn.tnic>
 <SN6PR12MB26397E000684890C92075985F82C0@SN6PR12MB2639.namprd12.prod.outlook.com>
 <20190409203412.GE6150@zn.tnic>
 <SN6PR12MB2639C9B48E46DA6A8BA9DB3DF82E0@SN6PR12MB2639.namprd12.prod.outlook.com>
 <20190410164036.GC26580@zn.tnic>
 <SN6PR12MB2639F37C634BC7F6D4B86194F82E0@SN6PR12MB2639.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <SN6PR12MB2639F37C634BC7F6D4B86194F82E0@SN6PR12MB2639.namprd12.prod.outlook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 10, 2019 at 04:58:12PM +0000, Ghannam, Yazen wrote:
> Yes, unused banks in the middle are counted in the MCG_CAP[Count] value.

Good.

> Okay, so you're saying the sysfs access should fail if a bank is
> disabled. Is that correct?

Well, think about it. If a bank is not operational for whatever reason,
we should tell the user that.

> Does "disabled" mean one or both of these?
> Unused = RAZ/WI in hardware
> Uninitialized = Not initialized by kernel due to quirks, etc.
> 
> For an unused bank, it doesn't hurt to write MCA_CTL, but really
> there's no reason to do so and go through mce_restart().

Yes, but that bank is non-operational in some form. So we should prevent
all writes to it because, well, it is not going to do anything. And this
would be a good way to give feedback to the user that that is the case.

> For an uninitialized bank, should we prevent users from overriding the
> kernel's settings?

That all depends on the quirks. Whether we should allow them to be
overridden or not. I don't think we've ever thought about it, though.

Let's look at one:

        if (c->x86_vendor == X86_VENDOR_AMD) {
                if (c->x86 == 15 && cfg->banks > 4) {
                        /*
                         * disable GART TBL walk error reporting, which
                         * trips off incorrectly with the IOMMU & 3ware
                         * & Cerberus:
                         */
                        clear_bit(10, (unsigned long *)&mce_banks[4].ctl);


Yah, so if the user reenables those GART errors, then she/he will see a
lot of MCEs reported and will maybe complain about it. And then we'll
say, but why did you enable them then. And she/he'll say: uh, didn't
know. Or, I was just poking at sysfs and this happened.

Then we can say, well, don't do that then! :-)

So my current position is, meh, who cares. But then I'm looking at
another quirk:

        if (c->x86_vendor == X86_VENDOR_INTEL) {
                /*
                 * SDM documents that on family 6 bank 0 should not be written
                 * because it aliases to another special BIOS controlled
                 * register.
                 * But it's not aliased anymore on model 0x1a+
                 * Don't ignore bank 0 completely because there could be a
                 * valid event later, merely don't write CTL0.
                 */

                if (c->x86 == 6 && c->x86_model < 0x1A && cfg->banks > 0)
                        mce_banks[0].init = 0;


which basically prevents that bank from being reinitialized. So I guess
we have that functionality already - we simply need to pay attention to
w->init.

Right?

-- 
Regards/Gruss,
    Boris.

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