hvc_dcc: bind driver to core0 for reads and writes

From: Shanker Donthineni <[email protected]>

Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle reads/writes from/to DCC on secondary cores. Each core has its own DCC device registers, so when a core reads or writes from/to DCC, it only accesses its own DCC device. Since kernel code can run on any core, every time the kernel wants to write to the console, it might write to a different DCC.

In SMP mode, Trace32 only uses the DCC on core 0. In AMP mode, it creates multiple windows, and each window shows the DCC output only from that core's DCC. The result is that console output is either lost or scattered across windows.

Selecting this option will enable code that serializes all console input and output to core 0. The DCC driver will create input and output FIFOs that all cores will use. Reads and writes from/to DCC are handled by a workqueue that runs only core 0.

---
 drivers/tty/hvc/Kconfig   |  21 +++++++
 drivers/tty/hvc/hvc_dcc.c | 157 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 177 insertions(+), 1 deletion(-)

index 8902f9b..2c6883c 100644
@@ -95,6 +95,27 @@ config HVC_DCC
 	 driver. This console is used through a JTAG only on ARM. If you don't have
 	 a JTAG then you probably don't want this option.
 
+config HVC_DCC_SERIALIZE_SMP
+	bool "Use DCC only on core 0"
+	depends on SMP && HVC_DCC
+	help
+	  Some debuggers, such as Trace32 from Lauterbach GmbH, do not handle
+	  reads/writes from/to DCC on more than one core.  Each core has its
+	  own DCC device registers, so when a core reads or writes from/to DCC,
+	  it only accesses its own DCC device.  Since kernel code can run on
+	  any core, every time the kernel wants to write to the console, it
+	  might write to a different DCC.
+
+	  In SMP mode, Trace32 only uses the DCC on core 0.  In AMP mode, it
+	  creates multiple windows, and each window shows the DCC output
+	  only from that core's DCC.  The result is that console output is
+	  either lost or scattered across windows.
+
+	  Selecting this option will enable code that serializes all console
+	  input and output to core 0.  The DCC driver will create input and
+	  output FIFOs that all cores will use.  Reads and writes from/to DCC
+	  are handled by a workqueue that runs only core 0.
+
 config HVC_BFIN_JTAG
 	bool "Blackfin JTAG console"
 	depends on BLACKFIN

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

hvc_instantiate() and hvc_alloc() return errors if they fail, so don't ignore them.

---
 drivers/tty/hvc/hvc_dcc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

index 33657dc..f8b8cf2 100644
@@ -225,20 +225,29 @@ static const struct hv_ops hvc_dcc_get_put_ops = {
 
 static int __init hvc_dcc_console_init(void)
 {
+	int ret;
+
+	/* This always runs on boot core */
 	if (!hvc_dcc_check())
 		return -ENODEV;
 
-	hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
-	return 0;
+	/* Returns -1 if error */
+	ret = hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
+
+	return ret < 0 ? -ENODEV : 0;
 }
 console_initcall(hvc_dcc_console_init);
 
 static int __init hvc_dcc_init(void)
 {
+	struct hvc_struct *p;
+
+	/* This can run on any core */
 	if (!hvc_dcc_check())
 		return -ENODEV;
 
-	hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
-	return 0;
+	p = hvc_alloc(0, 0, &hvc_dcc_get_put_ops, 128);
+
+	return IS_ERR(p) ? PTR_ERR(p) : 0;
 }
 device_initcall(hvc_dcc_init);

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Well, I didn't want to change common HVC code. I would have to change all the other drivers that call hvc_instatiate as well. I agree that returning -1 is bad, but fixing that is something that should be done in another patch.

Yes, thanks. I'll change it.

From: Abhimanyu Kapur <[email protected]>

Add support for debug communications channel based hvc console for arm64 cpus.

---
 arch/arm64/include/asm/dcc.h | 52 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/tty/hvc/Kconfig      |  2 +-
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/dcc.h

new file mode 100644
index 0000000..fcb8d7d
@@ -0,0 +1,52 @@
+/* Copyright (c) 2014-2015 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * A call to __dcc_getchar() or __dcc_putchar() is typically followed by
+ * a call to __dcc_getstatus().  We want to make sure that the CPU does
+ * not speculative read the DCC status before executing the read or write
+ * instruction.  That's what the ISBs are for.
+ *
+ * The 'volatile' ensures that the compiler does not cache the status bits,
+ * and instead reads the DCC register every time.
+ */
+#ifndef __ASM_DCC_H
+#define __ASM_DCC_H
+
+#include <asm/barrier.h>
+
+static inline u32 __dcc_getstatus(void)
+{
+	u32 ret;
+
+	asm volatile("mrs %0, mdccsr_el0" : "=r" (ret));
+
+	return ret;
+}
+
+static inline char __dcc_getchar(void)
+{
+	char c;
+
+	asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (c));
+	isb();
+
+	return c;
+}
+
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));
+	isb();
+}
+
+#endif

index 2c6883c..9a60d18 100644
@@ -88,7 +88,7 @@ config HVC_UDBG
 
 config HVC_DCC
        bool "ARM JTAG DCC console"
-       depends on ARM
+       depends on ARM || ARM64
        select HVC_DRIVER
        help
          This console uses the JTAG DCC on ARM to create a console under the HVC

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

If the DCC driver loads, then disable user-space access to the DCC so that we don't have two entities trying to access the DCC at the same time.

---
 arch/arm/include/asm/dcc.h   | 15 +++++++++++++++
 arch/arm64/include/asm/dcc.h | 11 +++++++++++
 drivers/tty/hvc/hvc_dcc.c    |  2 ++
 3 files changed, 28 insertions(+)

index b74899d..c50056b 100644
@@ -9,8 +9,11 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
+#ifndef __ASM_DCC_H
+#define __ASM_DCC_H
 
 #include <asm/barrier.h>
+#include <asm/hardware/cp14.h>
 
 static inline u32 __dcc_getstatus(void)
 {

@@ -39,3 +42,15 @@ static inline void __dcc_putchar(char c)
 		: "r" (c));
 	isb();
 }
+
+static inline void __dcc_initialize(void)
+{
+	u32 val;
+
+	/* Disable user-space access to DCC */
+	val = MRC14(0, c0, c1, 0);
+	val |= 1 << 12; /* DSCR[Comms] */
+	MCR14(val, 0, c0, c1, 0);
+}
+
+#endif

index fcb8d7d..7843e62 100644
@@ -49,4 +49,15 @@ static inline void __dcc_putchar(char c)
 	isb();
 }
 
+static inline void __dcc_initialize(void)
+{
+	u32 val;
+
+	/* Disable user-space access to DCC */
+	asm ("mrs %0, mdscr_el1\n"
+		"	orr %0, %0, #4096\n" /* Set the TDCC bit */
+		"	msr mdscr_el1, %0\n"
+		: "=r" (val));
+}
+
 #endif

index f8b8cf2..5ccafa4 100644
@@ -231,6 +231,8 @@ static int __init hvc_dcc_console_init(void)
 	if (!hvc_dcc_check())
 		return -ENODEV;
 
+	__dcc_initialize();
+
 	/* Returns -1 if error */
 	ret = hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
 

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

And no one else is using it either.

I agree, but I didn't want to expand the scope of this patch beyond what it's supposed to do.

Well, I figured it wasn't important exactly why these instructions are executed, since it just disables user-space. I usually get chewed out by someone if I add volatile where it's not needed, so I didn't add it this time.

Hmmm, the only thing I see in debug_monitors.h that seems applicable are the mdscr_write() and mdscr_read() functions, which seem to overlap cp14.h.

Maybe we should look into making the console number (i.e. ttyHVC0, ttyHVC1, etc.) correspond to the logical CPU number 0, 1, etc? We would need some hotplug notifier to tear down and restore the console when the CPU comes online and goes offline, but it may work out nicer than taking the approach this patch does.

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

My understanding is that Trace32 only responds to core 0 in SMP mode. So if CPU0 goes offline, there's no point in migrating the thread to another CPU, because Trace32 won't listen for it anyway. Without this patch, console output is randomly scattered across CPUs because the put_chars call run on any CPU. Without consolidating all console output to one CPU, DCC is effectively useless.

So I can make the changes you suggested, but I don't think that actually fixes anything. When CPU0 goes offline, what does schedule_work_on(0, actually do? If it does nothing, then the output FIFO will fill up, and put_chars will return 0, and that's it.

Does CPU hotplug automatically take CPUs offline when the load is low? If so, then then thread could randomly bounce from CPU to CPU.

Last I checked, ARM64 ACPI does not support discontiguous CPUs, so the boot CPU is always core "0".

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

It would at least fix the AMP case where you have one tty per CPU. If a CPU goes offline, the tty would be removed at the same time. The user could put the console on another CPU, or all of them, if they want to offline CPUs.

It sounds like in the SMP case the tool is broken and it should do better about consolidating output from different CPUs into one place. If it really only listens to the boot CPU then making a tty per-CPU would work just the same as this patch even when the CPU goes offline.

schedule_work_on() shouldn't be used when a CPU can go offline (see the comment above queue_work_on). I think it has to break affinity to run the work item on some other CPU.

Sorry, I should have been clearer. The thread would be bound to the CPU the tty corresponds to. No thread migration or bouncing. We would have to tear down and restore the tty on hotplug as well. I guess one problem here is that it doesn't do anything about the console. The console doesn't go through the khvcd thread like the tty does so we would still need some sort of dcc specific code to move the console writes to a particular CPU. And the downside there is that consoles are supposed to support atomic operations so that debugging information can always get out to the user. When we need to migrate the write to a particular CPU we might lose information if scheduling is broken or something.

So can we fix the tool instead and keep writing out data on random CPUs?

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

How would the user put the TTY on another CPU? The command-line parameter is a boot-time configuration. I'm okay with adding that parameter, but we still have the problem of losing the TTY altogether if that CPU goes offline. I would like to see the TTY migrated to another CPU, but if I don't know if that makes sense either. Is it possible for CPUs to go randomly offline temporarily to manage load?

To be clear, I'm okay with adding a hotplug hook that shuts down the thread if the TTY CPU goes offline, so that we don't attempt to schedule a thread on a CPU that's offline. I am concerned about whether hotplug can automatically offline the TTY CPU without warning, and suddenly we don't have TTY any more.

Is there a way to prevent the TTY CPU from going offline via hotplug? A way of saying, "if you want to offline a CPU, don't offline this one?"

Yes, the tool is broken, but that's not the only problem. Each CPU has its own DCC, and since the console driver can run on any thread, it will scatter console output across all the CPUs. So any tool that listens on DCC is going to have this problem on an SMP system. I'm pretty sure Trace32 isn't the only one.

So if I run schedule_work_on() on a CPU, and that CPU is offline, it will just schedule the workqueue on some other random CPU?

I'm not sure I understand all that. What do you mean by the thread is bound to the CPU that the TTY correspond to? When a kernel thread (running on some CPU) calls printk(), doesn't the call to the HVC driver occur also on that CPU? That's the whole reason behind this patch -- that each call to printk() results in a DCC write that occurs on a different CPU.

No, we can't. The impression I got that Lauterbach is not responsive to feature requests like this one. And again, this problem really occurs with any tool that listens to DCC on an SMP system.