vboot: do not request recovery for VB2_REQUEST_* from VB2_TRY()
When the returned value is not an error (such as VB2_REQUEST_*), do not
call vb2api_fail() from VB2_TRY() to request recovery.
During EC sync, instead of explicitly setting VB2_NV_RECOVERY_REQUEST in
nvdata to request recovery, utilize vb2api_fail() instead to try the
other AP slot before giving up on EC sync and going into recovery.
In addition, remove the retry of EC RO sync for the following reasons.
EC sync rarely fails, and even if it does, it's not very likely to be a
transient problem that disappears on the next attempt. Besides, the RO
sync is just a debug feature that only people who have a servo attached
and can manually reflash should be using. Therefore, the retry is
removed and hence we no longer need to restore the recovery request in
nvdata.
BRANCH=none
BUG=chromium:1075488
TEST=make runtests
Change-Id: I9ad8e5e0886679a9a342449553170317b010237b
Signed-off-by: Yu-Ping Wu <yupingso@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2145272
Reviewed-by: Joel Kitching <kitching@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
diff --git a/firmware/2lib/2ec_sync.c b/firmware/2lib/2ec_sync.c
index 7106790..9c49ceb 100644
--- a/firmware/2lib/2ec_sync.c
+++ b/firmware/2lib/2ec_sync.c
@@ -17,52 +17,6 @@
VB2_SD_FLAG_ECSYNC_EC_RO : VB2_SD_FLAG_ECSYNC_EC_RW)
/**
- * Set the RECOVERY_REQUEST flag in NV space
- */
-static void request_recovery(struct vb2_context *ctx, uint32_t recovery_request)
-{
- VB2_DEBUG("request_recovery(%u)\n", recovery_request);
-
- vb2_nv_set(ctx, VB2_NV_RECOVERY_REQUEST, recovery_request);
-}
-
-/**
- * Whether a reboot is requested.
- *
- * When this function returns 1, rv isn't considered an error and hence
- * recovery mode shouldn't be triggered. This includes the following
- * situations:
- *
- * VBERROR_REBOOT_REQUIRED: When we need to display firmware sync screen, but
- * the display hasn't been initialized, a reboot will be required.
- *
- * VBERROR_EC_REBOOT_TO_RO_REQUIRED: The EC may know it needs a reboot. It may
- * need to reboot to unprotect the region before updating, or may need to reboot
- * after updating. When EC update fails, it also needs to reboot.
- */
-static inline int reboot_requested(vb2_error_t rv)
-{
- return rv == VB2_REQUEST_REBOOT || rv == VB2_REQUEST_REBOOT_EC_TO_RO;
-}
-
-/**
- * Wrapper around vb2ex_ec_protect() which sets recovery reason on error.
- */
-static vb2_error_t protect_ec(struct vb2_context *ctx,
- enum vb2_firmware_selection select)
-{
- vb2_error_t rv = vb2ex_ec_protect(select);
-
- if (reboot_requested(rv)) {
- VB2_DEBUG("vb2ex_ec_protect() needs reboot: %#x\n", rv);
- } else if (rv != VB2_SUCCESS) {
- VB2_DEBUG("vb2ex_ec_protect() returned %#x\n", rv);
- request_recovery(ctx, VB2_RECOVERY_EC_PROTECT);
- }
- return rv;
-}
-
-/**
* Print a hash to debug output
*
* @param hash Pointer to the hash
@@ -112,13 +66,8 @@
/*
* Get expected EC hash and length.
*/
- rv = vb2ex_ec_get_expected_image_hash(select, &hexp, &hexp_len);
- if (rv) {
- VB2_DEBUG("vb2ex_ec_get_expected_image_hash() returned %#x\n",
- rv);
- request_recovery(ctx, VB2_RECOVERY_EC_EXPECTED_HASH);
- return VB2_ERROR_EC_HASH_EXPECTED;
- }
+ VB2_TRY(vb2ex_ec_get_expected_image_hash(select, &hexp, &hexp_len),
+ ctx, VB2_RECOVERY_EC_EXPECTED_HASH);
VB2_DEBUG("Hexp %10s: ", image_name_to_string(select));
print_hash(hexp, hexp_len);
@@ -135,8 +84,9 @@
if (hmir_len != hexp_len) {
VB2_DEBUG("Hmir size (%d) != Hexp size (%d)\n",
hmir_len, hexp_len);
- request_recovery(ctx, VB2_RECOVERY_EC_HASH_SIZE);
- return VB2_ERROR_EC_HASH_SIZE;
+ rv = VB2_ERROR_EC_HASH_SIZE;
+ vb2api_fail(ctx, VB2_RECOVERY_EC_HASH_SIZE, rv);
+ return rv;
}
if (vb2_safe_memcmp(hmir, hexp, hexp_len)) {
VB2_DEBUG("Hmir != Hexp. Update Hmir.\n");
@@ -148,12 +98,8 @@
/*
* Get effective EC hash and length.
*/
- rv = vb2ex_ec_hash_image(select, &heff, &heff_len);
- if (rv) {
- VB2_DEBUG("vb2ex_ec_hash_image() returned %#x\n", rv);
- request_recovery(ctx, VB2_RECOVERY_EC_HASH_FAILED);
- return VB2_ERROR_EC_HASH_IMAGE;
- }
+ VB2_TRY(vb2ex_ec_hash_image(select, &heff, &heff_len),
+ ctx, VB2_RECOVERY_EC_HASH_FAILED);
VB2_DEBUG("Heff %10s: ", image_name_to_string(select));
print_hash(heff, heff_len);
@@ -161,8 +107,9 @@
if (heff_len != hexp_len) {
VB2_DEBUG("EC uses %d-byte hash but AP-RW contains %d bytes\n",
heff_len, hexp_len);
- request_recovery(ctx, VB2_RECOVERY_EC_HASH_SIZE);
- return VB2_ERROR_EC_HASH_SIZE;
+ rv = VB2_ERROR_EC_HASH_SIZE;
+ vb2api_fail(ctx, VB2_RECOVERY_EC_HASH_SIZE, rv);
+ return rv;
}
if (vb2_safe_memcmp(heff, hexp, hexp_len)) {
@@ -184,25 +131,16 @@
enum vb2_firmware_selection select)
{
struct vb2_shared_data *sd = vb2_get_sd(ctx);
- vb2_error_t rv;
VB2_DEBUG("Updating %s...\n", image_name_to_string(select));
-
- rv = vb2ex_ec_update_image(select);
- if (rv != VB2_SUCCESS) {
- VB2_DEBUG("vb2ex_ec_update_image() returned %#x\n", rv);
- if (!reboot_requested(rv))
- request_recovery(ctx, VB2_RECOVERY_EC_UPDATE);
- return rv;
- }
+ VB2_TRY(vb2ex_ec_update_image(select), ctx, VB2_RECOVERY_EC_UPDATE);
/* Verify the EC was updated properly */
sd->flags &= ~SYNC_FLAG(select);
- if (check_ec_hash(ctx, select) != VB2_SUCCESS)
- return VB2_REQUEST_REBOOT_EC_TO_RO;
+ VB2_TRY(check_ec_hash(ctx, select));
if (sd->flags & SYNC_FLAG(select)) {
VB2_DEBUG("Failed to update\n");
- request_recovery(ctx, VB2_RECOVERY_EC_UPDATE);
+ vb2api_fail(ctx, VB2_RECOVERY_EC_UPDATE, 0);
return VB2_REQUEST_REBOOT_EC_TO_RO;
}
@@ -227,13 +165,9 @@
* resets. So, we trust what EC-RW says. If it lies it's in RO, we'll
* flash RW while it's in RW.
*/
- vb2_error_t rv = vb2ex_ec_running_rw(&in_rw);
/* If we couldn't determine where the EC was, reboot to recovery. */
- if (rv != VB2_SUCCESS) {
- VB2_DEBUG("vb2ex_ec_running_rw() returned %#x\n", rv);
- request_recovery(ctx, VB2_RECOVERY_EC_UNKNOWN_IMAGE);
- return VB2_REQUEST_REBOOT_EC_TO_RO;
- }
+ VB2_TRY(vb2ex_ec_running_rw(&in_rw),
+ ctx, VB2_RECOVERY_EC_UNKNOWN_IMAGE);
if (in_rw)
sd->flags |= VB2_SD_FLAG_ECSYNC_EC_IN_RW;
@@ -241,8 +175,6 @@
return VB2_SUCCESS;
}
-#define RO_RETRIES 2 /* Maximum times to retry flashing RO */
-
/**
* Sync, jump, and protect EC device
*
@@ -252,7 +184,6 @@
static vb2_error_t sync_ec(struct vb2_context *ctx)
{
struct vb2_shared_data *sd = vb2_get_sd(ctx);
- vb2_error_t rv;
const enum vb2_firmware_selection select_rw = EC_EFS ?
VB_SELECT_FIRMWARE_EC_UPDATE :
@@ -261,11 +192,7 @@
/* Update the RW Image */
if (sd->flags & SYNC_FLAG(select_rw)) {
- rv = update_ec(ctx, select_rw);
- if (reboot_requested(rv))
- return rv;
- else if (rv)
- return VB2_REQUEST_REBOOT_EC_TO_RO;
+ VB2_TRY(update_ec(ctx, select_rw), ctx, VB2_RECOVERY_EC_UPDATE);
/* Updated successfully. Cold reboot to switch to the new RW. */
if (ctx->flags & VB2_CONTEXT_NO_BOOT) {
VB2_DEBUG("Rebooting to jump to new EC-RW\n");
@@ -288,22 +215,7 @@
/* Tell EC to jump to RW. It should already be in RW for EFS2. */
if (!(sd->flags & VB2_SD_FLAG_ECSYNC_EC_IN_RW)) {
VB2_DEBUG("jumping to EC-RW\n");
- rv = vb2ex_ec_jump_to_rw();
- if (rv != VB2_SUCCESS) {
- VB2_DEBUG("vb2ex_ec_jump_to_rw() returned %x\n", rv);
-
- /*
- * If a previous AP boot has called
- * vb2ex_ec_disable_jump(), we need to reboot the EC to
- * unlock the ability to jump to the RW firmware.
- *
- * All other errors trigger recovery mode.
- */
- if (rv != VB2_REQUEST_REBOOT_EC_TO_RO)
- request_recovery(ctx, VB2_RECOVERY_EC_JUMP_RW);
-
- return VB2_REQUEST_REBOOT_EC_TO_RO;
- }
+ VB2_TRY(vb2ex_ec_jump_to_rw(), ctx, VB2_RECOVERY_EC_JUMP_RW);
}
/* Might need to update EC-RO */
@@ -313,56 +225,22 @@
/* Reset RO Software Sync NV flag */
vb2_nv_set(ctx, VB2_NV_TRY_RO_SYNC, 0);
- /*
- * Get the current recovery request (if any). This gets
- * overwritten by a failed try. If a later try succeeds, we'll
- * need to restore this request (or the lack of a request), or
- * else we'll end up in recovery mode even though RO software
- * sync did eventually succeed.
- */
- uint32_t recovery_request =
- vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST);
-
- /* Update the RO Image. */
- int num_tries;
- for (num_tries = 0; num_tries < RO_RETRIES; num_tries++) {
- rv = update_ec(ctx, VB_SELECT_FIRMWARE_READONLY);
- if (rv == VB2_SUCCESS)
- break;
- if (reboot_requested(rv))
- return rv;
- }
- if (num_tries == RO_RETRIES) {
- /* Ran out of tries */
- return VB2_REQUEST_REBOOT_EC_TO_RO;
- } else if (num_tries) {
- /*
- * Update succeeded after a failure, so we've polluted
- * the recovery request. Restore it.
- */
- request_recovery(ctx, recovery_request);
- }
+ /* Update the RO Image */
+ VB2_TRY(update_ec(ctx, VB_SELECT_FIRMWARE_READONLY),
+ ctx, VB2_RECOVERY_EC_UPDATE);
}
/* Protect RO flash */
- rv = protect_ec(ctx, VB_SELECT_FIRMWARE_READONLY);
- if (rv != VB2_SUCCESS)
- return rv;
+ VB2_TRY(vb2ex_ec_protect(VB_SELECT_FIRMWARE_READONLY),
+ ctx, VB2_RECOVERY_EC_PROTECT);
/* Protect RW flash */
- rv = protect_ec(ctx, select_rw);
- if (rv != VB2_SUCCESS)
- return rv;
+ VB2_TRY(vb2ex_ec_protect(select_rw), ctx, VB2_RECOVERY_EC_PROTECT);
/* Disable further sysjumps */
- rv = vb2ex_ec_disable_jump();
- if (rv != VB2_SUCCESS) {
- VB2_DEBUG("vb2ex_ec_disable_jump() returned %#x\n", rv);
- request_recovery(ctx, VB2_RECOVERY_EC_SOFTWARE_SYNC);
- return VB2_REQUEST_REBOOT_EC_TO_RO;
- }
+ VB2_TRY(vb2ex_ec_disable_jump(), ctx, VB2_RECOVERY_EC_SOFTWARE_SYNC);
- return rv;
+ return VB2_SUCCESS;
}
/**
diff --git a/firmware/2lib/include/2api.h b/firmware/2lib/include/2api.h
index 4a6fe0d..5b34082 100644
--- a/firmware/2lib/include/2api.h
+++ b/firmware/2lib/include/2api.h
@@ -35,7 +35,8 @@
if (_vb2_try_rv != VB2_SUCCESS) { \
vb2ex_printf(__func__, \
"%s returned %#x\n", #expr, _vb2_try_rv); \
- if ((_vb2_try_ctx) && \
+ if (_vb2_try_rv >= VB2_REQUEST_END && \
+ (_vb2_try_ctx) && \
(_vb2_try_reason) != VB2_RECOVERY_NOT_REQUESTED) \
vb2api_fail(_vb2_try_ctx, _vb2_try_reason, \
_vb2_try_rv); \
@@ -44,7 +45,8 @@
} while (0)
/*
- * Evaluate an expression and return *from the caller* on failure.
+ * Evaluate an expression and return *from the caller* on failure or if an
+ * action (such as reboot) is requested.
*
* This macro supports two forms of usage:
* 1. VB2_TRY(expr)
diff --git a/tests/vb2_api_tests.c b/tests/vb2_api_tests.c
index a03a004..3a32d4b 100644
--- a/tests/vb2_api_tests.c
+++ b/tests/vb2_api_tests.c
@@ -320,6 +320,12 @@
VB2_RECOVERY_NOT_REQUESTED, " vb2api_fail no request");
reset_common_data(FOR_MISC);
+ call_vb2_try(VB2_REQUEST, VB2_RECOVERY_NOT_REQUESTED, 1);
+ TEST_EQ(vb2_try_returned, 1, "VB2_TRY(expr) request");
+ TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+ VB2_RECOVERY_NOT_REQUESTED, " vb2api_fail no request");
+
+ reset_common_data(FOR_MISC);
call_vb2_try(VB2_ERROR_MOCK, VB2_RECOVERY_NOT_REQUESTED, 1);
TEST_EQ(vb2_try_returned, 1, "VB2_TRY(expr) error");
TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
@@ -334,12 +340,20 @@
0, " vb2api_fail no subcode");
reset_common_data(FOR_MISC);
- call_vb2_try(456, 123, 0);
+ call_vb2_try(VB2_REQUEST, 123, 0);
+ TEST_EQ(vb2_try_returned, 1, "VB2_TRY(expr, ...) request");
+ TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
+ VB2_RECOVERY_NOT_REQUESTED, " vb2api_fail no request");
+ TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE),
+ 0, " vb2api_fail no subcode");
+
+ reset_common_data(FOR_MISC);
+ call_vb2_try(VB2_ERROR_MOCK, 123, 0);
TEST_EQ(vb2_try_returned, 1, "VB2_TRY(expr, ...) error");
TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_REQUEST),
123, " vb2api_fail request");
TEST_EQ(vb2_nv_get(ctx, VB2_NV_RECOVERY_SUBCODE),
- 456 & 0xff, " vb2api_fail subcode");
+ VB2_ERROR_MOCK & 0xff, " vb2api_fail subcode");
}
static void phase1_tests(void)
diff --git a/tests/vb2_ec_sync_tests.c b/tests/vb2_ec_sync_tests.c
index 51e3dfb..6c06caf 100644
--- a/tests/vb2_ec_sync_tests.c
+++ b/tests/vb2_ec_sync_tests.c
@@ -476,7 +476,7 @@
ResetMocks();
mock_ec_rw_hash[0]++;
update_retval = VB2_ERROR_MOCK;
- test_ssync(VB2_REQUEST_REBOOT_EC_TO_RO,
+ test_ssync(VB2_ERROR_MOCK,
VB2_RECOVERY_EC_UPDATE, "Update failed");
TEST_EQ(ec_ro_updated, 0, " ec rw updated");
TEST_EQ(ec_rw_updated, 0, " ec rw updated");
@@ -529,7 +529,7 @@
ResetMocks();
jump_retval = VB2_ERROR_MOCK;
- test_ssync(VB2_REQUEST_REBOOT_EC_TO_RO,
+ test_ssync(VB2_ERROR_MOCK,
VB2_RECOVERY_EC_JUMP_RW, "Jump to RW fail");
TEST_EQ(ec_ro_updated, 0, " ec ro updated");
TEST_EQ(ec_rw_updated, 0, " ec rw updated");
