| Message ID | 1438992995-22610-3-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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);
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(+)