-
-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch from flashrom to flashprog #1769
Changes from 2 commits
0cd00f2
f8eb0a2
04bb3be
58081cb
36aea9e
e26cd76
8b524be
56ae5f7
efb6126
668f697
af2c45b
a407462
bd7b1c8
29c97e8
6fe86df
75f1c2a
aad131b
5cba23e
6c16a50
d5ddab5
2c6eec5
32d5173
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
modules-$(CONFIG_FLASHPROG) += flashprog | ||
|
||
flashprog_depends := pciutils $(musl_dep) | ||
|
||
flashprog_version := 9dc6d843b0678001c9baf0e8602ecb25b16329d2 | ||
flashprog_dir := flashprog-$(flashprog_version) | ||
flashprog_tar := $(flashprog_dir).tar.gz | ||
flashprog_url := https://github.com/SourceArcade/flashprog/archive/$(flashprog_version).tar.gz | ||
flashprog_hash := fa4ddf3b60314994a37e4599122ae4c7f42135c13c782e580bc580d715cfa2fc | ||
|
||
# Default options for flashprog | ||
flashprog_cfg := \ | ||
WARNERROR=no \ | ||
CONFIG_NOTHING=yes \ | ||
CONFIG_INTERNAL=yes \ | ||
CONFIG_INTERNAL_X86=yes \ | ||
|
||
ifeq "$(CONFIG_TARGET_ARCH)" "ppc64" | ||
flashprog_cfg := \ | ||
WARNERROR=no \ | ||
CONFIG_NOTHING=yes \ | ||
CONFIG_LINUX_MTD=yes | ||
endif | ||
|
||
#Only enable AST1100 if requested per board configs | ||
ifeq "$(CONFIG_FLASHPROG_AST1100)" "y" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patch missing. Even i currently activated under server board and passed at make, doesn't o anything without https://github.com/linuxboot/heads/blob/master/patches/flashrom-b1f858f65b2abd276542650d8cb9e382da258967/0100-enable-kgpe-d16.patch missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can have a look at it. AFAIR, nobody made a real effort to upstream this. If I rebase this, we'd still need somebody to test it thoroughly. We are quite pedantic when it comes to internal flashing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @i-c-o-n it is a problem of upstreaming indeed, keeping track of patches downstream never proved to do any good. If not under flashprog/flashrom: this means end users need to flash bmc externally which is not a desirable thing. Also as you may know, that bmc port is old but still in charge of best fan control out there for the d16. Having heads flash through https://github.com/linuxboot/heads/pull/1769/files#diff-08a8a9080a4ca4377e0de4dbef6bd94c84209343b4e12a8982352a1c1b09b71b would be useful. That would be ideal, but not a blocker for this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @i-c-o-n do you see anything else that should be improved in the flashprog makefile for size/functionalities inclusion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you see anything that should be added in board config for flash options that should be passed? Something that would make speed of internal flashing faster? Also not a requirement for this PR, but a nice thing to track in separate issue for improvements, let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, the current selection is already the minimum AFAICT. Anything else would need some hacking on the code. One low hanging fruit: For targets that use Intel hwseq or linux_mtd, you could drop everything else from
|
||
flashprog_cfg += CONFIG_AST1100=yes | ||
endif | ||
|
||
flashprog_target := \ | ||
$(MAKE_JOBS) \ | ||
CFLAGS="-Os -I$(INSTALL)/include/pci" \ | ||
DESTDIR="$(INSTALL)" \ | ||
INSTALL="$(INSTALL)" \ | ||
LDFLAGS="-L$(INSTALL)/lib" \ | ||
PREFIX="$(INSTALL)" \ | ||
$(CROSS_TOOLS) \ | ||
$(flashprog_cfg) \ | ||
flashprog | ||
|
||
flashprog_output := \ | ||
flashprog | ||
|
||
flashprog_libraries := \ | ||
|
||
flashprog_configure := |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-c-o-n still good commit? would wp_cli branch need update prior of merging? next iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sitting ducks for review. I don't expect bigger changes.
@SergiiDmytruk know any users of the block protection that could give some feedback on the CLI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe users who commented in flashrom/flashrom#185? My impression on WP is that people are interested in it for a relatively short period of time until they realize its practical limitations @i-c-o-n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yeah, it's not just a give-me-security switch. Modern chips don't even have a /WP pin (because they prefer quad-i/o). Biggest bummer though is the lacking support to read/write all registers on Intel platforms (I have one more idea to try but little hope), otherwise one could use the protect-until-powercycle feature.
@ln2max, @jtf7 looking for user test and feedback on the WP CLI available through this PR, or this branch. It's not too different from flashrom's, e.g.
wp --status
instead of--wp-status
. And it supports only one command at a time to be sure about the execution order.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-c-o-n are you talking about chipset locking (PR0) as per #1373 (comment) OP's todo pointing to @root-hardenedvault 's x11 downstream (non-upstreamed) work for Haswell+ platforms https://github.com/hardenedvault/vaultboot/blob/master/patches/coreboot/0001-x11.patch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there's another feature of the flash chip itself. Most common for SPI flashes is that you can configure which blocks to protect and then there are four states:
For 2. we need the pin and support or modification of the mainboard. 3. and 4. often require to write a bit in a secondary register. Problem is so far nobody knows how to write that register with Intel's
hwseq
, which is the only option since Skylake. I don't think Intel implemented that. They just want you to use their protection mechanisms like PR0; those are, however, very inconvenient when Intel FSP is involved.Something like that hardenedvault patch wouldn't work on (at least some) newer systems because FSP would lock PR0 too early, unconditionally. Security with Intel is never easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(tlaurion edit: tagging @i-c-o-n) :
I thought chips supporting this feature are fairly uncommon. I remember seeing notes in a bunch of datasheets to the effect of "this feature is available per request" (maybe all from a single vendor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, my recollection may be biased by our own chip selection (we used this feature on an AMD based system almost 15 years ago). I thought it's very common for Winbond; and GD and XMC for instance seem to copy everything from them judging from datasheets. Macronix seems to be an exception. This "available per request" does ring a bell, but I usually see this regarding 4., the permanent locking.
(tlaurion edit : please tag people to trigger a notification to external people directly. Here @SergiiDmytruk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-c-o-n you might want to look at #1818 which thanks to @miczyg1 patchset over dasharo fork, brings PR0 to Skylake+
Will need to update https://github.com/linuxboot/heads/pull/1769/files#diff-546b89876634c383a0dd21ef655ef6d3c5c62c69e33a2f6910dea84ac1d49eea to reflect this fact, since merging this PR will happen soon. Just need to make sure newer boards addition (optiplex only if I recall well) also need to be switched to flashprog and work as expected (should be).
@i-c-o-n : should I point to newer flashprog commit in this PR, or do so in another PR?