hwpstate_amd(4): attach(): More diagnostic on CPPC enable

When the 'debug.hwpstate_verbose' tunable/sysctl knob is set, dump the
initial content of the CPPC_CAPABILITY_1 and CPPC_REQUEST registers.

If, after enabling CPPC, reading/writing some MSR fails during the attach
sequence, print a diagnostic.  However, once CPPC is enabled, we cannot
go back (disabling it is impossible), so we'll attach even if fiddling
with other MSRs failed.

While here, move diagnostic printing on attach out of the callback that
is executed on (potentially) another CPU and with interrupts disabled,
putting it into the attach routine itself.

While here, fix format for printing the CPU ID.

PR:             292615
Reviewed by:    aokblast (older version)
Sponsored by:   The FreeBSD Foundation
Differential Revision:  https://reviews.freebsd.org/D55006

sys/x86/cpufreq/hwpstate_amd.c

@@ -261,7 +261,7 @@ print_cppc_caps_1(struct sbuf *const sb, const uint64_t caps)
 	    AMD_CPPC_CAPS_1_LOWEST_PERF_BITS, caps);
 }
 
-#define MSR_NOT_READ_MSG	"Fault on read"
+#define MSR_NOT_READ_MSG	"Not read (fault or previous errors)"
 
 static void
 print_cppc_no_caps_1(struct sbuf *const sb)
@@ -644,46 +644,45 @@ hwpstate_identify(driver_t *driver, device_t parent)
 		device_printf(parent, "hwpstate: add child failed\n");
 }
 
-struct amd_set_autonomous_hwp_request {
+struct set_autonomous_hwp_data {
-	device_t dev;
+	/* Inputs */
-	int res;
+	struct hwpstate_softc *sc;
+	/* Outputs */
+	/* HWP_ERROR_CPPC_* */
+	u_int res;
+	/* Below fields filled depending on 'res'. */
+	uint64_t caps;
+	uint64_t init_request;
+	uint64_t request;
 };
 
 static void
 amd_set_autonomous_hwp_cb(void *args)
 {
-	struct hwpstate_softc *sc;
+	struct set_autonomous_hwp_data *const data = args;
-	struct amd_set_autonomous_hwp_request *req =
+	struct hwpstate_softc *const sc = data->sc;
-	    (struct amd_set_autonomous_hwp_request *)args;
+	int error;
-	device_t dev;
-	uint64_t caps;
-	int ret;
 
-	dev = req->dev;
+	/* We proceed sequentially, so we'll clear out errors on progress. */
-	sc = device_get_softc(dev);
+	data->res = HWP_ERROR_CPPC_ENABLE | HWP_ERROR_CPPC_CAPS |
-	ret = wrmsr_safe(MSR_AMD_CPPC_ENABLE, 1);
+	    HWP_ERROR_CPPC_REQUEST | HWP_ERROR_CPPC_REQUEST_WRITE;
-	if (ret != 0) {
-		device_printf(dev, "Failed to enable cppc for cpu%d (%d)\n",
-		    curcpu, ret);
-		req->res = ret;
-	}
 
-	ret = rdmsr_safe(MSR_AMD_CPPC_REQUEST, &sc->cppc.request);
+	error = wrmsr_safe(MSR_AMD_CPPC_ENABLE, 1);
-	if (ret != 0) {
+	if (error != 0)
-		device_printf(dev,
-		    "Failed to read CPPC request MSR for cpu%d (%d)\n", curcpu,
-		    ret);
-		req->res = ret;
-	}
-
-	ret = rdmsr_safe(MSR_AMD_CPPC_CAPS_1, &caps);
-	if (ret != 0) {
-		device_printf(dev,
-		    "Failed to read HWP capabilities MSR for cpu%d (%d)\n",
-		    curcpu, ret);
-		req->res = ret;
 		return;
-	}
+	data->res &= ~HWP_ERROR_CPPC_ENABLE;
+
+	error = rdmsr_safe(MSR_AMD_CPPC_CAPS_1, &data->caps);
+	if (error != 0)
+		return;
+	data->res &= ~HWP_ERROR_CPPC_CAPS;
+
+	error = rdmsr_safe(MSR_AMD_CPPC_REQUEST, &sc->cppc.request);
+	if (error != 0)
+		return;
+	data->res &= ~HWP_ERROR_CPPC_REQUEST;
+	/* The CPPC_REQUEST value before we tweak it. */
+	data->init_request = sc->cppc.request;
 
 	/*
 	 * In Intel's reference manual, the default value of EPP is 0x80u which
@@ -692,35 +691,71 @@ amd_set_autonomous_hwp_cb(void *args)
 	 */
 	SET_BITS_VALUE(sc->cppc.request, AMD_CPPC_REQUEST_EPP_BITS, 0x80);
 	SET_BITS_VALUE(sc->cppc.request, AMD_CPPC_REQUEST_MIN_PERF_BITS,
-	    BITS_VALUE(AMD_CPPC_CAPS_1_LOWEST_PERF_BITS, caps));
+	    BITS_VALUE(AMD_CPPC_CAPS_1_LOWEST_PERF_BITS, data->caps));
 	SET_BITS_VALUE(sc->cppc.request, AMD_CPPC_REQUEST_MAX_PERF_BITS,
-	    BITS_VALUE(AMD_CPPC_CAPS_1_HIGHEST_PERF_BITS, caps));
+	    BITS_VALUE(AMD_CPPC_CAPS_1_HIGHEST_PERF_BITS, data->caps));
 	/* enable autonomous mode by setting desired performance to 0 */
 	SET_BITS_VALUE(sc->cppc.request, AMD_CPPC_REQUEST_DES_PERF_BITS, 0);
 
-	ret = wrmsr_safe(MSR_AMD_CPPC_REQUEST, sc->cppc.request);
+	error = wrmsr_safe(MSR_AMD_CPPC_REQUEST, sc->cppc.request);
-	if (ret) {
+	if (error != 0)
-		device_printf(dev, "Failed to setup autonomous HWP for cpu%d\n",
-		    curcpu);
-		req->res = ret;
 		return;
-	}
+	data->res &= ~HWP_ERROR_CPPC_REQUEST_WRITE;
-	req->res = 0;
+	data->request = sc->cppc.request;
 }
 
 static int
 amd_set_autonomous_hwp(struct hwpstate_softc *sc)
 {
-	struct amd_set_autonomous_hwp_request req;
+	const device_t dev = sc->dev;
-	device_t dev;
+	const u_int cpuid = cpu_get_pcpu(dev)->pc_cpuid;
+	struct set_autonomous_hwp_data data;
+	struct sbuf sbs;
+	struct sbuf *sb;
 
-	dev = sc->dev;
+	data.sc = sc;
-	req.dev = dev;
+	smp_rendezvous_cpu(cpuid, smp_no_rendezvous_barrier,
-	smp_rendezvous_cpu(cpu_get_pcpu(dev)->pc_cpuid,
+	    amd_set_autonomous_hwp_cb, smp_no_rendezvous_barrier, &data);
-	    smp_no_rendezvous_barrier, amd_set_autonomous_hwp_cb,
-	    smp_no_rendezvous_barrier, &req);
 
-	return (req.res);
+	if (hwp_has_error(data.res, HWP_ERROR_CPPC_ENABLE)) {
+		device_printf(dev, "CPU%u: Failed to enable CPPC!\n", cpuid);
+		return (ENXIO);
+	}
+	device_printf(dev, "CPU%u: CPPC enabled.\n", cpuid);
+
+	/*
+	 * Now that we have enabled CPPC, we can't go back, so we'll attach even
+	 * in case of further malfunction, allowing the user to retry setting
+	 * CPPC_REQUEST via the sysctl knobs.
+	 */
+
+	sb = sbuf_new(&sbs, NULL, 0, SBUF_AUTOEXTEND);
+
+	if (hwpstate_verbose)
+		sbuf_printf(sb,
+		    "CPU%u: Initial MSR values after CPPC enable:\n", cpuid);
+	if (hwp_has_error(data.res, HWP_ERROR_CPPC_CAPS))
+		print_cppc_no_caps_1(sb);
+	else if (hwpstate_verbose)
+		print_cppc_caps_1(sb, data.caps);
+	if (hwp_has_error(data.res, HWP_ERROR_CPPC_REQUEST))
+		print_cppc_no_request(sb);
+	else if (hwpstate_verbose)
+		print_cppc_request(sb, data.init_request);
+	if (hwp_has_error(data.res, HWP_ERROR_CPPC_REQUEST_WRITE))
+		device_printf(dev, "CPU%u: Could not write into "
+		    MSR_AMD_CPPC_REQUEST_NAME "!\n",
+		    cpuid);
+	else if (hwpstate_verbose) {
+		sbuf_printf(sb, "CPU%u: Tweaked MSR values:\n", cpuid);
+		print_cppc_request(sb, data.request);
+	}
+
+	sbuf_finish(sb);
+	sbuf_putbuf(sb);
+	sbuf_delete(sb);
+
+	return (0);
 }
 
 static int