nvram: Flag dangerous NVRAM options

Most nvram options used by skiboot are just for debug or testing for
regressions. They should never be used long term.

We've hit a number of issues in testing and the field where nvram
options have been set "temporarily" but haven't been properly cleared
after, resulting in crashes or real bugs being masked.

This patch marks most nvram options used by skiboot as dangerous and
prints a chicken to remind users of the problem.

Signed-off-by: Michael Neuling <mikey@neuling.org>
Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Acked-By: Alistair Popple <alistair@popple.id.au>
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>

5 files changed, 61 insertions, 18 deletions

diff --git a/core/hmi.c b/core/hmi.c
index e813286..af3f7fe 100644
--- a/core/hmi.c
+++ b/core/hmi.c
@@ -672,7 +672,7 @@ static void find_npu2_checkstop_reason(int flat_chip_id,
 	if (!total_errors)
 		return;
 
-	npu2_hmi_verbose = nvram_query_eq("npu2-hmi-verbose", "true");
+	npu2_hmi_verbose = nvram_query_eq_safe("npu2-hmi-verbose", "true");
 	/* Force this for now until we sort out something better */
 	npu2_hmi_verbose = true;
 
diff --git a/core/init.c b/core/init.c
index bca12df..c066163 100644
--- a/core/init.c
+++ b/core/init.c
@@ -582,7 +582,7 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
 	fsp_console_select_stdout();
 
 	/* Use nvram bootargs over device tree */
-	cmdline = nvram_query("bootargs");
+	cmdline = nvram_query_safe("bootargs");
 	if (cmdline) {
 		dt_check_del_prop(dt_chosen, "bootargs");
 		dt_add_property_string(dt_chosen, "bootargs", cmdline);
@@ -740,7 +740,7 @@ static void console_log_level(void)
 
 	/* console log level:
 	 *   high 4 bits in memory, low 4 bits driver (e.g. uart). */
-	s = nvram_query("log-level-driver");
+	s = nvram_query_safe("log-level-driver");
 	if (s) {
 		level = console_get_level(s);
 		debug_descriptor.console_log_levels =
@@ -749,7 +749,7 @@ static void console_log_level(void)
 		prlog(PR_NOTICE, "console: Setting driver log level to %i\n",
 		      level & 0x0f);
 	}
-	s = nvram_query("log-level-memory");
+	s = nvram_query_safe("log-level-memory");
 	if (s) {
 		level = console_get_level(s);
 		debug_descriptor.console_log_levels =
@@ -867,7 +867,7 @@ static void pci_nvram_init(void)
 
 	pcie_max_link_speed = 0;
 
-	nvram_speed = nvram_query("pcie-max-link-speed");
+	nvram_speed = nvram_query_dangerous("pcie-max-link-speed");
 	if (nvram_speed) {
 		pcie_max_link_speed = atoi(nvram_speed);
 		prlog(PR_NOTICE, "PHB: NVRAM set max link speed to GEN%i\n",
diff --git a/core/nvram-format.c b/core/nvram-format.c
index b2fc960..e34add3 100644
--- a/core/nvram-format.c
+++ b/core/nvram-format.c
@@ -206,13 +206,31 @@ static const char *find_next_key(const char *start, const char *end)
 	return NULL;
 }
 
+static void nvram_dangerous(const char *key)
+{
+	prlog(PR_ERR, " ___________________________________________________________\n");
+	prlog(PR_ERR, "<  Dangerous NVRAM option: %s\n", key);
+	prlog(PR_ERR, " -----------------------------------------------------------\n");
+	prlog(PR_ERR, "                  \\                         \n");
+	prlog(PR_ERR, "                   \\   WW                   \n");
+	prlog(PR_ERR, "                      <^ \\___/|             \n");
+	prlog(PR_ERR, "                       \\      /             \n");
+	prlog(PR_ERR, "                        \\_  _/              \n");
+	prlog(PR_ERR, "                          }{                 \n");
+}
+
+
 /*
- * nvram_query() - Searches skiboot NVRAM partition for a key=value pair.
+ * nvram_query_safe/dangerous() - Searches skiboot NVRAM partition
+ * for a key=value pair.
+ *
+ * Dangerous means it should only be used for testing as it may
+ * mask issues. Safe is ok for long term use.
  *
  * Returns a pointer to a NUL terminated string that contains the value
  * associated with the given key.
  */
-const char *nvram_query(const char *key)
+static const char *__nvram_query(const char *key, bool dangerous)
 {
 	const char *part_end, *start;
 	int key_len = strlen(key);
@@ -269,6 +287,8 @@ const char *nvram_query(const char *key)
 			prlog(PR_DEBUG, "NVRAM: Searched for '%s' found '%s'\n",
 				key, value);
 
+			if (dangerous)
+				nvram_dangerous(start);
 			return value;
 		}
 
@@ -280,18 +300,30 @@ const char *nvram_query(const char *key)
 	return NULL;
 }
 
+const char *nvram_query_safe(const char *key)
+{
+	return __nvram_query(key, false);
+}
+
+const char *nvram_query_dangerous(const char *key)
+{
+	return __nvram_query(key, true);
+}
 
 /*
- * nvram_query_eq() - Check if the given 'key' exists and
- * is set to 'value'.
+ * nvram_query_eq_safe/dangerous() - Check if the given 'key' exists
+ * and is set to 'value'.
+ *
+ * Dangerous means it should only be used for testing as it may
+ * mask issues. Safe is ok for long term use.
  *
  * Note: Its an error to check for non-existence of a key
  * by passing 'value == NULL' as a key's value can never be
  * NULL in nvram.
  */
-bool nvram_query_eq(const char *key, const char *value)
+static bool __nvram_query_eq(const char *key, const char *value, bool dangerous)
 {
-	const char *s = nvram_query(key);
+	const char *s = __nvram_query(key, dangerous);
 
 	if (!s)
 		return false;
@@ -299,3 +331,14 @@ bool nvram_query_eq(const char *key, const char *value)
 	assert(value != NULL);
 	return !strcmp(s, value);
 }
+
+bool nvram_query_eq_safe(const char *key, const char *value)
+{
+	return __nvram_query_eq(key, value, false);
+}
+
+bool nvram_query_eq_dangerous(const char *key, const char *value)
+{
+	return __nvram_query_eq(key, value, true);
+}
+
diff --git a/core/platform.c b/core/platform.c
index 62361f5..afa00ad 100644
--- a/core/platform.c
+++ b/core/platform.c
@@ -59,13 +59,13 @@ static int64_t opal_cec_reboot(void)
 
 	opal_quiesce(QUIESCE_HOLD, -1);
 
-	if (proc_gen == proc_gen_p8 && nvram_query_eq("fast-reset","1")) {
+	if (proc_gen == proc_gen_p8 && nvram_query_eq_safe("fast-reset","1")) {
 		/*
 		 * Bugs in P8 mean fast reboot isn't 100% reliable when cores
 		 * are busy, so only attempt if explicitly *enabled*.
 		 */
 		fast_reboot();
-	} else if (!nvram_query_eq("fast-reset","0")) {
+	} else if (!nvram_query_eq_safe("fast-reset","0")) {
 		/* Try fast-reset unless explicitly disabled */
 		fast_reboot();
 	}
diff --git a/core/test/run-nvram-format.c b/core/test/run-nvram-format.c
index d86b1dc..c8a7583 100644
--- a/core/test/run-nvram-format.c
+++ b/core/test/run-nvram-format.c
@@ -144,23 +144,23 @@ int main(void)
 
 	/* does an empty partition break us? */
 	data = nvram_reset(nvram_image, 128*1024);
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does a zero length key break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = '=';
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does a missing = break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = 'a';
-	assert(nvram_query("test") == NULL);
+	assert(nvram_query_safe("test") == NULL);
 
 	/* does an empty value break us? */
 	data = nvram_reset(nvram_image, 128*1024);
 	data[0] = 'a';
 	data[1] = '=';
-	result = nvram_query("a");
+	result = nvram_query_safe("a");
 	assert(result);
 	assert(strlen(result) == 0);
 
@@ -168,7 +168,7 @@ int main(void)
 	data = nvram_reset(nvram_image, 128*1024);
 #define TEST_1 "a\0a=\0test=test\0"
 	memcpy(data, TEST_1, sizeof(TEST_1));
-	result = nvram_query("test");
+	result = nvram_query_safe("test");
 	assert(result);
 	assert(strcmp(result, "test") == 0);