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

Commit Message

Timur Tabi Aug. 8, 2015, 12:16 a.m. UTC

From: Abhimanyu Kapur <[e-mail]>

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

Signed-off-by: Abhimanyu Kapur <[e-mail]>
Signed-off-by: Timur Tabi <[e-mail]>
---
 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

Comments

Will Deacon Aug. 10, 2015, 9:40 a.m. UTC | #1

On Sat, Aug 08, 2015 at 01:16:34AM +0100, Timur Tabi wrote:
> From: Abhimanyu Kapur <[e-mail]>
> 
> Add support for debug communications channel based
> hvc console for arm64 cpus.
> 
> Signed-off-by: Abhimanyu Kapur <[e-mail]>
> Signed-off-by: Timur Tabi <[e-mail]>
> ---
>  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

Timur Tabi Aug. 17, 2015, 11:56 p.m. UTC | #2

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

Dave Martin Aug. 18, 2015, 8:21 a.m. UTC | #3

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

Timur Tabi Aug. 18, 2015, 7:07 p.m. UTC | #4

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?

Dave Martin Aug. 19, 2015, 10:14 a.m. UTC | #5

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

Timur Tabi Aug. 19, 2015, 4:16 p.m. UTC | #6

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?

Dave Martin Aug. 19, 2015, 4:37 p.m. UTC | #7

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

Stephen Boyd Aug. 24, 2015, 11:51 p.m. UTC | #8

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?

Dave Martin Sept. 2, 2015, 11:10 a.m. UTC | #9

On Mon, Aug 24, 2015 at 04:51:44PM -0700, [e-mail] 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

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