| Message ID | 1434751734-2178-1-git-send-email-timur@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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
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.
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