| Message ID | 20231206013846.1859347-1-sohil.mehta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mce: Update references to the Intel SDM | expand |
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))
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(-)