Message ID | 20231206013846.1859347-1-sohil.mehta@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp3823983vqy; Tue, 5 Dec 2023 17:41:07 -0800 (PST) X-Google-Smtp-Source: AGHT+IFQRFpXe3U1yXp8qZXrWhg0YFs4Oi3XDis9Jo3pXPZXDhPv0Bz4SrvxKVu+4grD2+4nE1tf X-Received: by 2002:a05:6a00:1d09:b0:6ce:49f9:d3e1 with SMTP id a9-20020a056a001d0900b006ce49f9d3e1mr99004pfx.12.1701826867358; Tue, 05 Dec 2023 17:41:07 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701826867; cv=none; d=google.com; s=arc-20160816; b=RQWjEqsbwiOVnCFoETtujRSwd8fLaQarldZ+ZM54yMcqZsvrgOzmxhctlm7G6B6N5c Xik4SnODxcKS/KTgdP25Z9oWH4CgtJV2VA67q2QTXoKLLTAmrKizC6rylrBG4txC0T4n jNuAeaPjz8StSxSswkApZUf/lcYeh1vczrsoGQhhvvDVIHieiBPMu5zLpzwX9oFmH7ji Nx8Fr+dPqQu5NSytJnoRUEBsumEdJvYZjzxpQMtvjvRcqhsvGJf3qtDB/72WTh9I6dTj mb+g/HOEH+npbJjN1diZWupFXXgT7g0IZG+dJxvOIDi80inXQUhZq3gNwyPnNACuCyBy QjsA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=pE4wwothtRJLgrVaQZ6XFf/UR7oCE2Brpcu2Cfj/TdU=; fh=MiQcay5QYfYYDDdCOO92vBm+WZxz0OoqtdWNdgf5PHY=; b=tcxQvtwGIgRyCoYCgSOKceRSbmVusTtkb40Gspe6e8oP8cd5RPTX35gyOFp6gUPyav NsjukxkrFv+t/zMBNI8sVEQ2t/+b6GVKOlZFndr602s/WUf6NfpV4y87zt/89RkzMepq 56kB++GBqSZKBX5PaXqALQXx31Un+BzPWqKUO1mh6VbMxK5FZ37iWsEJMrr4J97DD9n6 5ZEazeZ+aUVZ5awKgtUFQIHYMBpcVeAKH9JMXlz2o2BS77dQXowukvpPOuGbSNrUA+Ih 3RE7tjg7yODmW+oCQzJYBSkgWnjWLjRsVZ99eX+YspFhadPXNAJ885z5MGWHi9cWuW0N dg/A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=dHDFB5xW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id fd34-20020a056a002ea200b006cded1e8135si10213448pfb.197.2023.12.05.17.41.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 17:41:07 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=dHDFB5xW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id C624D807DE16; Tue, 5 Dec 2023 17:40:26 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1376485AbjLFBkR (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 20:40:17 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51398 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1376467AbjLFBkQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 20:40:16 -0500 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D79C91B5; Tue, 5 Dec 2023 17:40:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1701826822; x=1733362822; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=84aGtB/DLStDTk9sSVHU7L8uXgDTgViYEQou2kivFBY=; b=dHDFB5xWYL4BuIGHCn7sYe3QNtUaFlANmiOYQ12hfleOVOZPY/FPLgcs 7btnEl64C3rd2hl8iHPB/JX19qE6KgBWm/39xg2f4/nLlTx93Rrx2/Paj MeV0ipXRf/+C9WnJ3gJBBdBvrsR/CJsE8UblKPmhavV1wESgX9pvvWncL 5yG4MZe7/ev346GmRNDrBgwrlsVEvi0BKDc6JJ7eibR2E03wp3eDK0SBA 4bSLCBRoLCm+5U4mPmwZIaOSWyqJwrtKQMyviF66TTxTeOzXdyeU6eWaC RF6KdaoAdwUeI1KZGnJbL4dH3KhQI5jn1y+YdnwNO2WE0WaPBbeNBn2Sm g==; X-IronPort-AV: E=McAfee;i="6600,9927,10915"; a="392853023" X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="392853023" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Dec 2023 17:40:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10915"; a="1018406103" X-IronPort-AV: E=Sophos;i="6.04,254,1695711600"; d="scan'208";a="1018406103" Received: from sohilmeh.sc.intel.com ([172.25.103.65]) by fmsmga006.fm.intel.com with ESMTP; 05 Dec 2023 17:40:06 -0800 From: Sohil Mehta <sohil.mehta@intel.com> To: x86@kernel.org Cc: Thomas Gleixner <tglx@linutronix.de>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, "H . Peter Anvin" <hpa@zytor.com>, Tony Luck <tony.luck@intel.com>, Sohil Mehta <sohil.mehta@intel.com>, Yazen Ghannam <yazen.ghannam@amd.com>, Arnd Bergmann <arnd@arndb.de>, linux-kernel@vger.kernel.org, linux-edac@vger.kernel.org Subject: [PATCH] x86/mce: Update references to the Intel SDM Date: Wed, 6 Dec 2023 01:38:46 +0000 Message-Id: <20231206013846.1859347-1-sohil.mehta@intel.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 05 Dec 2023 17:40:27 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784494809395298415 X-GMAIL-MSGID: 1784494809395298415 |
Series |
x86/mce: Update references to the Intel SDM
|
|
Commit Message
Sohil Mehta
Dec. 6, 2023, 1:38 a.m. UTC
Chapter numbers in the SDM are not expected to be stable. In case of
Machine-Check Architecture, it has moved from chapter 15 to chapter 16
with the recent SDM updates.
Instead of changing the chapter number and having to do it again later,
update the comments with 'Chapter name -> "Sub-section name"' to keep it
easy enough to find the specific reference. Note, this intentionally
skips the intermediate section names to avoid making the comments
unnecessarily wordy.
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
There are other places in arch/x86 that have stale references to the SDM as
well. I am sending an MCE specific patch first to get a pulse. I can send out
more patches if this approach seems reasonable.
I am open to suggestions, is there a better way to do this? Or should we get
rid of the references all together (expect for really the obscure text that
would be hard to find otherwise)?
---
arch/x86/include/asm/mce.h | 2 +-
arch/x86/kernel/cpu/mce/core.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
Comments
On Wed, Dec 06, 2023 at 01:38:46AM +0000, Sohil Mehta wrote:
> I am open to suggestions, is there a better way to do this?
Yes, drop the references and make the comments self-contained. What
point would there even be to refer to a document which can change:
* the chapter number, name, etc
* text
* ...
And I mean that about all technical documents. They do change so either
we can freeze them - upload the exact version to kernel bugzilla and
refer to it - or simply look at the code, try to understand what it
does, see if the comment makes sense or not and if so, explain what it
is commenting properly, without the need to refer to an external doc.
If the comment is outdated, drop it and also explain why you're dropping
it.
Thx.
On 12/6/2023 2:13 AM, Borislav Petkov wrote: > On Wed, Dec 06, 2023 at 01:38:46AM +0000, Sohil Mehta wrote: >> I am open to suggestions, is there a better way to do this? > > Yes, drop the references and make the comments self-contained. Sure, I can take a shot at that. > > If the comment is outdated, drop it and also explain why you're dropping > it. > Sounds good. Sohil
On 12/6/2023 7:08 AM, Sohil Mehta wrote: In an effort to rewrite the below Intel specific comment in mce_is_memory_error(), I think I may have found an issue. It would be really helpful if someone can help check the analysis below. > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 7b397370b4d6..d42122b1afea 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -482,7 +482,8 @@ bool mce_is_memory_error(struct mce *m) > case X86_VENDOR_INTEL: > case X86_VENDOR_ZHAOXIN: > /* > - * Intel SDM Volume 3B - 15.9.2 Compound Error Codes > + * Intel SDM: Machine-Check Architecture -> "Compound Error > + * Codes" > * > * Bit 7 of the MCACOD field of IA32_MCi_STATUS is used for > * indicating a memory error. Bit 8 is used for indicating a The full comment reads as follows: * Bit 7 of the MCACOD field of IA32_MCi_STATUS is used for * indicating a memory error. Bit 8 is used for indicating a * cache hierarchy error. The combination of bit 2 and bit 3 * is used for indicating a `generic' cache hierarchy error * But we can't just blindly check the above bits, because if * bit 11 is set, then it is a bus/interconnect error - and * either way the above bits just gives more detail on what * bus/interconnect error happened. Note that bit 12 can be * ignored, as it's the "filter" bit. */ return (m->status & 0xef80) == BIT(7) || ---> Memory Controller Errors (m->status & 0xef00) == BIT(8) || ---> Cache Hierarchy Errors (m->status & 0xeffc) == 0xc; ---> Generic Cache Hierarchy The code tries to identify the memory and cache errors by masking the status and then comparing based on the bit encodings below. But it seems to be missing the "Extended Memory Errors" encoding which may have been added after the original code was written. Type Form ---- ---- Generic Cache Hierarchy 000F 0000 0000 11LL TLB Errors 000F 0000 0001 TTLL Memory Controller Errors 000F 0000 1MMM CCCC Cache Hierarchy Errors 000F 0001 RRRR TTLL Extended Memory Errors 000F 0010 1MMM CCCC Bus and Interconnect Errors 000F 1PPT RRRR IILL I am not sure what are the practical implications of getting mce_is_memory_error() wrong. (This issue is completely theoretical right now.) Any insights? A couple of other points: - The code seems ripe for a rewrite to be rid of the magic masks and bit comparisons. I am thinking of doing that in a separate patch along side of rewriting the comment. Would that be useful even if no issue exists? - Relying on these bit encodings seems problematic in the long run with the possibility of more things that could always be added. Is there a better way to do it? Sohil
> The code tries to identify the memory and cache errors by masking the > status and then comparing based on the bit encodings below. But it seems > to be missing the "Extended Memory Errors" encoding which may have been > added after the original code was written. That's right. The "Extended Memory Errors" were added for systems that use some "fast" memory as a cache for "slower" memory. Initial use case was for 3D-Xpoint (using this to add capacity rather than making use of persistence). > Type Form > ---- ---- > Generic Cache Hierarchy 000F 0000 0000 11LL > TLB Errors 000F 0000 0001 TTLL > Memory Controller Errors 000F 0000 1MMM CCCC > Cache Hierarchy Errors 000F 0001 RRRR TTLL > Extended Memory Errors 000F 0010 1MMM CCCC > Bus and Interconnect Errors 000F 1PPT RRRR IILL > > I am not sure what are the practical implications of getting > mce_is_memory_error() wrong. (This issue is completely theoretical right > now.) Any insights? This function is used to check whether an address is OS addressable memory (i.e. for a page that could be taken offline). That doesn't apply to the caching use case (the only way to "offline" such a page would be to offline each of the slow memory pages that it might be used for). I'm not quite sure why bit 8 (cache hierarchy error) was added into this check, It would seem to have the same issues as extended memory. > A couple of other points: > > - The code seems ripe for a rewrite to be rid of the magic masks and bit > comparisons. I am thinking of doing that in a separate patch along side > of rewriting the comment. Would that be useful even if no issue exists? Maybe the code would be prettier, or at least easier to read, with some well chosen names for the bits and fields. Only way to tell would be to write the code and see how it looks. > - Relying on these bit encodings seems problematic in the long run with > the possibility of more things that could always be added. Is there a > better way to do it? Computer h/w architecture is going to evolve so change is going to be needed however you code it. -Tony
Thanks Tony for the explanation. It is very helpful. >> Type Form >> ---- ---- >> Generic Cache Hierarchy 000F 0000 0000 11LL >> TLB Errors 000F 0000 0001 TTLL >> Memory Controller Errors 000F 0000 1MMM CCCC >> Cache Hierarchy Errors 000F 0001 RRRR TTLL >> Extended Memory Errors 000F 0010 1MMM CCCC >> Bus and Interconnect Errors 000F 1PPT RRRR IILL >> >> I am not sure what are the practical implications of getting >> mce_is_memory_error() wrong. (This issue is completely theoretical right >> now.) Any insights? > > This function is used to check whether an address is OS addressable memory > (i.e. for a page that could be taken offline). That doesn't apply to the caching > use case (the only way to "offline" such a page would be to offline each of the > slow memory pages that it might be used for). > Makes sense. I am assuming these Extended Memory Errors will not be used anymore (even for CXL.mem type configs) and we don't need to include them in the mce_is_memory_error() check? I'll update the comment accordingly. > I'm not quite sure why bit 8 (cache hierarchy error) was added into this check, > It would seem to have the same issues as extended memory. > From a little bit of digging it seems the check for "cache hierarchy errors" was always there. Commit fa92c5869426 ("x86, mce: Support memory error recovery for both UCNA and Deferred error in machine_check_poll") introduced the original checks but maybe the intention at that time was different? I see that the CEC stuff was added later so maybe the original memory related failures were handled differently? Now, should we remove the cache error related check from mce_is_memory_error()?
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index 6de6e1d95952..35fa25eb815b 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -72,7 +72,7 @@ */ #define MCACOD 0xefff /* MCA Error Code */ -/* Architecturally defined codes from SDM Vol. 3B Chapter 15 */ +/* Architecturally defined error codes from SDM: Machine-Check Architecture */ #define MCACOD_SCRUB 0x00C0 /* 0xC0-0xCF Memory Scrubbing */ #define MCACOD_SCRUBMSK 0xeff0 /* Skip bit 12 ('F' bit) */ #define MCACOD_L3WB 0x017A /* L3 Explicit Writeback */ diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 7b397370b4d6..d42122b1afea 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -482,7 +482,8 @@ bool mce_is_memory_error(struct mce *m) case X86_VENDOR_INTEL: case X86_VENDOR_ZHAOXIN: /* - * Intel SDM Volume 3B - 15.9.2 Compound Error Codes + * Intel SDM: Machine-Check Architecture -> "Compound Error + * Codes" * * Bit 7 of the MCACOD field of IA32_MCi_STATUS is used for * indicating a memory error. Bit 8 is used for indicating a @@ -698,7 +699,8 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b) goto log_it; /* - * Log UCNA (SDM: 15.6.3 "UCR Error Classification") + * Log UCNA (Intel SDM: Machine-Check Architecture -> "UCR + * Error Classification") * UC == 1 && PCC == 0 && S == 0 */ if (!(m.status & MCI_STATUS_PCC) && !(m.status & MCI_STATUS_S))