nvram: Flag dangerous NVRAM options

[ Upstream commit 5beda3c6fe5b72aac95b4c13746ae598dfd64c01 ]

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>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

15 files changed, 78 insertions, 34 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);
 
diff --git a/hw/lpc-uart.c b/hw/lpc-uart.c
index 365bf3e..bca10e0 100644
--- a/hw/lpc-uart.c
+++ b/hw/lpc-uart.c
@@ -506,7 +506,7 @@ static void uart_init_opal_console(void)
 	/* Update the policy if the corresponding nvram variable
 	 * is present
 	 */
-	nv_policy = nvram_query("uart-con-policy");
+	nv_policy = nvram_query_dangerous("uart-con-policy");
 	if (nv_policy) {
 		if (!strcmp(nv_policy, "opal"))
 			uart_console_policy = UART_CONSOLE_OPAL;
diff --git a/hw/npu2-common.c b/hw/npu2-common.c
index 16b285d..f3f2f45 100644
--- a/hw/npu2-common.c
+++ b/hw/npu2-common.c
@@ -663,7 +663,7 @@ void probe_npu2(void)
 	}
 
 	/* Check for a zcal override */
-	zcal = nvram_query("nv_zcal_override");
+	zcal = nvram_query_dangerous("nv_zcal_override");
 	if (zcal) {
 		nv_zcal_nominal = atoi(zcal);
 		prlog(PR_WARNING, "NPU2: Using ZCAL impedance override = %d\n", nv_zcal_nominal);
diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
index 9df51b2..27dfc12 100644
--- a/hw/npu2-opencapi.c
+++ b/hw/npu2-opencapi.c
@@ -1727,7 +1727,7 @@ static void read_nvram_training_state(void)
 {
 	const char *state;
 
-	state = nvram_query("opencapi-link-training");
+	state = nvram_query_dangerous("opencapi-link-training");
 	if (state) {
 		if (!strcmp(state, "prbs31"))
 			npu2_ocapi_training_state = NPU2_TRAIN_PRBS31;
diff --git a/hw/npu2.c b/hw/npu2.c
index 0d79d8a..97139dd 100644
--- a/hw/npu2.c
+++ b/hw/npu2.c
@@ -1473,7 +1473,7 @@ int npu2_nvlink_init_npu(struct npu2 *npu)
 	 * it throws machine checkstop. Disabling snarfing fixes this so let's
 	 * disable it by default.
 	 */
-	if (nvram_query_eq("opal-npu2-snarf-cpm", "enable")) {
+	if (nvram_query_eq_dangerous("opal-npu2-snarf-cpm", "enable")) {
 		prlog(PR_WARNING, "NPU2#%d: enabling Probe.I.MO snarfing, a bad GPU driver may crash the system!\n",
 				npu->index);
 		val |= PPC_BIT(40); /* CONFIG_ENABLE_SNARF_CPM */
diff --git a/hw/phb4.c b/hw/phb4.c
index 3b0ebfb..5565d94 100644
--- a/hw/phb4.c
+++ b/hw/phb4.c
@@ -5919,16 +5919,16 @@ void probe_phb4(void)
 	struct dt_node *np;
 	const char *s;
 
-	verbose_eeh = nvram_query_eq("pci-eeh-verbose", "true");
+	verbose_eeh = nvram_query_eq_safe("pci-eeh-verbose", "true");
 	/* REMOVEME: force this for now until we stabalise PCIe */
 	verbose_eeh = 1;
 	if (verbose_eeh)
 		prlog(PR_INFO, "PHB4: Verbose EEH enabled\n");
 
-	pci_tracing = nvram_query_eq("pci-tracing", "true");
-	pci_eeh_mmio = !nvram_query_eq("pci-eeh-mmio", "disabled");
-	pci_retry_all = nvram_query_eq("pci-retry-all", "true");
-	s = nvram_query("phb-rx-err-max");
+	pci_tracing = nvram_query_eq_safe("pci-tracing", "true");
+	pci_eeh_mmio = !nvram_query_eq_dangerous("pci-eeh-mmio", "disabled");
+	pci_retry_all = nvram_query_eq_dangerous("pci-retry-all", "true");
+	s = nvram_query_dangerous("phb-rx-err-max");
 	if (s) {
 		rx_err_max = atoi(s);
 
@@ -5937,7 +5937,6 @@ void probe_phb4(void)
 		rx_err_max = MIN(rx_err_max, 255);
 	}
 	prlog(PR_DEBUG, "PHB4: Maximum RX errors during training: %d\n", rx_err_max);
-
 	/* Look for PBCQ XSCOM nodes */
 	dt_for_each_compatible(dt_root, np, "ibm,power9-pbcq")
 		phb4_probe_pbcq(np);
diff --git a/hw/slw.c b/hw/slw.c
index adbfdce..fcf4d57 100644
--- a/hw/slw.c
+++ b/hw/slw.c
@@ -883,7 +883,7 @@ void add_cpu_idle_state_properties(void)
 		if (wakeup_engine_state == WAKEUP_ENGINE_PRESENT)
 			supported_states_mask |= OPAL_PM_WINKLE_ENABLED;
 	}
-	nvram_disable_str = nvram_query("opal-stop-state-disable-mask");
+	nvram_disable_str = nvram_query_dangerous("opal-stop-state-disable-mask");
 	if (nvram_disable_str)
 		nvram_disabled_states_mask = strtol(nvram_disable_str, NULL, 0);
 	prlog(PR_DEBUG, "NVRAM stop disable mask: %x\n", nvram_disabled_states_mask);
diff --git a/hw/xscom.c b/hw/xscom.c
index b652e61..30f4e83 100644
--- a/hw/xscom.c
+++ b/hw/xscom.c
@@ -833,7 +833,7 @@ int64_t xscom_trigger_xstop(void)
 	int rc = OPAL_UNSUPPORTED;
 	bool xstop_disabled = false;
 
-	if (nvram_query_eq("opal-sw-xstop", "disable"))
+	if (nvram_query_eq_dangerous("opal-sw-xstop", "disable"))
 		xstop_disabled = true;
 
 	if (xstop_disabled) {
diff --git a/include/nvram.h b/include/nvram.h
index 012c107..a018a11 100644
--- a/include/nvram.h
+++ b/include/nvram.h
@@ -24,7 +24,9 @@ bool nvram_validate(void);
 bool nvram_has_loaded(void);
 bool nvram_wait_for_load(void);
 
-const char *nvram_query(const char *name);
-bool nvram_query_eq(const char *key, const char *value);
+const char *nvram_query_safe(const char *name);
+const char *nvram_query_dangerous(const char *name);
+bool nvram_query_eq_safe(const char *key, const char *value);
+bool nvram_query_eq_dangerous(const char *key, const char *value);
 
 #endif /* __NVRAM_H */
diff --git a/libstb/secureboot.c b/libstb/secureboot.c
index 4f6a301..1578f52 100644
--- a/libstb/secureboot.c
+++ b/libstb/secureboot.c
@@ -104,7 +104,7 @@ void secureboot_init(void)
 
 	prlog(PR_DEBUG, "Found %s\n", compat);
 
-	if (nvram_query_eq("force-secure-mode", "always")) {
+	if (nvram_query_eq_dangerous("force-secure-mode", "always")) {
 		secure_mode = true;
 		prlog(PR_NOTICE, "secure mode on (FORCED by nvram)\n");
 	} else {
diff --git a/libstb/trustedboot.c b/libstb/trustedboot.c
index ae2cc55..d9bacb2 100644
--- a/libstb/trustedboot.c
+++ b/libstb/trustedboot.c
@@ -102,7 +102,7 @@ void trustedboot_init(void)
 		return;
 	}
 
-	if (nvram_query_eq("force-trusted-mode", "true")) {
+	if (nvram_query_eq_dangerous("force-trusted-mode", "true")) {
 		trusted_mode = true;
 		prlog(PR_NOTICE, "trusted mode on (FORCED by nvram)\n");
 	} else {