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

[PATCH 1/3] [v3] 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/hvc/hvc_dcc.c b/drivers/tty/hvc/hvc_dcc.c
index 809920d..82f240f 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -70,20 +70,27 @@ static const struct hv_ops hvc_dcc_get_put_ops = {
 
 static int __init hvc_dcc_console_init(void)
 {
+	int ret;
+
 	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;
+
 	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 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 65e0190..cff9512 100644
--- a/arch/arm64/include/asm/dcc.h
+++ b/arch/arm64/include/asm/dcc.h
@@ -52,4 +52,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 82f240f..14d4a473 100644
--- a/drivers/tty/hvc/hvc_dcc.c
+++ b/drivers/tty/hvc/hvc_dcc.c
@@ -75,6 +75,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 Thu, Aug 20, 2015 at 12:02:50AM +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.

In the interests of making some headway with this, just drop this patch
and I'll queue the diff below via the arm64 tree.

Will

--->8

diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 39139a3aa16d..4c922ecce600 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -156,7 +156,7 @@ ENTRY(__cpu_setup)
 
 	mov	x0, #3 << 20
 	msr	cpacr_el1, x0			// Enable FP/ASIMD
-	msr	mdscr_el1, xzr			// Reset mdscr_el1
+	msr	mdscr_el1, #1 << 12		// Reset mdscr_el1
 	/*
 	 * Memory region attributes for LPAE:
 	 *

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

[Catalin Marinas]

On Thu, Aug 20, 2015 at 11:46:45AM +0100, Will Deacon wrote:
> On Thu, Aug 20, 2015 at 12:02:50AM +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.
> 
> In the interests of making some headway with this, just drop this patch
> and I'll queue the diff below via the arm64 tree.
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 39139a3aa16d..4c922ecce600 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -156,7 +156,7 @@ ENTRY(__cpu_setup)
>  
>  	mov	x0, #3 << 20
>  	msr	cpacr_el1, x0			// Enable FP/ASIMD
> -	msr	mdscr_el1, xzr			// Reset mdscr_el1
> +	msr	mdscr_el1, #1 << 12		// Reset mdscr_el1

Nitpick: expand the "Reset..." comment to include some text about this
bit.

-- 
Catalin

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

[Timur Tabi]

On 08/20/2015 05:46 AM, Will Deacon 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.

> In the interests of making some headway with this, just drop this patch
> and I'll queue the diff below via the arm64 tree.

Yes, that would be great.  Thank you.

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