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

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

[Timur Tabi]

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 809920d..47654ea 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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);
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Timur Tabi]

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 2509d05..574da15 100644
--- a/drivers/tty/hvc/Kconfig
+++ b/drivers/tty/hvc/Kconfig
@@ -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
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Will Deacon]

On Sat, Aug 08, 2015 at 01:16:34AM +0100, Timur Tabi wrote:
> 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();

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

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

[Timur Tabi]

On 08/10/2015 04:40 AM, Will Deacon wrote:
>> >+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"?

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

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

[Dave Martin]

On Mon, Aug 17, 2015 at 06:56:10PM -0500, Timur Tabi wrote:
> On 08/10/2015 04:40 AM, Will Deacon wrote:
> >>>+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.

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

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

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

[Timur Tabi]

On 08/18/2015 03:21 AM, Dave Martin wrote:
>> 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.

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.

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

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

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

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

[Dave Martin]

On Tue, Aug 18, 2015 at 02:07:09PM -0500, Timur Tabi wrote:
> On 08/18/2015 03:21 AM, Dave Martin wrote:
> >>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.
> 
> 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.

[...]

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

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

[Timur Tabi]

On 08/19/2015 05:14 AM, Dave Martin wrote:
>> >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?

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

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

[Dave Martin]

On Wed, Aug 19, 2015 at 11:16:52AM -0500, Timur Tabi wrote:
> On 08/19/2015 05:14 AM, Dave Martin wrote:
> >>>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

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

[sboyd at codeaurora.org]

On 08/19, Dave Martin wrote:
> On Wed, Aug 19, 2015 at 11:16:52AM -0500, Timur Tabi wrote:
> > On 08/19/2015 05:14 AM, Dave Martin wrote:
> > >>>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.

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

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

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

[Dave Martin]

On Mon, Aug 24, 2015 at 04:51:44PM -0700, sboyd at codeaurora.org wrote:
> On 08/19, Dave Martin wrote:
> > On Wed, Aug 19, 2015 at 11:16:52AM -0500, Timur Tabi wrote:
> > > On 08/19/2015 05:14 AM, Dave Martin wrote:
> > > >>>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.
> 
> 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

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

[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 <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..a6496f2 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 volatile ("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 47654ea..e260acb 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -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);
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Will Deacon]

On Sat, Aug 08, 2015 at 01:16:35AM +0100, Timur Tabi wrote:
> 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..a6496f2 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 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?

> +		"	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 47654ea..e260acb 100644
> --- a/drivers/tty/hvc/hvc_dcc.c
> +++ b/drivers/tty/hvc/hvc_dcc.c
> @@ -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

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

[Timur Tabi]

On 08/10/2015 04:47 AM, Will Deacon wrote:
>> +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.

>> +	__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"

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

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

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

[Will Deacon]

On Sat, Aug 08, 2015 at 01:16:33AM +0100, 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 809920d..47654ea 100644
> --- a/drivers/tty/hvc/hvc_dcc.c
> +++ b/drivers/tty/hvc/hvc_dcc.c
> @@ -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

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

[Timur Tabi]

On 08/10/2015 04:48 AM, Will Deacon wrote:
>> >
>> >  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.

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.

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