| Message ID | 1455216004-19499-22-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote: > With VHE, we place kernel {watch,break}-points at EL2 to get things > like kgdb and "perf -e mem:..." working. > > This requires a bit of repainting in the low-level encore/decode, > but is otherwise pretty simple. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> To the best of my knowledge, this patch is fine ;) Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote: > With VHE, we place kernel {watch,break}-points at EL2 to get things > like kgdb and "perf -e mem:..." working. > > This requires a bit of repainting in the low-level encore/decode, > but is otherwise pretty simple. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++------------- > 1 file changed, 31 insertions(+), 18 deletions(-) > > diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h > index 9732908..4d8d5a8 100644 > --- a/arch/arm64/include/asm/hw_breakpoint.h > +++ b/arch/arm64/include/asm/hw_breakpoint.h > @@ -18,6 +18,7 @@ > > #include <asm/cputype.h> > #include <asm/cpufeature.h> > +#include <asm/virt.h> > > #ifdef __KERNEL__ > > @@ -35,24 +36,6 @@ struct arch_hw_breakpoint { > struct arch_hw_breakpoint_ctrl ctrl; > }; > > -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) > -{ > - return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) | > - ctrl.enabled; > -} > - > -static inline void decode_ctrl_reg(u32 reg, > - struct arch_hw_breakpoint_ctrl *ctrl) > -{ > - ctrl->enabled = reg & 0x1; > - reg >>= 1; > - ctrl->privilege = reg & 0x3; > - reg >>= 2; > - ctrl->type = reg & 0x3; > - reg >>= 2; > - ctrl->len = reg & 0xff; > -} > - > /* Breakpoint */ > #define ARM_BREAKPOINT_EXECUTE 0 > > @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg, > #define AARCH64_ESR_ACCESS_MASK (1 << 6) > > /* Privilege Levels */ > +#define AARCH64_BREAKPOINT_EL2 0 > #define AARCH64_BREAKPOINT_EL1 1 > #define AARCH64_BREAKPOINT_EL0 2 > > @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg, > #define ARM_KERNEL_STEP_ACTIVE 1 > #define ARM_KERNEL_STEP_SUSPEND 2 > > +#define DBG_HMC_HYP (1 << 13) > +#define DBG_SSC_HYP (3 << 14) Why do we need to touch the SSC field at all? > + > +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) > +{ > + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled; > + > + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1) > + val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1); I don't think this is correct. We want to allow, for example, a userspace watchpoint to fire thanks to something like put_user, so the encoding really needs to build up the PMC field (like we do already), then orr in the HMC field. The "gotcha", which is similar to the PMU stuff, is that you can't have HMC==1 (EL2) and PMC==2 (i.e. EL2 and EL0, but not EL1). I *think* the conclusion is that you need AARCH64_BREAKPOINT_EL2 to look like DBG_HMC_HYP | AARCH64_BREAKPOINT_EL1. Will
On Mon, Feb 15, 2016 at 05:46:56PM +0000, Will Deacon wrote: > On Thu, Feb 11, 2016 at 06:40:02PM +0000, Marc Zyngier wrote: > > With VHE, we place kernel {watch,break}-points at EL2 to get things > > like kgdb and "perf -e mem:..." working. > > > > This requires a bit of repainting in the low-level encore/decode, > > but is otherwise pretty simple. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++------------- > > 1 file changed, 31 insertions(+), 18 deletions(-) > > > > diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h > > index 9732908..4d8d5a8 100644 > > --- a/arch/arm64/include/asm/hw_breakpoint.h > > +++ b/arch/arm64/include/asm/hw_breakpoint.h > > @@ -18,6 +18,7 @@ > > > > #include <asm/cputype.h> > > #include <asm/cpufeature.h> > > +#include <asm/virt.h> > > > > #ifdef __KERNEL__ > > > > @@ -35,24 +36,6 @@ struct arch_hw_breakpoint { > > struct arch_hw_breakpoint_ctrl ctrl; > > }; > > > > -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) > > -{ > > - return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) | > > - ctrl.enabled; > > -} > > - > > -static inline void decode_ctrl_reg(u32 reg, > > - struct arch_hw_breakpoint_ctrl *ctrl) > > -{ > > - ctrl->enabled = reg & 0x1; > > - reg >>= 1; > > - ctrl->privilege = reg & 0x3; > > - reg >>= 2; > > - ctrl->type = reg & 0x3; > > - reg >>= 2; > > - ctrl->len = reg & 0xff; > > -} > > - > > /* Breakpoint */ > > #define ARM_BREAKPOINT_EXECUTE 0 > > > > @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg, > > #define AARCH64_ESR_ACCESS_MASK (1 << 6) > > > > /* Privilege Levels */ > > +#define AARCH64_BREAKPOINT_EL2 0 > > #define AARCH64_BREAKPOINT_EL1 1 > > #define AARCH64_BREAKPOINT_EL0 2 > > > > @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg, > > #define ARM_KERNEL_STEP_ACTIVE 1 > > #define ARM_KERNEL_STEP_SUSPEND 2 > > > > +#define DBG_HMC_HYP (1 << 13) > > +#define DBG_SSC_HYP (3 << 14) > > Why do we need to touch the SSC field at all? > > > + > > +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) > > +{ > > + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled; > > + > > + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1) > > + val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1); > > I don't think this is correct. We want to allow, for example, a userspace > watchpoint to fire thanks to something like put_user, so the encoding > really needs to build up the PMC field (like we do already), then orr in > the HMC field. Hmm, I got my arm and my arm64 mixed up here. For the latter, we don't actually support EL0+EL1 watchpoints, but I still think that the {HMC,SSC,PMC} encoding of {1,00,xx} is cleaner. Will
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h index 9732908..4d8d5a8 100644 --- a/arch/arm64/include/asm/hw_breakpoint.h +++ b/arch/arm64/include/asm/hw_breakpoint.h @@ -18,6 +18,7 @@ #include <asm/cputype.h> #include <asm/cpufeature.h> +#include <asm/virt.h> #ifdef __KERNEL__ @@ -35,24 +36,6 @@ struct arch_hw_breakpoint { struct arch_hw_breakpoint_ctrl ctrl; }; -static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) -{ - return (ctrl.len << 5) | (ctrl.type << 3) | (ctrl.privilege << 1) | - ctrl.enabled; -} - -static inline void decode_ctrl_reg(u32 reg, - struct arch_hw_breakpoint_ctrl *ctrl) -{ - ctrl->enabled = reg & 0x1; - reg >>= 1; - ctrl->privilege = reg & 0x3; - reg >>= 2; - ctrl->type = reg & 0x3; - reg >>= 2; - ctrl->len = reg & 0xff; -} - /* Breakpoint */ #define ARM_BREAKPOINT_EXECUTE 0 @@ -62,6 +45,7 @@ static inline void decode_ctrl_reg(u32 reg, #define AARCH64_ESR_ACCESS_MASK (1 << 6) /* Privilege Levels */ +#define AARCH64_BREAKPOINT_EL2 0 #define AARCH64_BREAKPOINT_EL1 1 #define AARCH64_BREAKPOINT_EL0 2 @@ -76,6 +60,35 @@ static inline void decode_ctrl_reg(u32 reg, #define ARM_KERNEL_STEP_ACTIVE 1 #define ARM_KERNEL_STEP_SUSPEND 2 +#define DBG_HMC_HYP (1 << 13) +#define DBG_SSC_HYP (3 << 14) + +static inline u32 encode_ctrl_reg(struct arch_hw_breakpoint_ctrl ctrl) +{ + u32 val = (ctrl.len << 5) | (ctrl.type << 3) | ctrl.enabled; + + if (is_kernel_in_hyp_mode() && ctrl.privilege == AARCH64_BREAKPOINT_EL1) + val |= DBG_HMC_HYP | DBG_SSC_HYP | (AARCH64_BREAKPOINT_EL2 << 1); + else + val |= ctrl.privilege << 1; + + return val; +} + +static inline void decode_ctrl_reg(u32 reg, + struct arch_hw_breakpoint_ctrl *ctrl) +{ + ctrl->enabled = reg & 0x1; + reg >>= 1; + ctrl->privilege = reg & 0x3; + if (ctrl->privilege == AARCH64_BREAKPOINT_EL2) + ctrl->privilege = AARCH64_BREAKPOINT_EL1; + reg >>= 2; + ctrl->type = reg & 0x3; + reg >>= 2; + ctrl->len = reg & 0xff; +} + /* * Limits. * Changing these will require modifications to the register accessors.
With VHE, we place kernel {watch,break}-points at EL2 to get things like kgdb and "perf -e mem:..." working. This requires a bit of repainting in the low-level encore/decode, but is otherwise pretty simple. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/include/asm/hw_breakpoint.h | 49 +++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 18 deletions(-)