Firmware: Remove get_sw_write_protect_state |
||||
Issue descriptionCurrently in ChromeOS there's no a clear sign of "WP is enabled or not", except the message reported by firmware updater. However, sometimes it's very helpful if we can figure out if WP (HW + SW) is enabled or not in feedback or bug reports, which may contain only crossystem output. In crossystem, there is a sw_wpsw_boot that should tell the AP firmware SPI WP state, but it's implemented starting from Baytrail (and the crossytem help message said it only works for BYT). Looking at code, it's implemented by baytrail, APL, SKL, BDW - all Intel platforms. Since Coreboot does support SPI now and should be able to get SRP status, can we make it always implemented, even adding it to FAFT? It would be even better if crossystem can error out if the platform "does not support SW_WPSW_BOOT".
,
Jul 14 2017
(FWIW, I don't know why this field was added in the first place, and why it is implemented on Bay Trail. If it's only intended to tell that state to the kernel, that would generally seem like the wrong way to do that. b/35510092 suggests that it was meant for some feature that was abandoned. Maybe we should just remove it, at least the crossystem part, to avoid confusion.)
,
Jul 15 2017
> This information is already included in all feedback reports as part of the 'verified boot' section (debug_vboot_noisy.log). Many non-engineers find it pretty difficult to figure out if WP is enabled or not from that huge log. Also, what vboot reports is "current WP", not "boot WP". And I often receiving mails asking me "is sw_wpsw_boot reflecting right WP state on this device?", especially from partners. > Why should the firmware do stuff for the kernel/OS that the latter can easily figure out on its own? Technically it's different. "WP is enabled or not at boot" is only known by vboot firmware, just like devsw_boot vs devsw_cur. Also, the only way to get "current SW WP state" is flashrom, which is known to create locks, need initial calibration, ... etc pretty slow. A crossystem call with cached VDAT is simple and fast. It's also easier for how to teach partner to examine the devices' WP state. Compare this: 1. Tell me your HW and SW WP states: crossystem wpsw_boot flashrom -p host --wp-status # Another cycle for Google to interpret the output for very dumb partners. 2. Check your WP state: crossystem wpsw_boot sw_wpsw_boot 1 = on, 0= off > Maybe we should just remove it I'm ok if you still prefer to remove it, that's better than leaving an indeterministic command there.
,
Jul 15 2017
So an alternative is, remove this command, and include WP info somewhere in chrome://system for example bios_info.
,
Jul 17 2017
> Many non-engineers find it pretty difficult to figure out if WP is enabled or not from that huge log. Is this really such a common problem that has to be handled by non-engineers all the time? I would expect software WP state to matter very rarely in debugging, and to only really be interesting to the few people who work on firmware updates and should know about debug_vboot_noisy.log. > Also, what vboot reports is "current WP", not "boot WP". True, but when does that ever matter? The firmware never writes the RO region anyway. Why do you need to know the soft-WP state at boot? (Also, the difference here is not between "boot" and "current", it's between "500ms after boot" and "5000ms after boot". The value is still only logged once, when dev_debug_vboot runs. > So an alternative is, remove this command, and include WP info somewhere in chrome://system for example bios_info. If you really need this, you could add another direct flashrom call to platform2/userfeedback/scripts/bios_info. That's better than making a layering violation for convenience. Like dev_debug_vboot, it's not in the critical boot path so it shouldn't bother anyone. > I'm ok if you still prefer to remove it, that's better than leaving an indeterministic command there. Yeah, let's remove it.
,
Jul 18 2017
> Is this really such a common problem that has to be handled by non-engineers all the time? Firmware AU logic are pending on WP state, and when there's some AU issues, I usually get asked for this question for how to figure out if updater will update RO -> need info of if WP was enabled. > Yeah, let's remove it. Agree.
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/8b71425257d251858410de71efcf389df8b200d2 commit 8b71425257d251858410de71efcf389df8b200d2 Author: Julius Werner <jwerner@chromium.org> Date: Tue Jul 18 09:36:16 2017 crossystem: Remove defunct sw_wpsw_boot field The sw_wpsw_boot field only ever worked correctly on some platforms. It also isn't used anywhere in the codebase (only other reference is a comment about how it doesn't always work in factory_installer.sh), and it's no longer clear what it was meant for in the first place (b/35510092 hints at needing it for some planned feature that was never implemented). Let's get rid of it to avoid confusing people. If userspace tools need to know the software write-protect state, they can instead run flashrom directly. For feedback reports, this output is already included in the "verified boot" section. BRANCH=none BUG=chromium:508269, chromium:742685 TEST=none Change-Id: I8975b1e2c8e604b4cb48d092c13b923b4db2d207 Signed-off-by: Julius Werner <jwerner@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/575389 Reviewed-by: Hung-Te Lin <hungte@chromium.org> Reviewed-by: Aaron Durbin <adurbin@chromium.org> Reviewed-by: Randall Spangler <rspangler@chromium.org> [modify] https://crrev.com/8b71425257d251858410de71efcf389df8b200d2/utility/crossystem.c [modify] https://crrev.com/8b71425257d251858410de71efcf389df8b200d2/host/lib/crossystem.c
,
Mar 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b commit 7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b Author: Randall Spangler <rspangler@chromium.org> Date: Sun Mar 04 23:57:57 2018 firmware: Remove deprecated SW_WP_ENABLED flag This was deprecated months ago in crossystem, and isn't set by depthcharge or coreboot. Remove the flag from vboot as well, keeping only a reminder in vboot_struct.h so we don't reuse the VbSharedData bit. BUG= chromium:742685 BRANCH=none TEST=make runtests Change-Id: Ifa928e8ec4d999c524c6f4168695859261f384c9 Signed-off-by: Randall Spangler <rspangler@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/947256 Reviewed-by: Julius Werner <jwerner@chromium.org> [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/firmware/include/vboot_struct.h [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/firmware/2lib/include/2api.h [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/firmware/lib/ec_sync.c [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/tests/ec_sync_tests.c [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/firmware/lib/vboot_api_kernel.c [modify] https://crrev.com/7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b/firmware/include/vboot_api.h
,
Mar 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/1efb5acb5d94900048d55cba5c596a33d30973dc commit 1efb5acb5d94900048d55cba5c596a33d30973dc Author: Randall Spangler <rspangler@chromium.org> Date: Mon Mar 05 20:00:34 2018 firmware: Remove deprecated SW_WP_ENABLED flag This was deprecated months ago in crossystem, and isn't set by depthcharge or coreboot. Remove the flag from vboot as well, keeping only a reminder in vboot_struct.h so we don't reuse the VbSharedData bit. BUG= chromium:742685 BRANCH=none TEST=make runtests Change-Id: Ifa928e8ec4d999c524c6f4168695859261f384c9 Signed-off-by: Randall Spangler <rspangler@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/947256 Reviewed-by: Julius Werner <jwerner@chromium.org> (cherry picked from commit 7bb45097af1e8b5f2bcf8e7a8bc6557c6505693b) Reviewed-on: https://chromium-review.googlesource.com/949621 Reviewed-by: Furquan Shaikh <furquan@chromium.org> Commit-Queue: Furquan Shaikh <furquan@chromium.org> Tested-by: Furquan Shaikh <furquan@chromium.org> Trybot-Ready: Furquan Shaikh <furquan@chromium.org> [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/firmware/include/vboot_struct.h [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/firmware/2lib/include/2api.h [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/firmware/lib/ec_sync.c [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/tests/ec_sync_tests.c [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/firmware/lib/vboot_api_kernel.c [modify] https://crrev.com/1efb5acb5d94900048d55cba5c596a33d30973dc/firmware/include/vboot_api.h
,
Mar 23 2018
I think everything that needed to be done here has been done. |
||||
►
Sign in to add a comment |
||||
Comment 1 by jwer...@chromium.org
, Jul 14 2017