hey and thanks for working on getting stuff back upstream!
i have very little time at the moment (and the next few weeks) so i just skimmed over the patches a few days ago and will just mention the main points i noticed.
On Fri, 2 Dec 2011 17:38:20 -0800 David Hendricks dhendrix@google.com wrote:
The following patches (attached and in-lined below) add support for "graceful" handling of certain errors and in particular the read/write errors that occur on newer Intel chipsets. With a little work, it could be extended to handle SPI hardware write protection schema, too.
1. the patches seemed to be divided in a little bit odd way, but that might actually be because i did not read that carefully. btw please post one patch per mail so that patchwork recognize them all.
Note: I think we should probably add a "I_want_a_brick" programmer parameter for those daring enough to try this. I suspect certain OEM tools use unknown Mebx commandshttp://support.dell.com/support/edocs/systems/latd630/en/amt/MEBX.htmto disable ME read/write protection when they do updates. Perhaps that stuff can be added later as a board_enable thing.
2. i dont know enough about MEBx, but i have not found any mention of it in any official documentation about ME unlocking - public or otherwise. the user interface as shown on the dell screenshot might not be accessible for users. i need to test if the MEBx settings have any influence on the ICH registers on my ICH10 based (and locked) intel board. wont happen soon though.
3. the official way to disabled the ME locking is via an HECI™/MEI™ service™ with™ the™ HMRFPO™ Enable™ Intel® command™. the MEI protocol is packet-based and well documented including an open source linux kernel module. i have ported this to flashrom and will try to get it into a mergeable state in the future. the only missing part then is the right MEI command/reply sequence to unlock the ME region. the integration would not use board enables but the unlocking would be done automatically in ich_init (and re-enabling in shutdown, if we could figure out how... not that important imho because it is security be obscurity anyway). the HMRFPO command is documented in the BIOS writer guide(s).
With the patches applied, the low-level ICH code will handle all the details of checking the address associated with an opcode against the permissions set in the flash descriptor. If an operation cannot be performed on a given region, an error code is propagated up the stack so that high-level read/write logic can decide to skip that region. As is, the patch will make the high-level logic fill in unreadable regions with 0xff bytes (for reads) and will quietly skip over unwriteable regions (for writes).
The biggest downside is, of course, that read/write operations no longer do exactly what one might expect. A full read operation (flashrom -r) will give you a ROM-sized image with 0xff bytes wherever read is forbidden (e.g. where the management engine firmware lives). A full write operation will skip forbidden regions (e.g. flash descriptor and ME) which may leave stuff in an inconsistent and potentially unusable state (thanks, Intel!).
4. i dont like the overall approach of the patches at all. they complicate the most complicated parts of flashrom even more (chunked read/erase/write) for little gain for an even littler target audience (no offense google :). the init code can already tell the upper level code which parts are accessible and which are not. there is no need to fiddle around with the opcodes on the fly (in addition to what we do to reprogram opcodes because we are forced to... :)
i would like to do this in a less intrusive and more generic way by means of layout(file)s. patches for enabling the usage of layouts in all program modes exists already and would give anyone who dares an easy way to operate on any accessible region on its own (reviews are welcome! :P). an additional layout file generation by programmer init code (when told so) or a generic automagic mode that queries the programmer, the flash chip and maybe other modules for access rights to plan the whole operation before really issuing read/erase/write commands (a level above your implementation) would give us the same functionality, but also would be useful for non-ich users, generic to be extended in the future and most importantly: it would not touch the core read/write functions.