linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes
@ 2015-06-30 21:23 Timur Tabi
  2015-06-30 21:23 ` [PATCH 2/4] hvc_dcc: don't ignore errors during initialization Timur Tabi
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Timur Tabi @ 2015-06-30 21:23 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, sboyd, Vipul Gandhi

From: Shanker Donthineni <shankerd@codeaurora.org>

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.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Acked-by: Adam Wallis <awallis@codeaurora.org>
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/tty/hvc/Kconfig   |  21 +++++++
 drivers/tty/hvc/hvc_dcc.c | 157 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 177 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 8902f9b..2c6883c 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -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
diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 809920d..33657dc 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -11,6 +11,10 @@
  */
 
 #include <linux/init.h>
+#include <linux/kfifo.h>
+#include <linux/spinlock.h>
+#include <linux/moduleparam.h>
+#include <linux/console.h>
 
 #include <asm/dcc.h>
 #include <asm/processor.h>
@@ -48,26 +52,177 @@ static int hvc_dcc_get_chars(uint32_t vt, char *buf, int count)
 	return i;
 }
 
+/*
+ * Check if the DCC is enabled.  If CONFIG_HVC_DCC_SERIALIZE_SMP is enabled,
+ * then we assume then this function will be called first on core 0.  That
+ * way, dcc_core0_available will be true only if it's available on core 0.
+ */
 static bool hvc_dcc_check(void)
 {
 	unsigned long time = jiffies + (HZ / 10);
 
+#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP
+	static bool dcc_core0_available;
+
+	/*
+	 * If we're not on core 0, but we previously confirmed that DCC is
+	 * active, then just return true.
+	 */
+	if (smp_processor_id() && dcc_core0_available)
+		return true;
+#endif
+
 	/* Write a test character to check if it is handled */
 	__dcc_putchar('\n');
 
 	while (time_is_after_jiffies(time)) {
-		if (!(__dcc_getstatus() & DCC_STATUS_TX))
+		if (!(__dcc_getstatus() & DCC_STATUS_TX)) {
+#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP
+			dcc_core0_available = true;
+#endif
 			return true;
+		}
 	}
 
 	return false;
 }
 
+#ifdef CONFIG_HVC_DCC_SERIALIZE_SMP
+
+static void dcc_put_work_fn(struct work_struct *work);
+static void dcc_get_work_fn(struct work_struct *work);
+static DECLARE_WORK(dcc_pwork, dcc_put_work_fn);
+static DECLARE_WORK(dcc_gwork, dcc_get_work_fn);
+static DEFINE_SPINLOCK(dcc_lock);
+static DEFINE_KFIFO(inbuf, unsigned char, 128);
+static DEFINE_KFIFO(outbuf, unsigned char, 1024);
+
+/*
+ * Workqueue function that writes the output FIFO to the DCC on core 0.
+ */
+static void dcc_put_work_fn(struct work_struct *work)
+{
+	unsigned char ch;
+
+	spin_lock(&dcc_lock);
+
+	/* While there's data in the output FIFO, write it to the DCC */
+	while (kfifo_get(&outbuf, &ch))
+		hvc_dcc_put_chars(0, &ch, 1);
+
+	/* While we're at it, check for any input characters */
+	while (!kfifo_is_full(&inbuf)) {
+		if (!hvc_dcc_get_chars(0, &ch, 1))
+			break;
+		kfifo_put(&inbuf, ch);
+	}
+
+	spin_unlock(&dcc_lock);
+}
+
+/*
+ * Workqueue function that reads characters from DCC and puts them into the
+ * input FIFO.
+ */
+static void dcc_get_work_fn(struct work_struct *work)
+{
+	unsigned char ch;
+
+	/*
+	 * Read characters from DCC and put them into the input FIFO, as
+	 * long as there is room and we have characters to read.
+	 */
+	spin_lock(&dcc_lock);
+
+	while (!kfifo_is_full(&inbuf)) {
+		if (!hvc_dcc_get_chars(0, &ch, 1))
+			break;
+		kfifo_put(&inbuf, ch);
+	}
+	spin_unlock(&dcc_lock);
+}
+
+/*
+ * Write characters directly to the DCC if we're on core 0 and the FIFO
+ * is empty, or write them to the FIFO if we're not.
+ */
+static int hvc_dcc0_put_chars(uint32_t vt, const char *buf,
+					     int count)
+{
+	int len;
+
+	spin_lock(&dcc_lock);
+	if (smp_processor_id() || (!kfifo_is_empty(&outbuf))) {
+		len = kfifo_in(&outbuf, buf, count);
+		spin_unlock(&dcc_lock);
+		/*
+		 * We just push data to the output FIFO, so schedule the
+		 * workqueue that will actually write that data to DCC.
+		 */
+		schedule_work_on(0, &dcc_pwork);
+		return len;
+	}
+
+	/*
+	 * If we're already on core 0, and the FIFO is empty, then just
+	 * write the data to DCC.
+	 */
+	len = hvc_dcc_put_chars(vt, buf, count);
+	spin_unlock(&dcc_lock);
+
+	return len;
+}
+
+/*
+ * Read characters directly from the DCC if we're on core 0 and the FIFO
+ * is empty, or read them from the FIFO if we're not.
+ */
+static int hvc_dcc0_get_chars(uint32_t vt, char *buf, int count)
+{
+	int len;
+
+	spin_lock(&dcc_lock);
+
+	if (smp_processor_id() || (!kfifo_is_empty(&inbuf))) {
+		len = kfifo_out(&inbuf, buf, count);
+		spin_unlock(&dcc_lock);
+
+		/*
+		 * If the FIFO was empty, there may be characters in the DCC
+		 * that we haven't read yet.  Schedule a workqueue to fill
+		 * the input FIFO, so that the next time this function is
+		 * called, we'll have data.
+		*/
+		if (!len)
+			schedule_work_on(0, &dcc_gwork);
+
+		return len;
+	}
+
+	/*
+	 * If we're already on core 0, and the FIFO is empty, then just
+	 * read the data from DCC.
+	 */
+	len = hvc_dcc_get_chars(vt, buf, count);
+	spin_unlock(&dcc_lock);
+
+	return len;
+}
+
+static const struct hv_ops hvc_dcc_get_put_ops = {
+	.get_chars = hvc_dcc0_get_chars,
+	.put_chars = hvc_dcc0_put_chars,
+};
+
+#else
+
 static const struct hv_ops hvc_dcc_get_put_ops = {
 	.get_chars = hvc_dcc_get_chars,
 	.put_chars = hvc_dcc_put_chars,
 };
 
+#endif
+
 static int __init hvc_dcc_console_init(void)
 {
 	if (!hvc_dcc_check())
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] hvc_dcc: don't ignore errors during initialization
  2015-06-30 21:23 [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Timur Tabi
@ 2015-06-30 21:23 ` Timur Tabi
  2015-07-01 23:54   ` Stephen Boyd
  2015-06-30 21:23 ` [PATCH 3/4] [v3] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Timur Tabi @ 2015-06-30 21:23 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, sboyd, Vipul Gandhi

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

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/tty/hvc/hvc_dcc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 33657dc..f8b8cf2 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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.


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] [v3] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
  2015-06-30 21:23 [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Timur Tabi
  2015-06-30 21:23 ` [PATCH 2/4] hvc_dcc: don't ignore errors during initialization Timur Tabi
@ 2015-06-30 21:23 ` Timur Tabi
  2015-06-30 21:23 ` [PATCH 4/4] hvc_dcc: disable user-space access to DCC Timur Tabi
  2015-07-01  1:37 ` [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Stephen Boyd
  3 siblings, 0 replies; 12+ messages in thread
From: Timur Tabi @ 2015-06-30 21:23 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, sboyd, Vipul Gandhi

From: Abhimanyu Kapur <abhimany@codeaurora.org>

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

Signed-off-by: Abhimanyu Kapur <abhimany@codeaurora.org>
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 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

diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
new file mode 100644
index 0000000..fcb8d7d
--- /dev/null
+++ b/arch/arm64/include/asm/dcc.h
@@ -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
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 2c6883c..9a60d18 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -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.


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] hvc_dcc: disable user-space access to DCC
  2015-06-30 21:23 [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Timur Tabi
  2015-06-30 21:23 ` [PATCH 2/4] hvc_dcc: don't ignore errors during initialization Timur Tabi
  2015-06-30 21:23 ` [PATCH 3/4] [v3] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
@ 2015-06-30 21:23 ` Timur Tabi
  2015-07-02  0:00   ` Stephen Boyd
  2015-07-01  1:37 ` [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Stephen Boyd
  3 siblings, 1 reply; 12+ messages in thread
From: Timur Tabi @ 2015-06-30 21:23 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, sboyd, Vipul Gandhi

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.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 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(+)

diff --git a/arch/arm/include/asm/dcc.h b/arch/arm/include/asm/dcc.h
index b74899d..c50056b 100644
--- a/arch/arm/include/asm/dcc.h
+++ b/arch/arm/include/asm/dcc.h
@@ -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
diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
index fcb8d7d..7843e62 100644
--- a/arch/arm64/include/asm/dcc.h
+++ b/arch/arm64/include/asm/dcc.h
@@ -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
diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index f8b8cf2..5ccafa4 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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.


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes
  2015-06-30 21:23 [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Timur Tabi
                   ` (2 preceding siblings ...)
  2015-06-30 21:23 ` [PATCH 4/4] hvc_dcc: disable user-space access to DCC Timur Tabi
@ 2015-07-01  1:37 ` Stephen Boyd
  2015-07-01  1:57   ` Timur Tabi
  3 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2015-07-01  1:37 UTC (permalink / raw)
  To: Timur Tabi, linux-arm-kernel, linux-kernel, Shanker Donthineni,
	awallis, abhimany, will.deacon, Vipul Gandhi

On 06/30/2015 02:23 PM, Timur Tabi wrote:
> From: Shanker Donthineni <shankerd@codeaurora.org>
>
> 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.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> Acked-by: Adam Wallis <awallis@codeaurora.org>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes
  2015-07-01  1:37 ` [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Stephen Boyd
@ 2015-07-01  1:57   ` Timur Tabi
  2015-07-01 23:38     ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Timur Tabi @ 2015-07-01  1:57 UTC (permalink / raw)
  To: Stephen Boyd, linux-arm-kernel, linux-kernel, Shanker Donthineni,
	awallis, abhimany, will.deacon, Vipul Gandhi

Stephen Boyd wrote:
> 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.

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes
  2015-07-01  1:57   ` Timur Tabi
@ 2015-07-01 23:38     ` Stephen Boyd
  2015-07-03 14:42       ` Timur Tabi
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2015-07-01 23:38 UTC (permalink / raw)
  To: Timur Tabi, linux-arm-kernel, linux-kernel, Shanker Donthineni,
	awallis, abhimany, will.deacon, Vipul Gandhi

On 06/30/2015 06:57 PM, Timur Tabi wrote:
> Stephen Boyd wrote:
>> 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.
>
> 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.

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.

> 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.

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.

>
> Does CPU hotplug automatically take CPUs offline when the load is low?
> If so, then then thread could randomly bounce from CPU to 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] hvc_dcc: don't ignore errors during initialization
  2015-06-30 21:23 ` [PATCH 2/4] hvc_dcc: don't ignore errors during initialization Timur Tabi
@ 2015-07-01 23:54   ` Stephen Boyd
  2015-07-03 14:46     ` Timur Tabi
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2015-07-01 23:54 UTC (permalink / raw)
  To: Timur Tabi, linux-arm-kernel, linux-kernel, Shanker Donthineni,
	awallis, abhimany, will.deacon, Vipul Gandhi

On 06/30/2015 02:23 PM, Timur Tabi wrote:
> hvc_instantiate() and hvc_alloc() return errors if they fail, so don't
> ignore them.
>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/tty/hvc/hvc_dcc.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
> index 33657dc..f8b8cf2 100644
> --- a/drivers/tty/hvc/hvc_dcc.c
> +++ b/drivers/tty/hvc/hvc_dcc.c
> @@ -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;

Why not just return the value that hvc_instantiate returns? And maybe we
should change those -1 in hvc_instantiate into -EPERM?

>  }
>  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;

return PTR_ERR_OR_ZERO()?

>  }
>  device_initcall(hvc_dcc_init);


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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] hvc_dcc: disable user-space access to DCC
  2015-06-30 21:23 ` [PATCH 4/4] hvc_dcc: disable user-space access to DCC Timur Tabi
@ 2015-07-02  0:00   ` Stephen Boyd
  2015-07-03 14:53     ` Timur Tabi
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2015-07-02  0:00 UTC (permalink / raw)
  To: Timur Tabi, linux-arm-kernel, linux-kernel, Shanker Donthineni,
	awallis, abhimany, will.deacon, Vipul Gandhi

On 06/30/2015 02:23 PM, Timur Tabi wrote:
> diff --git a/arch/arm/include/asm/dcc.h b/arch/arm/include/asm/dcc.h
> index b74899d..c50056b 100644
> --- a/arch/arm/include/asm/dcc.h
> +++ b/arch/arm/include/asm/dcc.h
> @@ -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>

That's new.

>  
>  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);
> +}

Nitpick: We do raw cp14 accesses for other functions in this file, so it
would be nice to be consistent throughout.

> +
> +#endif
> diff --git a/arch/arm64/include/asm/dcc.h b/arch/arm64/include/asm/dcc.h
> index fcb8d7d..7843e62 100644
> --- a/arch/arm64/include/asm/dcc.h
> +++ b/arch/arm64/include/asm/dcc.h
> @@ -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"

shouldn't this be volatile?

> +		"	orr %0, %0, #4096\n" /* Set the TDCC bit */

arm64 has some #defines in debug-monitors.h which we should probably
extend to have this bit as a #define too.

> +		"	msr mdscr_el1, %0\n"
> +		: "=r" (val));
> +}
> +
>  #endif
>

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes
  2015-07-01 23:38     ` Stephen Boyd
@ 2015-07-03 14:42       ` Timur Tabi
  0 siblings, 0 replies; 12+ messages in thread
From: Timur Tabi @ 2015-07-03 14:42 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, Vipul Gandhi

On Jul 1, 2015, at 7:38 PM, Stephen Boyd wrote:

> 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.

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?"

> 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.

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.

>> 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.
> 
> 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.

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?

>> Does CPU hotplug automatically take CPUs offline when the load is low?
>> If so, then then thread could randomly bounce from CPU to 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.

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.

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

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] hvc_dcc: don't ignore errors during initialization
  2015-07-01 23:54   ` Stephen Boyd
@ 2015-07-03 14:46     ` Timur Tabi
  0 siblings, 0 replies; 12+ messages in thread
From: Timur Tabi @ 2015-07-03 14:46 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, Vipul Gandhi

On Jul 1, 2015, at 7:54 PM, Stephen Boyd wrote:

>> 
>> +	/* Returns -1 if error */
>> +	ret = hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
>> +
>> +	return ret < 0 ? -ENODEV : 0;
> 
> Why not just return the value that hvc_instantiate returns? And maybe we
> should change those -1 in hvc_instantiate into -EPERM?

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.

>> 
>> -	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;
> 
> return PTR_ERR_OR_ZERO()?

Yes, thanks.  I'll change it.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] hvc_dcc: disable user-space access to DCC
  2015-07-02  0:00   ` Stephen Boyd
@ 2015-07-03 14:53     ` Timur Tabi
  0 siblings, 0 replies; 12+ messages in thread
From: Timur Tabi @ 2015-07-03 14:53 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-arm-kernel, linux-kernel, Shanker Donthineni, awallis,
	abhimany, will.deacon, Vipul Gandhi


On Jul 1, 2015, at 8:00 PM, Stephen Boyd wrote:

>> 
>> #include <asm/barrier.h>
>> +#include <asm/hardware/cp14.h>
> 
> That's new.

And no one else is using it either.

>> 
>> +
>> +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);
>> +}
> 
> Nitpick: We do raw cp14 accesses for other functions in this file, so it
> would be nice to be consistent throughout.

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

>> 
>> +static inline void __dcc_initialize(void)
>> +{
>> +	u32 val;
>> +
>> +	/* Disable user-space access to DCC */
>> +	asm ("mrs %0, mdscr_el1\n"
> 
> shouldn't this be volatile?

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.

> 
>> +		"	orr %0, %0, #4096\n" /* Set the TDCC bit */
> 
> arm64 has some #defines in debug-monitors.h which we should probably
> extend to have this bit as a #define too.

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-07-03 14:53 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-30 21:23 [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Timur Tabi
2015-06-30 21:23 ` [PATCH 2/4] hvc_dcc: don't ignore errors during initialization Timur Tabi
2015-07-01 23:54   ` Stephen Boyd
2015-07-03 14:46     ` Timur Tabi
2015-06-30 21:23 ` [PATCH 3/4] [v3] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
2015-06-30 21:23 ` [PATCH 4/4] hvc_dcc: disable user-space access to DCC Timur Tabi
2015-07-02  0:00   ` Stephen Boyd
2015-07-03 14:53     ` Timur Tabi
2015-07-01  1:37 ` [PATCH 1/4] hvc_dcc: bind driver to core0 for reads and writes Stephen Boyd
2015-07-01  1:57   ` Timur Tabi
2015-07-01 23:38     ` Stephen Boyd
2015-07-03 14:42       ` Timur Tabi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).