nvmem: do not use malloc for cached buffer

With introduction of encryption it is becoming impossible to read
NVMEM contents directly from flash. Decrypting the contents each time
there is a read request creates a significant performance hit. NVMEM
needs to be rearchitecture such that there is no need to run
decryption each time NVMEM read is performed.

This patch does just that, implementation details are described in the
header comment in common/nvmem.c.

To reduce memory impact the size of NVMEM is being decreased from 16K
to 12K. This is acceptable because eviction objects stored in NVMEM
serialized now, which dramatically reduces NVMEM size requirements.
The TPM2 NVMEM size definition must be kept in sync.

Another optimization this change introduces is bypassing writing into
the flash if NVMEM contents did not change, which is verified by
examining the hash of the cached storage.

A test is added to verify that the new commit scheme works as
expected, and the nvmem test is re-introduced to the list of test ran
on each 'make buildall'.

CQ-DEPEND=CL:433839
BRANCH=none
BUG=chrome-os-partner:62260,chrome-os-partner:62421
BUG=chrome-os-partner:62437
TEST=ran the following tests, all succeeded
     make buildall -j
     TEST_LIST_HOST=nvmem make runtests
     tcg test suite
     corp enroll on reef, reboot a few times, verify that enrollment sticks

Change-Id: I177daa3ceb4fd7aac299ca26b4506b863e31b946
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/433184
Reviewed-by: Aaron Durbin <adurbin@chromium.org>

7 files changed, 292 insertions, 280 deletions

diff --git a/chip/g/config_chip.h b/chip/g/config_chip.h
index 531883c504..838f38f812 100644
--- a/chip/g/config_chip.h
+++ b/chip/g/config_chip.h
@@ -95,7 +95,7 @@
  * use these two areas for the same thing, it's just more convenient to make
  * them the same size.
  */
-#define CFG_TOP_SIZE  0x4000
+#define CFG_TOP_SIZE  0x3000
 #define CFG_TOP_A_OFF (CFG_FLASH_HALF - CFG_TOP_SIZE)
 #define CFG_TOP_B_OFF (CONFIG_FLASH_SIZE - CFG_TOP_SIZE)
 
diff --git a/common/nvmem.c b/common/nvmem.c
index 95a2281c8b..561fa5e537 100644
--- a/common/nvmem.c
+++ b/common/nvmem.c
@@ -7,7 +7,6 @@
 #include "console.h"
 #include "flash.h"
 #include "nvmem.h"
-#include "shared_mem.h"
 #include "task.h"
 #include "timer.h"
 #include "util.h"
@@ -15,10 +14,50 @@
 #define CPRINTF(format, args...) cprintf(CC_COMMAND, format, ## args)
 #define CPRINTS(format, args...) cprints(CC_COMMAND, format, ## args)
 
-#define NVMEM_ACQUIRE_CACHE_SLEEP_MS 25
-#define NVMEM_ACQUIRE_CACHE_MAX_ATTEMPTS (250 / NVMEM_ACQUIRE_CACHE_SLEEP_MS)
 #define NVMEM_NOT_INITIALIZED (-1)
 
+/*
+ * The NVMEM contents are stored in flash memory. At run time there is an SRAM
+ * cache and two instances of the contents in the flash in two partitions.
+ *
+ * Each instance is protected by a 16 bytes hash and has a 'generation' value
+ * associated with it. When NVMEM module is initialized it checks the flash
+ * stored instances. If both of them are valid, it considers the newer one
+ * (younger generation) to be the proper NVMEM contents and copies it to the
+ * SRAM cache. If only one instance is valid, it is used, and if no instances
+ * are valid - a new valid partition is created and copied into the SRAM
+ * cache.
+ *
+ * When stored in flash, the contents are encrypted, the hash value is used as
+ * the IV for the encryption routine.
+ *
+ * There is a mutex controlling access to the NVMEM. There are two levels
+ * of protection - for read only accesses and for write accesses. When the
+ * module is initialized the mutex is opened.
+ *
+ * If there are no pending writes, each read access locks the mutex, reads out
+ * the data and unlocks the mutex, thus multiple tasks could be reading NVMEM,
+ * blocking access momentarily.
+ *
+ * If a write access ever occurs things get more complicated. The write access
+ * leaves the mutex locked and stores the flag, indicating that the
+ * contents have changed and need to be saved, and stores the task id of the
+ * task performing the write access.
+ *
+ * The mutex remains locked in this case. Next time a read access happens,
+ * if it comes from the same task, the unlock in the end of the read is
+ * bypassed because the 'write in progress' flag is set. If a read or write
+ * request comes from another task, they  will be blocked until the first
+ * task to write commits.
+ *
+ * nvmem_commit() calls the nvmem_save() function which checks if the cache
+ * contents indeed changed (by calculating the hash again). If there is no
+ * change - the mutex is released and the function exits. If there is a
+ * change, the new generation value is set, the new hash is calculated
+ * and the copy is saved in the least recently used flash partition, and
+ * then the lock is released.
+ */
+
 /* Table of start addresses for each partition */
 static const uintptr_t nvmem_base_addr[NVMEM_NUM_PARTITIONS] = {
 		CONFIG_FLASH_NVMEM_BASE_A,
@@ -32,22 +71,24 @@ static uint32_t nvmem_user_start_offset[NVMEM_NUM_USERS];
 static int nvmem_act_partition;
 
 /* NvMem cache memory structure */
-struct nvmem_cache {
-	uint8_t *base_ptr;
+struct nvmem_mutex_ {
 	task_id_t task;
+	int write_in_progress;
 	struct mutex mtx;
 };
 
-struct nvmem_cache cache;
+static struct nvmem_mutex_ nvmem_mutex;
+static uint8_t nvmem_cache[NVMEM_PARTITION_SIZE] __aligned(4);
 
 static uint8_t commits_enabled;
-static uint8_t commits_skipped;
 
 /* NvMem error state */
 static int nvmem_error_state;
 /* Flag to track if an Nv write/move is not completed */
 static int nvmem_write_error;
 
+static void nvmem_release_cache(void);
+
 /*
  * Given the nvmem tag address calculate the sha value of the nvmem buffer and
  * save it in the provided space. The caller is expected to provide enough
@@ -59,63 +100,85 @@ static void nvmem_compute_sha(struct nvmem_tag *tag, void *sha_buf)
 			 sha_buf, sizeof(tag->sha));
 }
 
-static int nvmem_save(uint8_t tag_generation)
+static int nvmem_save(void)
 {
-	struct nvmem_tag *tag;
+	struct nvmem_partition *part;
 	size_t nvmem_offset;
-	int dest_partition = (nvmem_act_partition + 1) % NVMEM_NUM_PARTITIONS;
+	int dest_partition;
+	uint8_t sha_comp[NVMEM_SHA_SIZE];
+	int rv = EC_SUCCESS;
+
+	part = (struct nvmem_partition *)nvmem_cache;
+
+	/* Has anything changed in the cache? */
+	nvmem_compute_sha(&part->tag, sha_comp);
 
-	/* Flash offset of the partition to save. */
+	if (!memcmp(part->tag.sha, sha_comp, sizeof(part->tag.sha))) {
+		CPRINTF("%s: Nothing changed, skipping flash write\n",
+			__func__);
+		goto release_cache;
+	}
+
+	/* Get flash offset of the partition to save to. */
+	dest_partition = (nvmem_act_partition + 1) % NVMEM_NUM_PARTITIONS;
 	nvmem_offset = nvmem_base_addr[dest_partition] -
 		CONFIG_PROGRAM_MEMORY_BASE;
 
 	/* Erase partition */
-	if (flash_physical_erase(nvmem_offset,
-				 NVMEM_PARTITION_SIZE)) {
-		CPRINTF("%s:%d\n", __func__, __LINE__);
-		return EC_ERROR_UNKNOWN;
+	rv = flash_physical_erase(nvmem_offset, NVMEM_PARTITION_SIZE);
+	if (rv != EC_SUCCESS) {
+		CPRINTF("%s flash erase failed\n", __func__);
+		goto release_cache;
 	}
 
-	tag = (struct nvmem_tag *)cache.base_ptr;
-	tag->generation = tag_generation;
-	tag->layout_version = NVMEM_LAYOUT_VERSION;
+	part->tag.layout_version = NVMEM_LAYOUT_VERSION;
+	part->tag.generation++;
 
 	/* Calculate sha of the whole thing. */
-	nvmem_compute_sha(tag, tag->sha);
+	nvmem_compute_sha(&part->tag, part->tag.sha);
 
 	/* Encrypt actual payload. */
-	if (!app_cipher(tag->sha, tag + 1, tag + 1,
-			NVMEM_PARTITION_SIZE - sizeof(struct nvmem_tag))) {
-		CPRINTF("%s:%d\n", __func__, __LINE__);
-		return EC_ERROR_UNKNOWN;
+	if (!app_cipher(part->tag.sha, part->buffer, part->buffer,
+			sizeof(part->buffer))) {
+		CPRINTF("%s encryption failed\n", __func__);
+		rv = EC_ERROR_UNKNOWN;
+		goto release_cache;
 	}
 
-	/* Write partition */
-	if (flash_physical_write(nvmem_offset,
-				 NVMEM_PARTITION_SIZE,
-				 cache.base_ptr)) {
-		CPRINTF("%s:%d\n", __func__, __LINE__);
-		return EC_ERROR_UNKNOWN;
+	rv = flash_physical_write(nvmem_offset,
+				  NVMEM_PARTITION_SIZE,
+				  nvmem_cache);
+	if (rv != EC_SUCCESS) {
+		CPRINTF("%s flash write failed\n", __func__);
+		goto release_cache;
+	}
+
+	/* Restore payload. */
+	if (!app_cipher(part->tag.sha, part->buffer, part->buffer,
+			sizeof(part->buffer))) {
+		CPRINTF("%s decryption failed\n", __func__);
+		rv = EC_ERROR_UNKNOWN;
+		goto release_cache;
 	}
 
 	nvmem_act_partition = dest_partition;
-	return EC_SUCCESS;
+
+ release_cache:
+	nvmem_mutex.write_in_progress = 0;
+	nvmem_release_cache();
+	return rv;
 }
 
 /*
  * Read from flash and verify partition.
  *
  * @param index - index of the partition to verify
- * @param part_buffer - if non-null, a pointer where the caller wants to save
- *                      the address of the verified partition in SRAM. If
- *                      verification succeeded, the caller is responsible for
- *                      releasing the allocated memory.
  *
  * Returns EC_SUCCESS on verification success
  *         EC_ERROR_BUSY in case of malloc failure
  *         EC_ERROR_UNKNOWN on failure to decrypt of verify.
  */
-static int nvmem_partition_read_verify(int index, void **part_buffer)
+static int nvmem_partition_read_verify(int index)
 {
 	uint8_t sha_comp[NVMEM_SHA_SIZE];
 	struct nvmem_partition *p_part;
@@ -123,13 +186,7 @@ static int nvmem_partition_read_verify(int index, void **part_buffer)
 	int ret;
 
 	p_part = (struct nvmem_partition *)nvmem_base_addr[index];
-
-	/* First copy it into ram. */
-	ret = shared_mem_acquire(NVMEM_PARTITION_SIZE, (char **)&p_copy);
-	if (ret != EC_SUCCESS) {
-		CPRINTF("%s failed to malloc!\n", __func__);
-		return ret;
-	}
+	p_copy = (struct nvmem_partition *)nvmem_cache;
 	memcpy(p_copy, p_part, NVMEM_PARTITION_SIZE);
 
 	/* Then decrypt it. */
@@ -137,7 +194,6 @@ static int nvmem_partition_read_verify(int index, void **part_buffer)
 			&p_copy->tag + 1,
 			NVMEM_PARTITION_SIZE - sizeof(struct nvmem_tag))) {
 		CPRINTF("%s: decryption failure\n", __func__);
-		shared_mem_release(p_copy);
 		return EC_ERROR_UNKNOWN;
 	}
 
@@ -148,113 +204,53 @@ static int nvmem_partition_read_verify(int index, void **part_buffer)
 	nvmem_compute_sha(&p_copy->tag, sha_comp);
 	ret = !memcmp(p_copy->tag.sha, sha_comp, NVMEM_SHA_SIZE);
 
-	if (ret && part_buffer)
-		*part_buffer = p_copy;
-
-	if (!ret || !part_buffer)
-		shared_mem_release(p_copy);
-
 	return ret ? EC_SUCCESS : EC_ERROR_UNKNOWN;
 }
 
-/* Called with the semaphore locked. */
-static int nvmem_acquire_cache(void)
-{
-	int attempts = 0;
-
-	cache.base_ptr = NULL;  /* Just in case. */
-
-	while (attempts < NVMEM_ACQUIRE_CACHE_MAX_ATTEMPTS) {
-		int ret;
-
-		ret = nvmem_partition_read_verify(nvmem_act_partition,
-						  (void **)&cache.base_ptr);
-
-		if (ret != EC_ERROR_BUSY)
-			return ret;
-
-		CPRINTF("Shared Mem not available on attempt %d!\n", attempts);
-		/* TODO: what time really makes sense? */
-		msleep(NVMEM_ACQUIRE_CACHE_SLEEP_MS);
-		attempts++;
-	}
-	/* Timeout Error condition */
-	CPRINTF("%s:%d\n", __func__, __LINE__);
-	return EC_ERROR_TIMEOUT;
-}
-
-static int nvmem_lock_cache(void)
+static void nvmem_lock_cache(void)
 {
 	/*
-	 * Need to protect the cache contents and pointer value from other tasks
-	 * attempting to do nvmem write operations. However, since this function
-	 * may be called mutliple times prior to the mutex lock being released,
-	 * there is a check first to see if the current task holds the lock. If
-	 * it does then the task number will equal the value in cache.task. If
-	 * the lock is held by a different task then mutex_lock function will
-	 * operate as normal.
+	 * Need to protect the cache contents value from other tasks
+	 * attempting to do nvmem write operations. However, since this
+	 * function may be called mutliple times prior to the mutex lock being
+	 * released, there is a check first to see if the current task holds
+	 * the lock. If it does then the task number will equal the value in
+	 * cache.task, no need to wait.
+	 *
+	 * If the lock is held by a different task then mutex_lock function
+	 * will operate as normal.
 	 */
-	if (cache.task != task_get_current()) {
-		mutex_lock(&cache.mtx);
-		cache.task = task_get_current();
-	} else
-		/* Lock is held by current task, nothing else to do. */
-		return EC_SUCCESS;
-
-	/*
-	 * Acquire the shared memory buffer and copy the full
-	 * partition size from flash into the cache buffer.
-	 */
-	if (nvmem_acquire_cache() != EC_SUCCESS) {
-		/* Shared memory not available, need to release lock */
-		/* Set task number to value that can't match a valid task */
-		cache.task = TASK_ID_COUNT;
-		/* Release lock */
-		mutex_unlock(&cache.mtx);
-		return EC_ERROR_TIMEOUT;
-	}
+	if (nvmem_mutex.task == task_get_current())
+		return;
 
-	return EC_SUCCESS;
+	mutex_lock(&nvmem_mutex.mtx);
+	nvmem_mutex.task = task_get_current();
 }
 
 static void nvmem_release_cache(void)
 {
-	/* Done with shared memory buffer, release it. */
-	shared_mem_release(cache.base_ptr);
-	/* Inidicate cache is not available */
-	cache.base_ptr = NULL;
+	if (nvmem_mutex.write_in_progress)
+		return;		/* It will have to be saved first. */
+
 	/* Reset task number to max value */
-	cache.task = TASK_ID_COUNT;
+	nvmem_mutex.task = TASK_ID_COUNT;
 	/* Release mutex lock here */
-	mutex_unlock(&cache.mtx);
+	mutex_unlock(&nvmem_mutex.mtx);
 }
 
 static int nvmem_reinitialize(void)
 {
-	int ret;
-
+	nvmem_lock_cache();  /* Unlocked by nvmem_save() below. */
 	/*
 	 * NvMem is not properly initialized. Let's just erase everything and
 	 * start over, so that at least 1 partition is ready to be used.
 	 */
 	nvmem_act_partition = 0;
 
-	/* Need to acquire the shared memory buffer */
-	ret = shared_mem_acquire(NVMEM_PARTITION_SIZE,
-				 (char **)&cache.base_ptr);
-
-	if (ret != EC_SUCCESS)
-		return ret;
-
-	memset(cache.base_ptr, 0xff, NVMEM_PARTITION_SIZE);
+	memset(nvmem_cache, 0xff, NVMEM_PARTITION_SIZE);
 
 	/* Start with generation zero in the current active partition. */
-	ret = nvmem_save(0);
-	shared_mem_release(cache.base_ptr);
-	cache.base_ptr = 0;
-	if (ret != EC_SUCCESS)
-		CPRINTF("%s:%d\n", __func__, __LINE__);
-	return ret;
+	return nvmem_save();
 }
 
 static int nvmem_compare_generation(void)
@@ -291,7 +287,7 @@ static int nvmem_find_partition(void)
 	 * select the most recent one.
 	 */
 	for (n = 0; n < NVMEM_NUM_PARTITIONS; n++)
-		if (nvmem_partition_read_verify(n, NULL) == EC_SUCCESS) {
+		if (nvmem_partition_read_verify(n) == EC_SUCCESS) {
 			if (nvmem_act_partition == NVMEM_NOT_INITIALIZED)
 				nvmem_act_partition = n;
 			else
@@ -363,7 +359,7 @@ static int nvmem_get_partition_off(int user, uint32_t offset,
 	return EC_SUCCESS;
 }
 
-int nvmem_setup(uint8_t starting_generation)
+int nvmem_setup(void)
 {
 	int part;
 	int ret;
@@ -373,27 +369,28 @@ int nvmem_setup(uint8_t starting_generation)
 	part = nvmem_act_partition;
 	nvmem_act_partition = 0;
 
-	/* Get the cache buffer */
-	if (nvmem_lock_cache() != EC_SUCCESS) {
-		CPRINTF("%s: Cache ram not available!\n", __func__);
-		nvmem_act_partition = part;
-		return EC_ERROR_TIMEOUT;
-	}
-
 	ret = EC_SUCCESS;
 
 	for (part = 0; part < NVMEM_NUM_PARTITIONS; part++) {
 		int rv;
 
-		memset(cache.base_ptr, 0xff, NVMEM_PARTITION_SIZE);
-		rv = nvmem_save(starting_generation + part);
+		memset(nvmem_cache, 0xff, NVMEM_PARTITION_SIZE);
+		/*
+		 * Make sure the contents change between runs of
+		 * nvmem_save() so that both flash partitions are
+		 * written with empty contents and different
+		 * generation numbers.
+		 */
+		((struct nvmem_partition *)nvmem_cache)->tag.generation = part;
+		/* Lock the cache buffer (unlocked by nvmem_save() */
+		nvmem_lock_cache();
+		rv = nvmem_save();
 
 		/* Even if one partition saving failed, let's keep going. */
 		if (rv != EC_SUCCESS)
 			ret = rv;
 	}
 
-	nvmem_release_cache();
 	return ret;
 }
 
@@ -410,11 +407,9 @@ int nvmem_init(void)
 	/* Initialize error state, assume everything is good */
 	nvmem_error_state = EC_SUCCESS;
 	nvmem_write_error = 0;
-	/* Default state for cache base_ptr and task number */
-	cache.base_ptr = NULL;
-	cache.task = TASK_ID_COUNT;
 
 	ret = nvmem_find_partition();
+
 	if (ret != EC_SUCCESS) {
 		/* Change error state to non-zero */
 		nvmem_error_state = ret;
@@ -424,7 +419,6 @@ int nvmem_init(void)
 
 	CPRINTS("Active Nvmem partition set to %d", nvmem_act_partition);
 	commits_enabled = 1;
-	commits_skipped = 0;
 
 	return EC_SUCCESS;
 }
@@ -439,17 +433,8 @@ int nvmem_is_different(uint32_t offset, uint32_t size, void *data,
 {
 	int ret;
 	uint32_t src_offset;
-	int need_to_release;
-
-	/* Point to either NvMem flash or ram if that's active */
-	if (!cache.base_ptr) {
-		ret = nvmem_lock_cache();
-		if (ret != EC_SUCCESS)
-			return ret;
-		need_to_release = 1;
-	} else {
-		need_to_release = 0;
-	}
+
+	nvmem_lock_cache();
 
 	/* Get partition offset for this read operation */
 	ret = nvmem_get_partition_off(user, offset, size, &src_offset);
@@ -459,9 +444,9 @@ int nvmem_is_different(uint32_t offset, uint32_t size, void *data,
 	/* Advance to the correct byte within the data buffer */
 
 	/* Compare NvMem with data */
-	ret = memcmp(cache.base_ptr + src_offset, data, size);
-	if (need_to_release)
-		nvmem_release_cache();
+	ret = memcmp(nvmem_cache + src_offset, data, size);
+
+	nvmem_release_cache();
 
 	return ret;
 }
@@ -471,26 +456,17 @@ int nvmem_read(uint32_t offset, uint32_t size,
 {
 	int ret;
 	uint32_t src_offset;
-	int need_to_release;
-
-	if (!cache.base_ptr) {
-		ret = nvmem_lock_cache();
-		if (ret != EC_SUCCESS)
-			return ret;
-		need_to_release = 1;
-	} else {
-		need_to_release = 0;
-	}
+
+	nvmem_lock_cache();
 
 	/* Get partition offset for this read operation */
 	ret = nvmem_get_partition_off(user, offset, size, &src_offset);
 
 	if (ret == EC_SUCCESS)
 		/* Copy from src into the caller's destination buffer */
-		memcpy(data, cache.base_ptr + src_offset, size);
+		memcpy(data, nvmem_cache + src_offset, size);
 
-	if (commits_enabled && need_to_release)
-		nvmem_release_cache();
+	nvmem_release_cache();
 
 	return ret;
 }
@@ -500,15 +476,11 @@ int nvmem_write(uint32_t offset, uint32_t size,
 {
 	int ret;
 	uint8_t *p_dest;
-	uintptr_t dest_addr;
 	uint32_t dest_offset;
 
 	/* Make sure that the cache buffer is active */
-	ret = nvmem_lock_cache();
-	if (ret) {
-		nvmem_write_error = 1;
-		return ret;
-	}
+	nvmem_lock_cache();
+	nvmem_mutex.write_in_progress = 1;
 
 	/* Compute partition offset for this write operation */
 	ret = nvmem_get_partition_off(user, offset, size, &dest_offset);
@@ -518,9 +490,8 @@ int nvmem_write(uint32_t offset, uint32_t size,
 	}
 
 	/* Advance to correct offset within data buffer */
-	dest_addr = (uintptr_t)cache.base_ptr;
-	dest_addr += dest_offset;
-	p_dest = (uint8_t *)dest_addr;
+	p_dest = nvmem_cache + dest_offset;
+
 	/* Copy data from caller into destination buffer */
 	memcpy(p_dest, data, size);
 
@@ -536,11 +507,8 @@ int nvmem_move(uint32_t src_offset, uint32_t dest_offset, uint32_t size,
 	uint32_t s_buff_offset, d_buff_offset;
 
 	/* Make sure that the cache buffer is active */
-	ret = nvmem_lock_cache();
-	if (ret) {
-		nvmem_write_error = 1;
-		return ret;
-	}
+	nvmem_lock_cache();
+	nvmem_mutex.write_in_progress = 1;
 
 	/* Compute partition offset for source */
 	ret = nvmem_get_partition_off(user, src_offset, size, &s_buff_offset);
@@ -556,7 +524,7 @@ int nvmem_move(uint32_t src_offset, uint32_t dest_offset, uint32_t size,
 		return ret;
 	}
 
-	base_addr = (uintptr_t)cache.base_ptr;
+	base_addr = (uintptr_t)nvmem_cache;
 	/* Create pointer to src location within partition */
 	p_src = (uint8_t *)(base_addr + s_buff_offset);
 	/* Create pointer to dest location within partition */
@@ -567,70 +535,59 @@ int nvmem_move(uint32_t src_offset, uint32_t dest_offset, uint32_t size,
 	return EC_SUCCESS;
 }
 
-void nvmem_enable_commits(void)
+int nvmem_enable_commits(void)
 {
 	if (commits_enabled)
-		return;
+		return EC_SUCCESS;
 
-	commits_enabled = 1;
-	if (!commits_skipped)
-		return;
+	if (nvmem_mutex.task != task_get_current()) {
+		CPRINTF("%s: locked by task %d, attempt to unlock by task %d\n",
+			__func__, nvmem_mutex.task, task_get_current());
+		return EC_ERROR_INVAL;
+	}
 
+	commits_enabled = 1;
 	CPRINTS("Committing NVMEM changes.");
-	nvmem_commit();
-	commits_skipped = 0;
+	return nvmem_commit();
 }
 
 void nvmem_disable_commits(void)
 {
+	/* Will be unlocked when nvmem_enable_commits() is called. */
+	nvmem_lock_cache();
+
 	commits_enabled = 0;
-	commits_skipped = 0;
 }
 
 int nvmem_commit(void)
 {
-	uint16_t generation;
-	struct nvmem_partition *p_part;
+	if (nvmem_mutex.task == TASK_ID_COUNT) {
+		CPRINTF("%s: attempt to commit in unlocked state\n",
+			__func__, nvmem_mutex.task);
+		return EC_ERROR_OVERFLOW;  /* Noting to commit. */
+	}
 
-	if (!commits_enabled) {
-		commits_skipped = 1;
-		CPRINTS("Skipping commit");
-		return EC_SUCCESS;
+	if (nvmem_mutex.task != task_get_current()) {
+		CPRINTF("%s: locked by task %d, attempt to unlock by task %d\n",
+			__func__, nvmem_mutex.task, task_get_current());
+		return EC_ERROR_INVAL;
 	}
 
 	/* Ensure that all writes/moves prior to commit call succeeded */
 	if (nvmem_write_error) {
-		CPRINTS("NvMem: Write Error, commit abandoned");
+		CPRINTS("%s: Write Error, commit abandoned", __func__);
 		/* Clear error state */
 		nvmem_write_error = 0;
+		commits_enabled = 1;
 		nvmem_release_cache();
 		return EC_ERROR_UNKNOWN;
 	}
-	/*
-	 * All scratch buffer blocks must be written to physical flash
-	 * memory. In addition, the scratch block buffer index table
-	 * entries must be reset along with the index itself.
-	 */
 
-	/* Update generation number */
-	if (cache.base_ptr == NULL) {
-		CPRINTF("%s:%d\n", __func__, __LINE__);
-		return EC_ERROR_UNKNOWN;
+	if (!commits_enabled) {
+		CPRINTS("Skipping commit");
+		return EC_SUCCESS;
 	}
-	p_part = (struct nvmem_partition *)cache.base_ptr;
-	generation = p_part->tag.generation + 1;
-	/* Check for restricted generation number */
-	if (generation == NVMEM_GENERATION_MASK)
-		generation = 0;
 
 	/* Write active partition to NvMem */
-	if (nvmem_save(generation) != EC_SUCCESS) {
-		/* Free up scratch buffers */
-		nvmem_release_cache();
-		return EC_ERROR_UNKNOWN;
-	}
-
-	/* Free up scratch buffers */
-	nvmem_release_cache();
-	return EC_SUCCESS;
+	return nvmem_save();
 }
diff --git a/common/tpm_registers.c b/common/tpm_registers.c
index e60c577804..4141e43602 100644
--- a/common/tpm_registers.c
+++ b/common/tpm_registers.c
@@ -726,7 +726,7 @@ static void tpm_reset_now(int wipe_first)
 		assert_ec_rst();
 
 		/* Now wipe nvmem */
-		wipe_result = nvmem_setup(0);
+		wipe_result = nvmem_setup();
 	} else {
 		wipe_result = EC_SUCCESS;
 	}
diff --git a/include/nvmem.h b/include/nvmem.h
index 1f9a27b1c7..87de8b9cae 100644
--- a/include/nvmem.h
+++ b/include/nvmem.h
@@ -35,7 +35,7 @@
  *
  * Note that the NvMem partitions can be placed anywhere in flash space, but
  * must be equal in total size. A table is used by the NvMem module to get the
- * correct base address and offset for each partition.
+ * correct base address for each partition.
  *
  * A generation number is used to distinguish between two valid partitions with
  * the newsest generation number (in a circular sense) marking the correct
@@ -54,8 +54,8 @@
  * The board.h file must define a macro or enum named NVMEM_NUM_USERS.
  * The board.c file must implement:
  *    nvmem_user_sizes[] -> array of user buffer lengths
- *    nvmem_compute_sha() -> function used to compute 4 byte sha (or equivalent)
- *    nvmem_wipe() -> function to erase and reformat the users' storage
+ * The chip must provide
+ *    app_compute_hash() -> function used to compute 16 byte sha (or equivalent)
  *
  * Note that total length of user buffers must satisfy the following:
  *   sum(user sizes) <= (NVMEM_PARTITION_SIZE) - sizeof(struct nvmem_tag)
@@ -130,6 +130,9 @@ int nvmem_read(uint32_t startOffset, uint32_t size,
 /**
  * Write 'size' amount of bytes to NvMem
  *
+ * Calling this function will wait for the mutex, then lock it until
+ * nvmem_commit() is invoked.
+ *
  * @param startOffset: Offset (in bytes) into NVmem logical space
  * @param size: Number of bytes to write
  * @param data: Pointer to source buffer
@@ -144,6 +147,9 @@ int nvmem_write(uint32_t startOffset, uint32_t size,
 /**
  * Move 'size' amount of bytes within NvMem
  *
+ * Calling this function will wait for the mutex, then lock it until
+ * nvmem_commit() is invoked.
+ *
  * @param src_offset: source offset within NvMem logical space
  * @param dest_offset: destination offset within NvMem logical space
  * @param size: Number of bytes to move
@@ -158,7 +164,12 @@ int nvmem_move(uint32_t src_offset, uint32_t dest_offset, uint32_t size,
  * Commit all previous NvMem writes to flash
  *
  * @return EC_SUCCESS if flash erase/operations are successful.
- *         EC_ERROR_UNKNOWN otherwise.
+
+ *         EC_ERROR_OVERFLOW in case the mutex is not locked when this
+ *                           function is called
+ *         EC_ERROR_INVAL    if task trying to commit is not the one
+ *                           holding the mutex
+ *         EC_ERROR_UNKNOWN  in other error cases
  */
 int nvmem_commit(void);
 
@@ -167,20 +178,28 @@ int nvmem_commit(void);
  *
  * This function should be called when NvMem needs to be wiped out.
  *
- * @param generation: Starting generation number of partition 0
- *
  * @return EC_SUCCESS if flash operations are successful.
  *         EC_ERROR_UNKNOWN otherwise.
  */
-int nvmem_setup(uint8_t generation);
+int nvmem_setup(void);
 
 /*
  * Temporarily stopping NVMEM commits could be beneficial. One use case is
  * when TPM operations need to be sped up.
  *
+ * Calling this function will wait for the mutex, then lock it until
+ * nvmem_commit() is invoked.
+ *
  * Both below functions should be called from the same task.
  */
-void nvmem_enable_commits(void);
 void nvmem_disable_commits(void);
 
+/*
+ * Only the task holding the mutex is allowed to enable commits.
+ *
+ * @return error if this task does not hold the lock or commit
+ *         fails, EC_SUCCESS otherwise.
+ */
+int nvmem_enable_commits(void);
+
 #endif /* __CROS_EC_NVMEM_UTILS_H */
diff --git a/test/build.mk b/test/build.mk
index ae1304cf59..bd124dacd1 100644
--- a/test/build.mk
+++ b/test/build.mk
@@ -58,6 +58,7 @@ test-list-host += lightbar
 test-list-host += math_util
 test-list-host += motion_lid
 test-list-host += mutex
+test-list-host += nvmem
 test-list-host += nvmem_vars
 test-list-host += pingpong
 test-list-host += power_button
diff --git a/test/nvmem.c b/test/nvmem.c
index b00c6f8569..b70c4732f4 100644
--- a/test/nvmem.c
+++ b/test/nvmem.c
@@ -199,7 +199,7 @@ static int test_nvmem_setup(void)
 	TEST_ASSERT(read_value == write_value);
 
 	/* nvmem_setup() is supposed to erase both partitions. */
-	nvmem_setup(0);
+	nvmem_setup();
 
 	nvmem_read(0, sizeof(read_value), &read_value, NVMEM_USER_0);
 	TEST_ASSERT(read_value == 0xffffffff);
@@ -258,14 +258,14 @@ static int test_corrupt_nvmem(void)
 	memset(write_buffer, invalid_value, NVMEM_PARTITION_SIZE);
 	p_data = (void *)CONFIG_FLASH_NVMEM_BASE_A;
 	TEST_ASSERT_ARRAY_EQ(write_buffer, p_data, NVMEM_PARTITION_SIZE);
-	memset(write_buffer, 0xff, NVMEM_PARTITION_SIZE);
 
-	/* Now let's write one byte of 'invalid_value' into user NVMEM_CR50 */
-	TEST_ASSERT(nvmem_write(0, 1, write_buffer, NVMEM_USER_0) ==
-		    EC_SUCCESS);
+	/* Now let's write a different value  into user NVMEM_CR50 */
+	invalid_value ^= ~0;
+	TEST_ASSERT(nvmem_write(0, sizeof(invalid_value),
+				&invalid_value, NVMEM_USER_0) == EC_SUCCESS);
 	TEST_ASSERT(nvmem_commit() == EC_SUCCESS);
 
-	/* Verify that partition 0 genration did not change. */
+	/* Verify that partition 1 generation did not change. */
 	TEST_ASSERT(p_part->generation == 0);
 
 	/*
@@ -346,38 +346,6 @@ static int test_write_fail(void)
 	return !ret;
 }
 
-static int test_cache_not_available(void)
-{
-	char **p_shared;
-	int ret;
-	uint32_t offset = 0;
-	uint32_t num_bytes = 0x200;
-
-	/*
-	 * The purpose of this test is to validate that NvMem writes behave as
-	 * expected when the shared memory buffer (used for cache ram) is and
-	 * isn't available.
-	 */
-
-	/* Do write/read sequence that's expected to be successful */
-	if (test_write_read(offset, num_bytes, NVMEM_USER_1))
-		return EC_ERROR_UNKNOWN;
-
-	/* Acquire shared memory */
-	if (shared_mem_acquire(num_bytes, p_shared))
-		return EC_ERROR_UNKNOWN;
-
-	/* Attempt write/read sequence that should fail */
-	ret = test_write_read(offset, num_bytes, NVMEM_USER_1);
-	/* Release shared memory */
-	shared_mem_release(*p_shared);
-	if (!ret)
-		return EC_ERROR_UNKNOWN;
-
-	/* Write/read sequence should work now */
-	return test_write_read(offset, num_bytes, NVMEM_USER_1);
-}
-
 static int test_buffer_overflow(void)
 {
 	int ret;
@@ -625,6 +593,82 @@ static int test_lock(void)
 	return EC_SUCCESS;
 }
 
+static int test_nvmem_save(void)
+{
+	/*
+	 * The purpose of this test is to verify that if the written value
+	 * did not change the cache contents there is no actual write
+	 * happening at the commit time.
+	 */
+	int dummy_value;
+	int offset = 0x10;
+	uint8_t generation_a;
+	uint8_t generation_b;
+	uint8_t prev_generation;
+	uint8_t new_generation;
+	const struct nvmem_tag *part_a;
+	const struct nvmem_tag *part_b;
+	const struct nvmem_tag *new_gen_part;
+	const struct nvmem_tag *prev_gen_part;
+
+	part_a = (const struct nvmem_tag *)CONFIG_FLASH_NVMEM_BASE_A;
+	part_b = (const struct nvmem_tag *)CONFIG_FLASH_NVMEM_BASE_B;
+	/*
+	 * Make sure nvmem is initialized and both partitions have been
+	 * written.
+	 */
+	nvmem_init();
+
+	/*
+	 * Make sure something is changed at offset 0x10 into the second user
+	 * space.
+	 */
+	nvmem_read(offset, sizeof(dummy_value), &dummy_value, NVMEM_USER_1);
+	dummy_value ^= ~0;
+	nvmem_write(0x10, sizeof(dummy_value), &dummy_value, NVMEM_USER_1);
+	nvmem_commit();
+
+	/* Verify that the two generation values are different. */
+	generation_a = part_a->generation;
+	generation_b = part_b->generation;
+	TEST_ASSERT(generation_a != generation_b);
+
+	/*
+	 * Figure out which one should change next, we are close to the
+	 * beginnig of the test, no wrap is expected.
+	 */
+	if (generation_a > generation_b) {
+		prev_generation = generation_a;
+		new_generation = generation_a + 1;
+		new_gen_part = part_b;
+		prev_gen_part = part_a;
+	} else {
+		prev_generation = generation_b;
+		new_generation = generation_b + 1;
+		new_gen_part = part_a;
+		prev_gen_part = part_b;
+	}
+
+	/* Write a new value, this should trigger generation switch. */
+	dummy_value += 1;
+	TEST_ASSERT(nvmem_write(0x10, sizeof(dummy_value),
+				&dummy_value, NVMEM_USER_1) == EC_SUCCESS);
+	TEST_ASSERT(nvmem_commit() == EC_SUCCESS);
+
+	TEST_ASSERT(prev_gen_part->generation == prev_generation);
+	TEST_ASSERT(new_gen_part->generation == new_generation);
+
+	/* Write the same value, this should NOT trigger generation switch. */
+	TEST_ASSERT(nvmem_write(0x10, sizeof(dummy_value),
+				&dummy_value, NVMEM_USER_1) == EC_SUCCESS);
+	TEST_ASSERT(nvmem_commit() == EC_SUCCESS);
+
+	TEST_ASSERT(prev_gen_part->generation == prev_generation);
+	TEST_ASSERT(new_gen_part->generation == new_generation);
+
+	return EC_SUCCESS;
+}
+
 static void run_test_setup(void)
 {
 	/* Allow Flash erase/writes */
@@ -635,26 +679,17 @@ static void run_test_setup(void)
 void run_test(void)
 {
 	run_test_setup();
-	/* Test NvMem Initialization function */
 	RUN_TEST(test_corrupt_nvmem);
 	RUN_TEST(test_fully_erased_nvmem);
 	RUN_TEST(test_configured_nvmem);
-	/* Test Read/Write/Commit functions */
 	RUN_TEST(test_write_read_sequence);
 	RUN_TEST(test_write_full_multi);
-	/* Test flash erase/write fail case */
 	RUN_TEST(test_write_fail);
-	/* Test shared_mem not available case */
-	RUN_TEST(test_cache_not_available);
-	/* Test buffer overflow logic */
 	RUN_TEST(test_buffer_overflow);
-	/* Test NvMem Move function */
 	RUN_TEST(test_move);
-	/* Test NvMem IsDifferent function */
 	RUN_TEST(test_is_different);
-	/* Test Nvmem write lock */
 	RUN_TEST(test_lock);
-	/* Test nvmem_setup() function. */
 	RUN_TEST(test_nvmem_setup);
+	RUN_TEST(test_nvmem_save);
 	test_print_result();
 }
diff --git a/test/test_config.h b/test/test_config.h
index fcad730c7f..db426ce01b 100644
--- a/test/test_config.h
+++ b/test/test_config.h
@@ -205,7 +205,7 @@ enum nvmem_users {
 #endif
 
 #ifdef TEST_NVMEM_VARS
-#define NVMEM_PARTITION_SIZE 0x4000
+#define NVMEM_PARTITION_SIZE 0x3000
 #define CONFIG_FLASH_NVMEM_VARS
 #ifndef __ASSEMBLER__
 /* Define the user region numbers */