hvc_dcc: don't ignore errors during initialization

PATCH Superseded Timur Tabi 4 participants 16 messages Aug 8, 2015 – Sep 2, 2015

Subsystems (4)

Arm (32-bit) arm64 TTY layer and serial drivers Hypervisor virtual console driver

lore

Share & tools

lore

Timur Tabi

author

2015-08-08 00:16 UTC #0 lore raw

PATCH v2 1/3 hvc_dcc: don't ignore errors during initialization

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

Signed-off-by Timur Tabi <[email protected]>

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

drivers/tty/hvc/hvc_dcc.c +13 -4

index 809920d..47654ea 100644
@@ -70,20 +70,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 PTR_ERR_OR_ZERO(p);
 }
 device_initcall(hvc_dcc_init);

Signature · 3 lines

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

Timur Tabi

author

2015-08-08 00:16 UTC #1 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

From: Abhimanyu Kapur <[email protected]>

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

Signed-off-by Abhimanyu Kapur <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

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

arch/arm64/include/asm/dcc.h new file +52 -0

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

drivers/tty/hvc/Kconfig +1 -1

index 2509d05..574da15 100644
@@ -81,7 +81,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

Signature · 3 lines

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

Will Deacon

2015-08-10 09:40 UTC #2 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 58 lines · Timur Tabi

From: Abhimanyu Kapur <[email protected]>

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

Signed-off-by Abhimanyu Kapur <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

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

arch/arm64/include/asm/dcc.h new file +39 -0

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();

arch/arm64/include/asm/dcc.h line 49

Quoted 10 lines

arch/arm64/include/asm/dcc.h new file +10 -0

+
+	return c;
+}
+
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));
+	isb();

I think we should be masking out the upper bits of c before the msr (the compiler probably expects a uxtb).

Other than that, this looks ok.

Will

Timur Tabi

author

2015-08-17 23:56 UTC #3 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 9 lines · Will Deacon

Quoted 6 lines
arch/arm64/include/asm/dcc.h line 49
Quoted 6 lines
arch/arm64/include/asm/dcc.h new file +6 -0
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));
+	isb();
I think we should be masking out the upper bits of c before the msr (the compiler probably expects a uxtb).

Well, we've never seen a problem, but that doesn't mean it doesn't exist. I couldn't find anything in the ARMv8 ARM (section H9.2.7 DBGDTRTX_EL0) about word sizes.

Do you think that I need an explicit instruction to clear the upper bits? I tried a few compiler tricks (e.g. "c && 0xff" and the like), and they had no effect.

I do need help with the inline assembly. I tried this:

static inline void __dcc_putchar(char c)

{
	unsigned int __c;

	asm volatile("uxtb %0, %w1\n"
		"msr dbgdtrtx_el0, %0"
			: "=r" (__c)
			: "r" (c));
	isb();
}

it gives this assembly code:

28: 38401423 ldrb w3, [x1],#1
2c: 53001c63 uxtb w3, w3
30: d5130503 msr dbgdtrrx_el0, x3

Is this correct? Shouldn't it be "uxtb x3, w3"?

Signature · 3 lines

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

Dave Martin

2015-08-18 08:21 UTC #4 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 11 lines · Timur Tabi

Quoted 9 lines · Will Deacon
Quoted 6 lines
arch/arm64/include/asm/dcc.h line 49
Quoted 6 lines
arch/arm64/include/asm/dcc.h new file +6 -0
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));
+	isb();
I think we should be masking out the upper bits of c before the msr (the compiler probably expects a uxtb).

Quoted 7 lines

Well, we've never seen a problem, but that doesn't mean it doesn't exist. I couldn't find anything in the ARMv8 ARM (section H9.2.7 DBGDTRTX_EL0) about word sizes.
Do you think that I need an explicit instruction to clear the upper bits? I tried a few compiler tricks (e.g. "c && 0xff" and the like), and they had no effect.

The in-register representation of a char permits the upper bits to be nonzero, so you need to convert to a register-sized type if you want to be able to force those bits to zero.

Try: (unsigned long)(unsigned char)c or (unsigned long)c & 0xff.

Quoted 14 lines

I do need help with the inline assembly. I tried this:
static inline void __dcc_putchar(char c)
{
	unsigned int __c;

	asm volatile("uxtb %0, %w1\n"
		"msr dbgdtrtx_el0, %0"
			: "=r" (__c)
			: "r" (c));
	isb();
}

Quoted 7 lines

it gives this assembly code:
28: 38401423 ldrb w3, [x1],#1
2c: 53001c63 uxtb w3, w3
30: d5130503 msr dbgdtrrx_el0, x3
Is this correct? Shouldn't it be "uxtb x3, w3"?

Check the ARM ARM for what operand combinations are allowed. However, it doesn't really make any difference here because it's a general rule in the architecture that when an instruction's output is in a W-register, the upper 32 bits of the corresponding X-register are always zeroed anyway.

Cheers
---Dave

Timur Tabi

author

2015-08-18 19:07 UTC #5 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 8 lines · Dave Martin

Quoted 3 lines
Do you think that I need an explicit instruction to clear the upper
Quoted 2 lines
bits? I tried a few compiler tricks (e.g. "c && 0xff" and the like), and they had no effect.
The in-register representation of a char permits the upper bits to be nonzero, so you need to convert to a register-sized type if you want to be able to force those bits to zero.
Try: (unsigned long)(unsigned char)c or (unsigned long)c & 0xff.

I tried all those, and more, and I still always get the same thing:

28: 38401423 ldrb w3, [x1],#1
2c: d5130503 msr dbgdtrrx_el0, x3

I know that ldrb will zero-extend the byte to a 32-bit word. But I don't see any way to zero-extend the word into a 64-bit register.

I'm not even sure that this is necessary. The dbgdtrrx_el0 register is technically only 32-bit anyway. It looks to me like the code is already correct.

I could change the (c) to "(c && 0xff)" to be extra sure, but I can't verify that it actually works.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
it gives this assembly code:
28: 38401423 ldrb w3, [x1],#1
2c: 53001c63 uxtb w3, w3
30: d5130503 msr dbgdtrrx_el0, x3

Quoted 6 lines

Quoted 1 line
Quoted 1 line
Is this correct? Shouldn't it be "uxtb x3, w3"?
Check the ARM ARM for what operand combinations are allowed. However, it doesn't really make any difference here because it's a general rule in the architecture that when an instruction's output is in a W-register, the upper 32 bits of the corresponding X-register are always zeroed anyway.

So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?

Signature · 3 lines

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

Dave Martin

2015-08-19 10:14 UTC #6 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 21 lines · Timur Tabi

Quoted 8 lines · Dave Martin
Quoted 3 lines
Do you think that I need an explicit instruction to clear the upper
Quoted 2 lines
bits? I tried a few compiler tricks (e.g. "c && 0xff" and the like), and they had no effect.
The in-register representation of a char permits the upper bits to be nonzero, so you need to convert to a register-sized type if you want to be able to force those bits to zero.
Try: (unsigned long)(unsigned char)c or (unsigned long)c & 0xff.
I tried all those, and more, and I still always get the same thing:
28: 38401423 ldrb w3, [x1],#1
2c: d5130503 msr dbgdtrrx_el0, x3
I know that ldrb will zero-extend the byte to a 32-bit word. But I don't see any way to zero-extend the word into a 64-bit register.
I'm not even sure that this is necessary. The dbgdtrrx_el0 register is technically only 32-bit anyway. It looks to me like the code is already correct.

[...]

Quoted 7 lines

Quoted 5 lines
Check the ARM ARM for what operand combinations are allowed. However, it doesn't really make any difference here because it's a general rule in the architecture that when an instruction's output is in a W-register, the upper 32 bits of the corresponding X-register are always zeroed anyway.
So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?

Yes. No extra operation is required.

Cheers
---Dave

Timur Tabi

author

2015-08-19 16:16 UTC #7 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 2 lines · Dave Martin

Quoted 1 line
Quoted 1 line
So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?
Yes. No extra operation is required.

So my patch is actually correct as-is?

Signature · 3 lines

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

Dave Martin

2015-08-19 16:37 UTC #8 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 5 lines · Timur Tabi

Quoted 2 lines · Dave Martin
Quoted 1 line
Quoted 1 line
So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?
Yes. No extra operation is required.
So my patch is actually correct as-is?

+static inline void __dcc_putchar(char c)
+{
+ asm volatile("msr dbgdtrtx_el0, %0"
+ : /* No output register */
+ : "r" (c));

For safety, you still need to make sure that c is appropriately masked before passing it to the asm. Something like this should definitely be safe:

: "r" ((unsigned long)(unsigned char)c)

GCC can then emit uxtb or not, depending on whether it's needed in each context where the asm is inlined.

Cheers
---Dave

sboyd at codeaurora.org

2015-08-24 23:51 UTC #9 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 13 lines · Dave Martin

Quoted 5 lines · Timur Tabi
Quoted 2 lines · Dave Martin
Quoted 1 line
Quoted 1 line
So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?
Yes. No extra operation is required.
So my patch is actually correct as-is?
(quoted diff) +5 -0
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));

Quoted 8 lines

For safety, you still need to make sure that c is appropriately masked before passing it to the asm. Something like this should definitely be safe:
: "r" ((unsigned long)(unsigned char)c)
GCC can then emit uxtb or not, depending on whether it's needed in each context where the asm is inlined.

Does this mean we ought to do the same thing in the arm header file too?

Signature · 2 lines

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

Dave Martin

2015-09-02 11:10 UTC #10 lore raw

PATCH v4 2/3 ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

Quoted 18 lines · sboyd at codeaurora.org

Quoted 17 lines · Dave Martin
Quoted 5 lines · Timur Tabi
Quoted 2 lines · Dave Martin
Quoted 1 line
Quoted 1 line
So does that mean that ldrb will zero-extend the byte to all 64 bits of x3?
Yes. No extra operation is required.
So my patch is actually correct as-is?
(quoted diff) +5 -0
+static inline void __dcc_putchar(char c)
+{
+	asm volatile("msr dbgdtrtx_el0, %0"
+			: /* No output register */
+			: "r" (c));
For safety, you still need to make sure that c is appropriately masked before passing it to the asm. Something like this should definitely be safe:

Quoted 7 lines

Quoted 4 lines
: "r" ((unsigned long)(unsigned char)c)
GCC can then emit uxtb or not, depending on whether it's needed in each context where the asm is inlined.
Does this mean we ought to do the same thing in the arm header file too?

Probably.

Even though current CPUs don't place special meaning on the upper bits, it's possible that future CPUs will.

Cheers
---Dave

Timur Tabi

author

2015-08-08 00:16 UTC #11 lore raw

PATCH v2 3/3 hvc_dcc: disable user-space access to DCC

In reply to Timur Tabi

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 <[email protected]>

---
 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(+)

arch/arm/include/asm/dcc.h +15 -0

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

arch/arm64/include/asm/dcc.h +11 -0

index fcb8d7d..a6496f2 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 volatile ("mrs %0, mdscr_el1\n"
+		"	orr %0, %0, #4096\n" /* Set the TDCC bit */
+		"	msr mdscr_el1, %0\n"
+		: "=r" (val));
+}
+
 #endif

drivers/tty/hvc/hvc_dcc.c +2 -0

index 47654ea..e260acb 100644
@@ -76,6 +76,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);

Signature · 3 lines

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

Will Deacon

2015-08-10 09:47 UTC #12 lore raw

PATCH v2 3/3 hvc_dcc: disable user-space access to DCC

Quoted 47 lines · Timur Tabi

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 <[email protected]>

---
 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(+)

arch/arm/include/asm/dcc.h +15 -0

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

arch/arm64/include/asm/dcc.h +0 -0

index fcb8d7d..a6496f2 100644

arch/arm64/include/asm/dcc.h static inline void __dcc_putchar(char c) · line 58

Quoted 10 lines

arch/arm64/include/asm/dcc.h +7 -0

@@ -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 volatile ("mrs %0, mdscr_el1\n"
+		"	orr %0, %0, #4096\n" /* Set the TDCC bit */

So this is the same as your "1 << 12" for arch/arm/. Shouldn't we #define that someplace common?

Quoted 10 lines

arch/arm64/include/asm/dcc.h +4 -0
+		"	msr mdscr_el1, %0\n"
+		: "=r" (val));
+}
+
 #endif
drivers/tty/hvc/hvc_dcc.c +0 -0
index 47654ea..e260acb 100644

drivers/tty/hvc/hvc_dcc.c static int __init hvc_dcc_console_init(void) · line 82

Quoted 7 lines

drivers/tty/hvc/hvc_dcc.c +2 -0

@@ -76,6 +76,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);

Can this driver be built as a module and unloaded? If so, should we re-enable userspace access when the kernel doesn't need it anymore?

Finally, have you checked the behaviour on CPU hotplug? It looks like we zero mdscr_el1 in the cold boot path. The alternative is to set this bit there and never allow userspace access. Do you know of any compelling use-cases where userspace accesses the DCC directly?

Will

Timur Tabi

author

2015-08-17 22:45 UTC #13 lore raw

PATCH v2 3/3 hvc_dcc: disable user-space access to DCC

Quoted 10 lines · Will Deacon

arch/arm64/include/asm/dcc.h static inline void __dcc_putchar(char c) · line 58
Quoted 7 lines
arch/arm64/include/asm/dcc.h +7 -0
+static inline void __dcc_initialize(void)
+{
+	u32 val;
+
+	/* Disable user-space access to DCC */
+	asm volatile ("mrs %0, mdscr_el1\n"
+		"	orr %0, %0, #4096\n" /* Set the TDCC bit */
So this is the same as your "1 << 12" for arch/arm/. Shouldn't we #define that someplace common?

Well, I'm not sure. First, there is no common place currently available. I would need to create a new header file that only has one line in it.

Secondly, DBGDSCR in ARMv7 and MDSCR_EL1 on ARMv8 don't have that much in common. Those two registers probably serve the same function, but only a few bits are the same.

I've documented the code. I can clean it up so that it looks like the same.

Quoted 7 lines

drivers/tty/hvc/hvc_dcc.c static int __init hvc_dcc_console_init(void) · line 82
Quoted 4 lines
drivers/tty/hvc/hvc_dcc.c +2 -0
+	__dcc_initialize();
+
  	/* Returns -1 if error */
  	ret = hvc_instantiate(0, 0, &hvc_dcc_get_put_ops);
Can this driver be built as a module and unloaded? If so, should we re-enable userspace access when the kernel doesn't need it anymore?

Not according to the Kconfig:

config HVC_DCC
bool "ARM JTAG DCC console"

Quoted 4 lines

Finally, have you checked the behaviour on CPU hotplug? It looks like we zero mdscr_el1 in the cold boot path. The alternative is to set this bit there and never allow userspace access. Do you know of any compelling use-cases where userspace accesses the DCC directly?

Maybe KVM?

I have to admit, this is starting to get a little out of my league. All I really wanted to do was make DCC available on ARM64, the same exact way it's available on ARM32. I don't want to have to solve every DCC problem that exists today on every ARM platform. I would rather drop this patch than have to spend an inordinate amount of time making it perfect.

We've never disabled user-space DCC support on any ARM platform before.
If there is a problem with user-space DCC, I've never seen it, and I
don't know anyone who has.

Signature · 3 lines

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

Will Deacon

2015-08-10 09:48 UTC #14 lore raw

PATCH v2 1/3 hvc_dcc: don't ignore errors during initialization

In reply to Timur Tabi

Quoted 13 lines · Timur Tabi

hvc_instantiate() and hvc_alloc() return errors if they fail, so don't ignore them.
Signed-off-by Timur Tabi <[email protected]>
---
 drivers/tty/hvc/hvc_dcc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/tty/hvc/hvc_dcc.c +0 -0
index 809920d..47654ea 100644

drivers/tty/hvc/hvc_dcc.c static const struct hv_ops hvc_dcc_get_put_ops = { · line 75

Quoted 6 lines

drivers/tty/hvc/hvc_dcc.c +3 -0

@@ -70,20 +70,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 */

Does this really "always run on the boot core"? I couldn't find anything suggesting that console_init is limited in that regard.

Will

Timur Tabi

author

2015-08-19 22:51 UTC #15 lore raw

PATCH v2 1/3 hvc_dcc: don't ignore errors during initialization

Quoted 8 lines · Will Deacon

Quoted 6 lines
drivers/tty/hvc/hvc_dcc.c static const struct hv_ops hvc_dcc_get_put_ops = { · line 75
Quoted 6 lines
static int __init hvc_dcc_console_init(void)
{
drivers/tty/hvc/hvc_dcc.c +3 -0
+	int ret;
+
+	/* This always runs on boot core */
Does this really "always run on the boot core"? I couldn't find anything suggesting that console_init is limited in that regard.

So this is a left-over comment from some other code that actually cared what code this function runs on. I've removed the comment.

Signature · 3 lines

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