futility: updater: Enable ec_partial_recovery quirk by default
Enable QUIRK_EC_PARTIAL_RECOVERY (ec_partial_recovery=1) by default so
full/recovery EC firmware updates only flash WP_RO when the EC is active
in RW with EC software sync enabled, leaving EC_RW to be updated by EC
software sync on the next reboot.
During a full EC update (EC_RECOVERY_FULL, RO+RW), flashrom's
cros_ec_prepare() sees EC_IMAGE_RO | EC_IMAGE_RW and forces an
unconditional sysjump from RW to RO when EXEC_IN_RAM is unsupported (such
as on ITE IT8xxx2 ECs), resetting the EC/TCPC or dropping host
communication while the AP is in S0 (b/574563907). Flashrom commit
af7ed034 (b/278652302) added support for RO-only updates (-i WP_RO,
EC_RECOVERY_RO) to skip the sysjump when the EC is already running in RW,
allowing WP_RO to be updated in place while EC software sync updates
EC_RW on reboot.
Callers or board CBFS configs that require an immediate full RO+RW write
can still opt out via --quirks=ec_partial_recovery=0.
BRANCH=none
BUG=b:574563907
TEST=make runfutiltests
TEST=futility update -a /usr/sbin/chromeos-firmwareupdate --mode=recovery
--force on dirks (IT81302) updates WP_RO without crashing EC in RW
Change-Id: Iadcb674d804e3397075fe93462199424ffa1a564
Signed-off-by: Jon Murphy <jpmurphy@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/8609747
Reviewed-by: Grzegorz Bernacki <bernacki@google.com>
diff --git a/futility/updater_dut.c b/futility/updater_dut.c
index 4946201..f1e950f 100644
--- a/futility/updater_dut.c
+++ b/futility/updater_dut.c
@@ -42,6 +42,7 @@
return VbSetSystemPropertyString(key, value);
}
+test_mockable
int dut_get_property_string(const char *key, char *dest, size_t size,
struct updater_config *cfg)
{
@@ -62,6 +63,7 @@
return VbSetSystemPropertyInt(key, value);
}
+test_mockable
int dut_get_property_int(const char *key, struct updater_config *cfg)
{
if (cfg->dut_is_remote) {
diff --git a/futility/updater_quirks.c b/futility/updater_quirks.c
index ed99104..36db084 100644
--- a/futility/updater_quirks.c
+++ b/futility/updater_quirks.c
@@ -232,14 +232,13 @@
*
* Some EC will reset TCPC when doing sysjump, and will make rootfs unavailable
* if the system was boot from USB, or other unexpected issues even if the
- * system was boot from internal disk. To prevent that, try to partial update
- * only RO and expect EC software sync to update RW later, or perform EC RO
- * software sync.
- *
- * Note: EC RO software sync was not fully tested and may cause problems
- * (b/218612817, b/187789991).
- * RO-update (without extra sysjump) needs support from flashrom and is
- * currently disabled.
+ * system was boot from internal disk. In addition, full RO+RW updates force an
+ * unconditional sysjump from RW to RO in flashrom's cros_ec_prepare(), which
+ * resets XIP ECs (such as ITE IT8xxx2) or drops host communication while the
+ * AP is in S0 (b/574563907). With flashrom support for RO-only updates without
+ * sysjump when already in RW (b/278652302), partial update only WP_RO when EC
+ * is active in RW with software sync enabled, and expect EC software sync to
+ * update RW on the next boot.
*
* Returns:
* EC_RECOVERY_FULL to indicate a full recovery is needed.
@@ -426,6 +425,7 @@
quirks->name = "ec_partial_recovery";
quirks->help = "chromium/1024401; recover EC by partial RO update.";
quirks->apply = quirk_ec_partial_recovery;
+ quirks->value = 1;
quirks = &cfg->quirks[QUIRK_OVERRIDE_CUSTOM_LABEL];
quirks->name = "override_custom_label";
diff --git a/tests/futility/test_updater_utils.c b/tests/futility/test_updater_utils.c
index 2eec2df..58a095e 100644
--- a/tests/futility/test_updater_utils.c
+++ b/tests/futility/test_updater_utils.c
@@ -5,6 +5,8 @@
#include "unit_tests.h"
#include "updater_utils.c"
+#include "crossystem.h"
+#include "vboot_struct.h"
#define IMAGE_MAIN GET_WORK_COPY_TEST_DATA_FILE_PATH("image.bin")
#define ARCHIVE GET_WORK_COPY_TEST_DATA_FILE_PATH("images.zip")
@@ -613,6 +615,97 @@
UNIT_TEST_RETURN;
}
+static bool mock_dut_props;
+static int mock_vdat_flags;
+static const char *mock_ecfw_act;
+
+int dut_get_property_int(const char *key, struct updater_config *cfg)
+{
+ if (cfg->dut_is_remote)
+ return -1;
+ if (mock_dut_props) {
+ if (strcmp(key, "vdat_flags") == 0)
+ return mock_vdat_flags;
+ return -1;
+ }
+ return VbGetSystemPropertyInt(key);
+}
+
+int dut_get_property_string(const char *key, char *dest, size_t size,
+ struct updater_config *cfg)
+{
+ if (cfg->dut_is_remote)
+ return -1;
+ if (mock_dut_props) {
+ if (strcmp(key, "ecfw_act") == 0 && mock_ecfw_act) {
+ strncpy(dest, mock_ecfw_act, size);
+ dest[size - 1] = '\0';
+ return 0;
+ }
+ return -1;
+ }
+ return VbGetSystemPropertyString(key, dest, size);
+}
+
+static enum unit_result test_ec_partial_recovery(void)
+{
+ UNIT_TEST_BEGIN;
+ struct updater_config *cfg = updater_new_config();
+ struct vb2_gbb_header *gbb = NULL; /* Do not free. */
+ FmapAreaHeader *ah = NULL; /* Do not free. */
+
+ mock_dut_props = true;
+ UNIT_ASSERT(cfg != NULL);
+ TEST_EQ(get_config_quirk(QUIRK_EC_PARTIAL_RECOVERY, cfg), 1,
+ "Default ec_partial_recovery quirk is enabled");
+
+ UNIT_ASSERT(load_firmware_image(&cfg->image, IMAGE_MAIN, NULL) == 0);
+ UNIT_ASSERT(load_firmware_image(&cfg->ec_image, IMAGE_MAIN, NULL) == 0);
+
+ mock_vdat_flags = VBSD_EC_SOFTWARE_SYNC;
+ mock_ecfw_act = "RW";
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_RO,
+ "EC partial recovery: EC in RW with software sync enabled -> WP_RO only");
+
+ mock_ecfw_act = "RO";
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: EC in RO -> full update");
+
+ mock_ecfw_act = "RW";
+ mock_vdat_flags = 0;
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: software sync not enabled -> full update");
+
+ mock_vdat_flags = -1;
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: unknown vdat_flags -> full update");
+
+ mock_vdat_flags = VBSD_EC_SOFTWARE_SYNC;
+ gbb = (struct vb2_gbb_header *)find_gbb(&cfg->image);
+ UNIT_ASSERT(gbb != NULL);
+ gbb->flags |= VB2_GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC;
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: GBB disables software sync -> full update");
+ gbb->flags &= ~VB2_GBB_FLAG_DISABLE_EC_SOFTWARE_SYNC;
+
+ cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].value = 0;
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: quirk disabled (=0) -> full update");
+ cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].value = 1;
+
+ UNIT_ASSERT(fmap_find_by_name(cfg->ec_image.data, cfg->ec_image.size,
+ cfg->ec_image.fmap_header, "WP_RO", &ah) != NULL);
+ ah->area_name[0] = '\0';
+ TEST_EQ(cfg->quirks[QUIRK_EC_PARTIAL_RECOVERY].apply(cfg), EC_RECOVERY_FULL,
+ "EC partial recovery: missing WP_RO section -> full update");
+
+unit_cleanup:
+ mock_dut_props = false;
+ if (cfg)
+ updater_delete_config(cfg);
+ UNIT_TEST_RETURN;
+}
+
int main(int argc, char *argv[])
{
if (prepare_test_data() == UNIT_FAIL) {
@@ -631,6 +724,7 @@
test_preserve_firmware_section();
test_gbb();
test_misc();
+ test_ec_partial_recovery();
return !gTestSuccess;
}
