EFS: Switch active slot when current slot is invalid

When EFS finds the active slot is invalid, it tries the other slot.
This patch makes the other slot active so that the following boots
will try the other slot first.

This patch also replaces enum flash_rw_slot with system_image_copy_t.
The new APIs are therefore renamed from *_slot to *_copy. Basically,
this makes vboot see slots as a conceptual place instead of physical
spaces bound to flash storage.

BUG=b:65028930
BRANCH=none
TEST=On Fizz, verify:
1. RW_B is old and updated by soft sync. RW_B is activated and
   executed after reboot. System continues to boot to OS.
2. RW_A is old and updated by soft sync. RW_A is activated and
   executed after reboot. System continues to boot to OS.

Change-Id: Icf97da13e651e7a931b9d507052b9422566eb16c
Signed-off-by: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/648449

common/flash.c

@@ -1286,28 +1286,6 @@ static int flash_command_protect(struct host_cmd_handler_args *args)
 	return EC_RES_SUCCESS;
 }
 
-enum flash_rw_slot flash_get_active_slot(void)
-{
-	uint8_t slot;
-	if (system_get_bbram(SYSTEM_BBRAM_IDX_TRY_SLOT, &slot))
-		slot = FLASH_RW_SLOT_A;
-	return slot;
-}
-
-enum flash_rw_slot flash_get_update_slot(void)
-{
-#ifdef CONFIG_VBOOT_EFS
-	return 1 - flash_get_active_slot();
-#else
-	return FLASH_RW_SLOT_A;
-#endif
-}
-
-enum system_image_copy_t flash_slot_to_image(enum flash_rw_slot slot)
-{
-	return slot == FLASH_RW_SLOT_A ? SYSTEM_IMAGE_RW_A : SYSTEM_IMAGE_RW_B;
-}
-
 /*
  * TODO(crbug.com/239197) : Adding both versions to the version mask is a
  * temporary workaround for a problem in the cros_ec driver. Drop
@@ -1330,7 +1308,7 @@ static int flash_command_region_info(struct host_cmd_handler_args *args)
 		r->size = CONFIG_RO_SIZE;
 		break;
 	case EC_FLASH_REGION_ACTIVE:
-		r->offset = flash_get_rw_offset(flash_get_active_slot()) -
+		r->offset = flash_get_rw_offset(system_get_active_copy()) -
 				EC_FLASH_REGION_START;
 		r->size = CONFIG_RW_SIZE;
 		break;
@@ -1340,7 +1318,7 @@ static int flash_command_region_info(struct host_cmd_handler_args *args)
 		r->size = CONFIG_WP_STORAGE_SIZE;
 		break;
 	case EC_FLASH_REGION_UPDATE:
-		r->offset = flash_get_rw_offset(flash_get_update_slot()) -
+		r->offset = flash_get_rw_offset(system_get_update_copy()) -
 				EC_FLASH_REGION_START;
 		r->size = CONFIG_RW_SIZE;
 		break;

common/system.c

@@ -412,30 +412,24 @@ test_mockable int system_unsafe_to_overwrite(uint32_t offset, uint32_t size)
 {
 	uint32_t r_offset;
 	uint32_t r_size;
+	enum system_image_copy_t copy = system_get_image_copy();
 
-	switch (system_get_image_copy()) {
+	switch (copy) {
 	case SYSTEM_IMAGE_RO:
-		r_offset = CONFIG_EC_PROTECTED_STORAGE_OFF +
-			   CONFIG_RO_STORAGE_OFF;
 		r_size = CONFIG_RO_SIZE;
 		break;
 	case SYSTEM_IMAGE_RW:
-		r_offset = flash_get_rw_offset(FLASH_RW_SLOT_A);
+	case SYSTEM_IMAGE_RW_B:
 		r_size = CONFIG_RW_SIZE;
 #ifdef CONFIG_RWSIG
 		/* Allow RW sig to be overwritten */
 		r_size -= CONFIG_RW_SIG_SIZE;
 #endif
 		break;
-#ifdef CONFIG_VBOOT_EFS
-	case SYSTEM_IMAGE_RW_B:
-		r_offset = flash_get_rw_offset(FLASH_RW_SLOT_B);
-		r_size = CONFIG_RW_SIZE - CONFIG_RW_SIG_SIZE;
-		break;
-#endif
 	default:
 		return 0;
 	}
+	r_offset = flash_get_rw_offset(copy);
 
 	if ((offset >= r_offset && offset < (r_offset + r_size)) ||
 	    (r_offset >= offset && r_offset < (offset + size)))
@@ -619,17 +613,45 @@ int system_run_image_copy(enum system_image_copy_t copy)
 	return EC_ERROR_UNKNOWN;
 }
 
+enum system_image_copy_t system_get_active_copy(void)
+{
+	uint8_t slot;
+	if (system_get_bbram(SYSTEM_BBRAM_IDX_TRY_SLOT, &slot))
+		slot = SYSTEM_IMAGE_RW_A;
+	/* This makes it return RW_A by default. For example, this happens when
+	 * BBRAM isn't initialized. */
+	return slot == SYSTEM_IMAGE_RW_B ? slot : SYSTEM_IMAGE_RW_A;
+}
+
+enum system_image_copy_t system_get_update_copy(void)
+{
+#ifdef CONFIG_VBOOT_EFS
+	return system_get_active_copy() == SYSTEM_IMAGE_RW_A ?
+			SYSTEM_IMAGE_RW_B : SYSTEM_IMAGE_RW_A;
+#else
+	return SYSTEM_IMAGE_RW_A;
+#endif
+}
+
+int system_set_active_copy(enum system_image_copy_t copy)
+{
+	return system_set_bbram(SYSTEM_BBRAM_IDX_TRY_SLOT, copy);
+}
+
 /*
  * This is defined in system.c instead of flash.c because it's called even
  * on the boards which don't include flash.o. (e.g. hadoken, stm32l476g-eval)
  */
-uint32_t flash_get_rw_offset(enum flash_rw_slot slot)
+uint32_t flash_get_rw_offset(enum system_image_copy_t copy)
 {
 #ifdef CONFIG_VBOOT_EFS
-	if (slot == FLASH_RW_SLOT_B)
+	if (copy == SYSTEM_IMAGE_RW_B)
 		return CONFIG_EC_WRITABLE_STORAGE_OFF + CONFIG_RW_B_STORAGE_OFF;
 #endif
-	return CONFIG_EC_WRITABLE_STORAGE_OFF + CONFIG_RW_STORAGE_OFF;
+	if (is_rw_image(copy))
+		return CONFIG_EC_WRITABLE_STORAGE_OFF + CONFIG_RW_STORAGE_OFF;
+
+	return CONFIG_EC_PROTECTED_STORAGE_OFF + CONFIG_RO_STORAGE_OFF;
 }
 
 static const struct image_data *system_get_image_data(
@@ -657,17 +679,7 @@ static const struct image_data *system_get_image_data(
 	 * Read the version information from the proper location
 	 * on storage.
 	 */
-	switch (copy) {
-	case SYSTEM_IMAGE_RW:
-		addr += flash_get_rw_offset(FLASH_RW_SLOT_A);
-		break;
-	case SYSTEM_IMAGE_RW_B:
-		addr += flash_get_rw_offset(FLASH_RW_SLOT_B);
-		break;
-	default:
-		addr += CONFIG_EC_PROTECTED_STORAGE_OFF + CONFIG_RO_STORAGE_OFF;
-		break;
-	}
+	addr += flash_get_rw_offset(copy);
 
 #ifdef CONFIG_MAPPED_STORAGE
 	addr += CONFIG_MAPPED_STORAGE_BASE;
@@ -1224,12 +1236,12 @@ DECLARE_CONSOLE_COMMAND(sysrq, command_sysrq,
 static int host_command_get_version(struct host_cmd_handler_args *args)
 {
 	struct ec_response_get_version *r = args->response;
-	enum flash_rw_slot active_slot = flash_get_active_slot();
+	enum system_image_copy_t active_slot = system_get_active_copy();
 
 	strzcpy(r->version_string_ro, system_get_version(SYSTEM_IMAGE_RO),
 		sizeof(r->version_string_ro));
 	strzcpy(r->version_string_rw,
-		system_get_version(flash_slot_to_image(active_slot)),
+		system_get_version(active_slot),
 		sizeof(r->version_string_rw));
 
 	switch (system_get_image_copy()) {

common/vboot/vboot.c

@@ -25,11 +25,6 @@
 #define CPRINTS(format, args...) cprints(CC_VBOOT,"VB " format, ## args)
 #define CPRINTF(format, args...) cprintf(CC_VBOOT,"VB " format, ## args)
 
-enum vboot_ec_slot {
-	VBOOT_EC_SLOT_A,
-	VBOOT_EC_SLOT_B,
-};
-
 static int has_matrix_keyboard(void)
 {
 	return 0;
@@ -49,7 +44,7 @@ static int is_low_power_ap_boot_supported(void)
 	return 0;
 }
 
-static int verify_slot(int slot)
+static int verify_slot(enum system_image_copy_t slot)
 {
 	const struct vb21_packed_key *vb21_key;
 	const struct vb21_signature *vb21_sig;
@@ -59,7 +54,7 @@ static int verify_slot(int slot)
 	int len;
 	int rv;
 
-	CPRINTS("Verifying RW_%c", slot ? 'B' : 'A');
+	CPRINTS("Verifying %s", system_image_copy_t_to_string(slot));
 
 	vb21_key = (const struct vb21_packed_key *)(
 			CONFIG_MAPPED_STORAGE_BASE +
@@ -73,7 +68,7 @@ static int verify_slot(int slot)
 	key = (const struct rsa_public_key *)
 		((const uint8_t *)vb21_key + vb21_key->key_offset);
 
-	if (slot == VBOOT_EC_SLOT_A) {
+	if (slot == SYSTEM_IMAGE_RW_A) {
 		data = (const uint8_t *)(CONFIG_MAPPED_STORAGE_BASE +
 				CONFIG_EC_WRITABLE_STORAGE_OFF +
 				CONFIG_RW_A_STORAGE_OFF);
@@ -111,39 +106,41 @@ static int verify_slot(int slot)
 		return EC_ERROR_INVAL;
 	}
 
-	CPRINTS("Verified RW_%c", slot ? 'B' : 'A');
+	CPRINTS("Verified %s", system_image_copy_t_to_string(slot));
 
 	return EC_SUCCESS;
 }
 
 static int verify_and_jump(void)
 {
-	uint8_t slot;
+	enum system_image_copy_t slot;
 	int rv;
 
 	/* 1. Decide which slot to try */
-	if (system_get_bbram(SYSTEM_BBRAM_IDX_TRY_SLOT, &slot)) {
-		CPRINTS("Failed to read try slot");
-		slot = VBOOT_EC_SLOT_A;
-	}
+	slot = system_get_active_copy();
 
 	/* 2. Verify the slot */
 	rv = verify_slot(slot);
 	if (rv) {
-		if (rv != EC_ERROR_INVAL)
-			/* Unknown error. The other slot isn't worth trying. */
+		if (rv == EC_ERROR_VBOOT_KEY)
+			/* Key error. The other slot isn't worth trying. */
 			return rv;
-		/* Verification error. The other slot is worth trying. */
-		slot = 1 - slot;
-		if (verify_slot(slot))
+		slot = system_get_update_copy();
+		/* TODO(chromium:767050): Skip reading key again. */
+		rv = verify_slot(slot);
+		if (rv)
 			/* Both slots failed */
 			return rv;
-		/* Proceed with the other slot. AP will help us fix it. */
+
+		/* Proceed with the other slot. If this slot isn't expected, AP
+		 * will catch it and request recovery after a few attempts. */
+		if (system_set_active_copy(slot))
+			CPRINTS("Failed to activate %s",
+				system_image_copy_t_to_string(slot));
 	}
 
 	/* 3. Jump (and reboot) */
-	rv = system_run_image_copy(slot == VBOOT_EC_SLOT_A ?
-			SYSTEM_IMAGE_RW : SYSTEM_IMAGE_RW_B);
+	rv = system_run_image_copy(slot);
 	CPRINTS("Failed to jump (%d)", rv);
 
 	return rv;
@@ -152,13 +149,13 @@ static int verify_and_jump(void)
 /* Request more power: charging battery or more powerful AC adapter */
 static void request_power(void)
 {
-	/* TODO: Blink LED */
+	/* TODO(crosbug.com/p/37646390): Blink LED */
 	CPRINTS("%s", __func__);
 }
 
 static void request_recovery(void)
 {
-	/* TODO: Blink LED */
+	/* TODO(crosbug.com/p/37646390): Blink LED */
 	CPRINTS("%s", __func__);
 }
 
@@ -226,7 +223,6 @@ static void vboot_main(void)
 		 * keyboard */
 		CPRINTS("Enable C%d PD comm", port);
 		pd_comm_enable(port, 1);
-		/* TODO: Inform PD task and make it negotiate */
 		return;
 	}
 

common/vboot_hash.c

@@ -261,7 +261,7 @@ static void vboot_hash_init(void)
 #endif
 	{
 		/* Start computing the hash of RW firmware */
-		vboot_hash_start(flash_get_rw_offset(flash_get_active_slot()),
+		vboot_hash_start(flash_get_rw_offset(system_get_active_copy()),
 				 get_rw_size(), NULL, 0);
 	}
 }
@@ -298,9 +298,9 @@ static int get_offset(int offset)
 	if (offset == EC_VBOOT_HASH_OFFSET_RO)
 		return CONFIG_EC_PROTECTED_STORAGE_OFF + CONFIG_RO_STORAGE_OFF;
 	if (offset == EC_VBOOT_HASH_OFFSET_ACTIVE)
-		return flash_get_rw_offset(flash_get_active_slot());
+		return flash_get_rw_offset(system_get_active_copy());
 	if (offset == EC_VBOOT_HASH_OFFSET_UPDATE)
-		return flash_get_rw_offset(flash_get_update_slot());
+		return flash_get_rw_offset(system_get_update_copy());
 	return offset;
 }
 

include/flash.h

@@ -102,31 +102,6 @@ enum flash_region {
 	FLASH_REGION_COUNT
 };
 
-/**
- * Get active slot
- *
- * Active slot contains an image which is being executed or will be executed
- * after sysjump.
- *
- * @return Active slot index
- */
-enum flash_rw_slot flash_get_active_slot(void);
-
-/**
- * Get updatable (non-active) slot
- *
- * @return Updatable slot index
- */
-enum flash_rw_slot flash_get_update_slot(void);
-
-/**
- * Translate slot index to image index
- *
- * @param slot Slot index to convert.
- * @return     Image index converted from <slot>
- */
-enum system_image_copy_t flash_slot_to_image(enum flash_rw_slot slot);
-
 /*****************************************************************************/
 /* Low-level methods, for use by flash_common. */
 

include/system.h

@@ -562,18 +562,37 @@ static inline void system_print_extended_version_info(void)
  */
 int system_can_boot_ap(void);
 
-enum flash_rw_slot {
-	/* Must be 0 and 1 because they are converted by 1 - slot_x. */
-	FLASH_RW_SLOT_A = 0,
-	FLASH_RW_SLOT_B = 1,
-};
+/**
+ * Get active image copy
+ *
+ * Active slot contains an image which is being executed or will be executed
+ * after sysjump.
+ *
+ * @return Active copy index
+ */
+enum system_image_copy_t system_get_active_copy(void);
 
 /**
- * Get flash offset of a RW slot
+ * Get updatable (non-active) image copy
  *
- * @param slot Slot index to get the flash offset of.
- * @return     Flash offset of the slot specified by <slot>
+ * @return Updatable copy index
  */
-uint32_t flash_get_rw_offset(enum flash_rw_slot slot);
+enum system_image_copy_t system_get_update_copy(void);
+
+/**
+ * Set active image copy
+ *
+ * @param copy Copy id to be activated.
+ * @return     Non-zero if error.
+ */
+int system_set_active_copy(enum system_image_copy_t copy);
+
+/**
+ * Get flash offset of a RW copy
+ *
+ * @param copy Copy index to get the flash offset of.
+ * @return     Flash offset of the slot storing <copy>
+ */
+uint32_t flash_get_rw_offset(enum system_image_copy_t copy);
 
 #endif  /* __CROS_EC_SYSTEM_H */