Skip to content

NAS-120951 / None / Enable APEI/GHES BIOS error detection via EDAC#82

Closed
umsaleem wants to merge 1 commit into
truenas/linux-5.15from
NAS-120951
Closed

NAS-120951 / None / Enable APEI/GHES BIOS error detection via EDAC#82
umsaleem wants to merge 1 commit into
truenas/linux-5.15from
NAS-120951

Conversation

@umsaleem

@umsaleem umsaleem commented Apr 13, 2023

Copy link
Copy Markdown

EDAC supports error reporting from MCA and APEI/GHES. CONFIG_EDAC_GHES was disabled from Debian config so it is being enabled from truenas.config.

Enable APEI/GHES BIOS error detection via EDAC
fbbfa2c 
EDAC supports error reporting from MCA and APEI/GHES.
CONFIG_EDAC_GHES was disabled from Debian config so it is being
enabled from truenas.config.

Signed-off-by: Umer Saleem <[email protected]>
@umsaleem umsaleem requested review from amotin and ixhamza April 13, 2023 12:49
@bugclerk bugclerk changed the title Enable APEI/GHES BIOS error detection via EDAC NAS-120951 / None / Enable APEI/GHES BIOS error detection via EDAC Apr 13, 2023
@bugclerk

bugclerk commented Apr 13, 2023

Copy link
Copy Markdown

Jira URL: https://ixsystems.atlassian.net/browse/NAS-120951

amotin
amotin approved these changes Apr 13, 2023
View reviewed changes

@amotin amotin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no specific objections, but I am suspicious about:
When this option is enabled, it will disable the hardware-driven mechanisms, if a GHES BIOS is detected, entering into the "Firmware First" mode.
Seems like it relies a lot on correct BIOS behavior, and white-lists only HPE servers, at least printing warning on all others. I wonder why Debian disabled it by default.

@umsaleem

umsaleem commented Apr 13, 2023

Copy link
Copy Markdown
Author

Yes, it relies on BIOS heavily and checks for different BIOS errors as well. I think Debian has this disabled by default because of inconsistent behavior, as ghes_edac_register() print following debug messages:

	if (fake) {
		pr_info("This system has a very crappy BIOS: It doesn't even list the DIMMS.\n");
		pr_info("Its SMBIOS info is wrong. It is doubtful that the error report would\n");
		pr_info("work on such system. Use this driver with caution\n");
	} else if (idx < 0) {
		pr_info("This EDAC driver relies on BIOS to enumerate memory and get error reports.\n");
		pr_info("Unfortunately, not all BIOSes reflect the memory layout correctly.\n");
		pr_info("So, the end result of using this driver varies from vendor to vendor.\n");
		pr_info("If you find incorrect reports, please contact your hardware vendor\n");
		pr_info("to correct its BIOS.\n");
		pr_info("This system has %d DIMM sockets.\n", ghes_hw.num_dimms);
	}

@amotin

amotin commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

Yes, I saw those messages too. That makes me think whether we are trying to fix smaller problem by creating a bigger one.

@umsaleem

umsaleem commented Apr 13, 2023

Copy link
Copy Markdown
Author

Looking at it more, I think it would be a bad user experience if this reports some errors that aren't there, and it is clearly documented that result can vary from vendor to vendor. So I think we should not enable it.

I also tested it on a MiniXP and found the following warning reported by it:

[    0.577283] [Firmware Bug]: APEI: Invalid physical address in GAR [0x0/0/0/0/0]

@umsaleem umsaleem closed this Apr 14, 2023
@umsaleem umsaleem deleted the NAS-120951 branch April 14, 2023 05:34
umsaleem pushed a commit that referenced this pull request Feb 7, 2024
@gregkh
bpf: Fix a few selftest failures due to llvm18 change
e02851a 
[ Upstream commit b16904f ]

With latest upstream llvm18, the following test cases failed:

  $ ./test_progs -j
  #13/2    bpf_cookie/multi_kprobe_link_api:FAIL
  #13/3    bpf_cookie/multi_kprobe_attach_api:FAIL
  #13      bpf_cookie:FAIL
  #77      fentry_fexit:FAIL
  #78/1    fentry_test/fentry:FAIL
  #78      fentry_test:FAIL
  #82/1    fexit_test/fexit:FAIL
  #82      fexit_test:FAIL
  #112/1   kprobe_multi_test/skel_api:FAIL
  #112/2   kprobe_multi_test/link_api_addrs:FAIL
  [...]
  #112     kprobe_multi_test:FAIL
  #356/17  test_global_funcs/global_func17:FAIL
  #356     test_global_funcs:FAIL

Further analysis shows llvm upstream patch [1] is responsible for the above
failures. For example, for function bpf_fentry_test7() in net/bpf/test_run.c,
without [1], the asm code is:

  0000000000000400 <bpf_fentry_test7>:
     400: f3 0f 1e fa                   endbr64
     404: e8 00 00 00 00                callq   0x409 <bpf_fentry_test7+0x9>
     409: 48 89 f8                      movq    %rdi, %rax
     40c: c3                            retq
     40d: 0f 1f 00                      nopl    (%rax)

... and with [1], the asm code is:

  0000000000005d20 <bpf_fentry_test7.specialized.1>:
    5d20: e8 00 00 00 00                callq   0x5d25 <bpf_fentry_test7.specialized.1+0x5>
    5d25: c3                            retq

... and <bpf_fentry_test7.specialized.1> is called instead of <bpf_fentry_test7>
and this caused test failures for #13/#77 etc. except #356.

For test case #356/17, with [1] (progs/test_global_func17.c)), the main prog
looks like:

  0000000000000000 <global_func17>:
       0:       b4 00 00 00 2a 00 00 00 w0 = 0x2a
       1:       95 00 00 00 00 00 00 00 exit

... which passed verification while the test itself expects a verification
failure.

Let us add 'barrier_var' style asm code in both places to prevent function
specialization which caused selftests failure.

  [1] llvm/llvm-project#72903

Signed-off-by: Yonghong Song <[email protected]>
Signed-off-by: Daniel Borkmann <[email protected]>
Link: https://lore.kernel.org/bpf/[email protected]
Signed-off-by: Sasha Levin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amotin amotin amotin approved these changes
@ixhamza ixhamza Awaiting requested review from ixhamza

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@umsaleem @bugclerk @amotin