[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[PATCH] 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 | 49 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/tty/hvc/Kconfig      |  2 +-
 2 files changed, 50 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..f038d61
--- /dev/null
+++ b/arch/arm64/include/asm/dcc.h
@@ -0,0 +1,49 @@
+/* Copyright (c) 2014 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.
+ */
+
+#include <asm/barrier.h>
+
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
+			: : "cc");
+
+	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();
+}
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.

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Will Deacon]

On Fri, Jun 19, 2015 at 11:08:54PM +0100, Timur Tabi wrote:
> From: Abhimanyu Kapur <abhimany@codeaurora.org>
> 
> Add support for debug communications channel based
> hvc console for arm64 cpus.

I still think we should be disabling userspace access to the DCC if the
kernel is using it as its console.

> Signed-off-by: Abhimanyu Kapur <abhimany@codeaurora.org>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  arch/arm64/include/asm/dcc.h | 49 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/tty/hvc/Kconfig      |  2 +-
>  2 files changed, 50 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..f038d61
> --- /dev/null
> +++ b/arch/arm64/include/asm/dcc.h
> @@ -0,0 +1,49 @@
> +/* Copyright (c) 2014 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.
> + */

Missing header guards.

> +#include <asm/barrier.h>
> +
> +static inline u32 __dcc_getstatus(void)
> +{
> +	u32 __ret;
> +
> +	asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
> +			: : "cc");

You don't need the "cc" clobber.

Will

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Timur Tabi]

Will Deacon wrote:
> On Fri, Jun 19, 2015 at 11:08:54PM +0100, Timur Tabi wrote:
>> From: Abhimanyu Kapur <abhimany@codeaurora.org>
>>
>> Add support for debug communications channel based
>> hvc console for arm64 cpus.
>
> I still think we should be disabling userspace access to the DCC if the
> kernel is using it as its console.

I don't disagree, I just don't know how to do that.

>> + * 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.
>> + */
>
> Missing header guards.

Will fix.

>> +#include <asm/barrier.h>
>> +
>> +static inline u32 __dcc_getstatus(void)
>> +{
>> +	u32 __ret;
>> +
>> +	asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
>> +			: : "cc");
>
> You don't need the "cc" clobber.

Will fix.

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

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Timur Tabi]

On 06/22/2015 08:12 AM, Will Deacon wrote:
> I still think we should be disabling userspace access to the DCC if the
> kernel is using it as its console.

I still need help with this.  I know you said a year ago that 
MDSCR_EL1.TDCC needs to be set to disable userspace access.  Where and 
how should I do this?  I can do this:

static int __init hvc_dcc_console_init(void)
{
#ifdef CONFIG_ARM64
	u32 val;

	asm("msr mdscr_el1, %0	"
	"orr %0, %0, #4096	" /* TDCC */
	"msr %0, mdscr_el1	"
	: "=r" (val));
#endif

But this seems clunky.

I am concerned about KVM, though.  There appears to be code in KVM in 
hyp.s and sys_regs.c that touches and/or emulates MDSCR_EL1.

On a side note, it does not appear that ARM32 blocks userspace DCC.  I 
don't see where DBGDSCR.UDCCdis is set.

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

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Will Deacon]

On Wed, Jun 24, 2015 at 09:11:24PM +0100, Timur Tabi wrote:
> On 06/22/2015 08:12 AM, Will Deacon wrote:
> > I still think we should be disabling userspace access to the DCC if the
> > kernel is using it as its console.
> 
> I still need help with this.  I know you said a year ago that 
> MDSCR_EL1.TDCC needs to be set to disable userspace access.  Where and 
> how should I do this?  I can do this:

Well, it's up to you to figure out the details, but I'd start by adding
some static inlines to the arch-specific header files for enabling/disabling
userspace access.

>From there, I think I'd get the architecture init code to reset the thing
to "disabled" (so it's disabled regardless of whether we build the hvc_dcc
driver) and then if you wanted to go all-out, we could have a sysfs entry
provided by the driver to toggle it on and off.

> static int __init hvc_dcc_console_init(void)
> {
> #ifdef CONFIG_ARM64
> 	u32 val;
> 
> 	asm("msr mdscr_el1, %0	"
> 	"orr %0, %0, #4096	" /* TDCC */
> 	"msr %0, mdscr_el1	"
> 	: "=r" (val));
> #endif
> 
> But this seems clunky.

Yeah, that's super ugly.

> I am concerned about KVM, though.  There appears to be code in KVM in 
> hyp.s and sys_regs.c that touches and/or emulates MDSCR_EL1.
> 
> On a side note, it does not appear that ARM32 blocks userspace DCC.  I 
> don't see where DBGDSCR.UDCCdis is set.

That's a bug imo.

Will

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Timur Tabi]

Will Deacon wrote:
> Well, it's up to you to figure out the details, but I'd start by adding
> some static inlines to the arch-specific header files for enabling/disabling
> userspace access.
>
>  From there, I think I'd get the architecture init code to reset the thing
> to "disabled" (so it's disabled regardless of whether we build the hvc_dcc
> driver) and then if you wanted to go all-out, we could have a sysfs entry
> provided by the driver to toggle it on and off.
>
>> >static int __init hvc_dcc_console_init(void)
>> >{
>> >#ifdef CONFIG_ARM64
>> >	u32 val;
>> >
>> >	asm("msr mdscr_el1, %0	"
>> >	"orr %0, %0, #4096	" /* TDCC */
>> >	"msr %0, mdscr_el1	"
>> >	: "=r" (val));
>> >#endif
>> >
>> >But this seems clunky.
> Yeah, that's super ugly.
>
>> >I am concerned about KVM, though.  There appears to be code in KVM in
>> >hyp.s and sys_regs.c that touches and/or emulates MDSCR_EL1.
>> >
>> >On a side note, it does not appear that ARM32 blocks userspace DCC.  I
>> >don't see where DBGDSCR.UDCCdis is set.
> That's a bug imo.

So wouldn't it be more appropriate to have a separate patch that handles 
disabling of user-space DCC for ARM32 and ARM64?  All I really want to 
do at this point is provide basic DCC support for ARM64, just like we 
have for ARM32.

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

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Abhimanyu Kapur]

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

Signed-off-by: Abhimanyu Kapur <abhimany@codeaurora.org>
---
 arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
 drivers/tty/hvc/Kconfig      |  2 +-
 2 files changed, 42 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..ef74324
--- /dev/null
+++ b/arch/arm64/include/asm/dcc.h
@@ -0,0 +1,41 @@
+/* Copyright (c) 2014 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.
+ */
+
+#include <asm/barrier.h>
+
+static inline u32 __dcc_getstatus(void)
+{
+	u32 __ret;
+
+	asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
+			: : "cc");
+
+	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();
+}
diff --git a/drivers/tty/hvc/Kconfig b/drivers/tty/hvc/Kconfig
index 8902f9b..02051a1 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
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Will Deacon]

On Mon, Jun 16, 2014 at 11:29:38PM +0100, Abhimanyu Kapur wrote:
> Add support for debug communications channel based
> hvc console for arm64 cpus.

Should we be setting MDSCR_EL1.TDCC to prevent userspace access to the DCC?

> Signed-off-by: Abhimanyu Kapur <abhimany@codeaurora.org>
> ---
>  arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/tty/hvc/Kconfig      |  2 +-
>  2 files changed, 42 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..ef74324
> --- /dev/null
> +++ b/arch/arm64/include/asm/dcc.h
> @@ -0,0 +1,41 @@
> +/* Copyright (c) 2014 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.
> + */
> +
> +#include <asm/barrier.h>
> +
> +static inline u32 __dcc_getstatus(void)
> +{
> +	u32 __ret;

Can this result in the mrs receiving a W register?

> +	asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
> +			: : "cc");

Why the CC clobber? Why volatile?

> +
> +	return __ret;
> +}
> +
> +static inline char __dcc_getchar(void)
> +{
> +	char __c;
> +
> +	asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (__c));
> +	isb();

Why the isb and why volatile??

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

Can you guarantee that GCC hasn't put junk in the upper bits of c?

> +	isb();

Why the isb?

Will

[PATCH] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc

[Timur Tabi]

On Tue, Jun 17, 2014 at 4:51 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Mon, Jun 16, 2014 at 11:29:38PM +0100, Abhimanyu Kapur wrote:
>> Add support for debug communications channel based
>> hvc console for arm64 cpus.

I know it's been a year since this patch was posted, but I'm working
on re-submitting it.  Unfortunately, I can't answer most of your
questions, but I want to try to fix this patch up and get it accepted.

> Should we be setting MDSCR_EL1.TDCC to prevent userspace access to the DCC?

I personally don't know enough about ARM to answer that question, but
it does makes sense that we don't want userspace to access the DCC
registers.  Where would MDSCR_EL1.TDCC be set?  Should we add it to
hvc_dcc_init()?

>> Signed-off-by: Abhimanyu Kapur <abhimany@codeaurora.org>
>> ---
>>  arch/arm64/include/asm/dcc.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>  drivers/tty/hvc/Kconfig      |  2 +-
>>  2 files changed, 42 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..ef74324
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/dcc.h
>> @@ -0,0 +1,41 @@
>> +/* Copyright (c) 2014 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.
>> + */
>> +
>> +#include <asm/barrier.h>
>> +
>> +static inline u32 __dcc_getstatus(void)
>> +{
>> +     u32 __ret;
>
> Can this result in the mrs receiving a W register?

Another question I can't answer.  I understand what you're saying, but
I don't know what to do about it.

>> +     asm volatile("mrs %0, mdccsr_el0" : "=r" (__ret)
>> +                     : : "cc");
>
> Why the CC clobber? Why volatile?

I don't think we need the CC clobber, but the volatiles do appear to
be important.  Without that, DCC does not work.

>> +
>> +     return __ret;
>> +}
>> +
>> +static inline char __dcc_getchar(void)
>> +{
>> +     char __c;
>> +
>> +     asm volatile("mrs %0, dbgdtrrx_el0" : "=r" (__c));
>> +     isb();
>
> Why the isb and why volatile??

The driver does appear to work without the isb, but there's a reason
it was added.  On ARM32, reading/writing the DCC register is
apparently a context change:

http://www.serverphorums.com/read.php?12,395361

I don't know if the same thing is true on ARM64.  However, I don't
understand why reading/writing the DCC register is a context change on
ARM32.

>> +
>> +     return __c;
>> +}
>> +
>> +static inline void __dcc_putchar(char c)
>> +{
>> +     asm volatile("msr dbgdtrtx_el0, %0"
>> +                     : /* No output register */
>> +                     : "r" (c));
>
> Can you guarantee that GCC hasn't put junk in the upper bits of c?

Should I change the prototype to:

static inline void __dcc_putchar(unsigned int c)


>
>> +     isb();
>
> Why the isb?
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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