* [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization
@ 2015-08-08 0:16 Timur Tabi
2015-08-08 0:16 ` [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Timur Tabi @ 2015-08-08 0:16 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-08 0:16 [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Timur Tabi
@ 2015-08-08 0:16 ` Timur Tabi
2015-08-10 9:40 ` Will Deacon
2015-08-08 0:16 ` [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC Timur Tabi
2015-08-10 9:48 ` [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Will Deacon
2 siblings, 1 reply; 16+ messages in thread
From: Timur Tabi @ 2015-08-08 0:16 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC
2015-08-08 0:16 [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Timur Tabi
2015-08-08 0:16 ` [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
@ 2015-08-08 0:16 ` Timur Tabi
2015-08-10 9:47 ` Will Deacon
2015-08-10 9:48 ` [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Will Deacon
2 siblings, 1 reply; 16+ messages in thread
From: Timur Tabi @ 2015-08-08 0:16 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-08 0:16 ` [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
@ 2015-08-10 9:40 ` Will Deacon
2015-08-17 23:56 ` Timur Tabi
0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2015-08-10 9:40 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC
2015-08-08 0:16 ` [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC Timur Tabi
@ 2015-08-10 9:47 ` Will Deacon
2015-08-17 22:45 ` Timur Tabi
0 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2015-08-10 9:47 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization
2015-08-08 0:16 [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Timur Tabi
2015-08-08 0:16 ` [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
2015-08-08 0:16 ` [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC Timur Tabi
@ 2015-08-10 9:48 ` Will Deacon
2015-08-19 22:51 ` Timur Tabi
2 siblings, 1 reply; 16+ messages in thread
From: Will Deacon @ 2015-08-10 9:48 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC
2015-08-10 9:47 ` Will Deacon
@ 2015-08-17 22:45 ` Timur Tabi
0 siblings, 0 replies; 16+ messages in thread
From: Timur Tabi @ 2015-08-17 22:45 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-10 9:40 ` Will Deacon
@ 2015-08-17 23:56 ` Timur Tabi
2015-08-18 8:21 ` Dave Martin
0 siblings, 1 reply; 16+ messages in thread
From: Timur Tabi @ 2015-08-17 23:56 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-17 23:56 ` Timur Tabi
@ 2015-08-18 8:21 ` Dave Martin
2015-08-18 19:07 ` Timur Tabi
0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2015-08-18 8:21 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-18 8:21 ` Dave Martin
@ 2015-08-18 19:07 ` Timur Tabi
2015-08-19 10:14 ` Dave Martin
0 siblings, 1 reply; 16+ messages in thread
From: Timur Tabi @ 2015-08-18 19:07 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-18 19:07 ` Timur Tabi
@ 2015-08-19 10:14 ` Dave Martin
2015-08-19 16:16 ` Timur Tabi
0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2015-08-19 10:14 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-19 10:14 ` Dave Martin
@ 2015-08-19 16:16 ` Timur Tabi
2015-08-19 16:37 ` Dave Martin
0 siblings, 1 reply; 16+ messages in thread
From: Timur Tabi @ 2015-08-19 16:16 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-19 16:16 ` Timur Tabi
@ 2015-08-19 16:37 ` Dave Martin
2015-08-24 23:51 ` sboyd at codeaurora.org
0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2015-08-19 16:37 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization
2015-08-10 9:48 ` [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Will Deacon
@ 2015-08-19 22:51 ` Timur Tabi
0 siblings, 0 replies; 16+ messages in thread
From: Timur Tabi @ 2015-08-19 22:51 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-19 16:37 ` Dave Martin
@ 2015-08-24 23:51 ` sboyd at codeaurora.org
2015-09-02 11:10 ` Dave Martin
0 siblings, 1 reply; 16+ messages in thread
From: sboyd at codeaurora.org @ 2015-08-24 23:51 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc
2015-08-24 23:51 ` sboyd at codeaurora.org
@ 2015-09-02 11:10 ` Dave Martin
0 siblings, 0 replies; 16+ messages in thread
From: Dave Martin @ 2015-09-02 11:10 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-09-02 11:10 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-08 0:16 [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Timur Tabi
2015-08-08 0:16 ` [PATCH 2/3] [v4] ARM64: TTY: hvc_dcc: Add support for ARM64 dcc Timur Tabi
2015-08-10 9:40 ` Will Deacon
2015-08-17 23:56 ` Timur Tabi
2015-08-18 8:21 ` Dave Martin
2015-08-18 19:07 ` Timur Tabi
2015-08-19 10:14 ` Dave Martin
2015-08-19 16:16 ` Timur Tabi
2015-08-19 16:37 ` Dave Martin
2015-08-24 23:51 ` sboyd at codeaurora.org
2015-09-02 11:10 ` Dave Martin
2015-08-08 0:16 ` [PATCH 3/3] [v2] hvc_dcc: disable user-space access to DCC Timur Tabi
2015-08-10 9:47 ` Will Deacon
2015-08-17 22:45 ` Timur Tabi
2015-08-10 9:48 ` [PATCH 1/3] [v2] hvc_dcc: don't ignore errors during initialization Will Deacon
2015-08-19 22:51 ` Timur Tabi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.