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 <[email protected]>
---
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(-)
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))
--
2.34.1
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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()?