Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1325757 > unrolled thread

[PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2016-02-03 19:10 +0100
Last post2016-02-08 18:00 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-03 19:10 +0100
    Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Catalin Marinas <catalin.marinas@arm.com> - 2016-02-08 17:00 +0100
      Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Marc Zyngier <marc.zyngier@arm.com> - 2016-02-08 17:50 +0100
        Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if  running in HYP Catalin Marinas <catalin.marinas@arm.com> - 2016-02-08 18:00 +0100

#1325757 — [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-03 19:10 +0100
Subject[PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
Message-ID<qYa2m-5r0-19@gated-at.bofh.it>
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..0da0272 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
 
@@ -76,6 +59,36 @@ 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;
+	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;
+	if (is_kernel_in_hyp_mode())
+		ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));
+	else
+		ctrl->privilege	= reg & 0x3;
+	reg >>= 2;
+	ctrl->type	= reg & 0x3;
+	reg >>= 2;
+	ctrl->len	= reg & 0xff;
+}
+
 /*
  * Limits.
  * Changing these will require modifications to the register accessors.
-- 
2.1.4

[toc] | [next] | [standalone]

#1329226 — Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

FromCatalin Marinas <catalin.marinas@arm.com>
Date2016-02-08 17:00 +0100
SubjectRe: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
Message-ID<qZWoi-7it-9@gated-at.bofh.it>
In reply to#1325757
On Wed, Feb 03, 2016 at 06:00:14PM +0000, Marc Zyngier wrote:
> @@ -76,6 +59,36 @@ 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;
> +	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;
> +	if (is_kernel_in_hyp_mode())
> +		ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));

I don't particularly like this part as it's not clear just by looking at
the code that it, in fact, generates AARCH64_BREAKPOINT_EL1. I would
make this clearer:

	if (is_kernel_in_hyp_mode() && (reg & (DBG_HMC_HYP >> 1)))
		ctrl->privilege = AARCH64_BREAKPOINT_EL1;

Alternatively, you could define the PMC field value as:

#define AARCH64_BREAKPOINT_EL2	0

and change the privilege to EL1 after masking, something like:

	ctrl->privilege = reg & 0x3;
	if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
		ctrl->privilege = AARCH64_BREAKPOINT_EL1;

BTW, do we need to check is_kernel_in_hyp_mode() when decoding? Is there
anything else that could have set this SSC/HMC/PMC fields other than
encode_ctrl_reg()?

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1329307 — Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-08 17:50 +0100
SubjectRe: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
Message-ID<qZXaH-7Ql-43@gated-at.bofh.it>
In reply to#1329226
On 08/02/16 15:56, Catalin Marinas wrote:
> On Wed, Feb 03, 2016 at 06:00:14PM +0000, Marc Zyngier wrote:
>> @@ -76,6 +59,36 @@ 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;
>> +	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;
>> +	if (is_kernel_in_hyp_mode())
>> +		ctrl->privilege = !!(reg & (DBG_HMC_HYP >> 1));
> 
> I don't particularly like this part as it's not clear just by looking at
> the code that it, in fact, generates AARCH64_BREAKPOINT_EL1. I would
> make this clearer:
> 
> 	if (is_kernel_in_hyp_mode() && (reg & (DBG_HMC_HYP >> 1)))
> 		ctrl->privilege = AARCH64_BREAKPOINT_EL1;
> 
> Alternatively, you could define the PMC field value as:
> 
> #define AARCH64_BREAKPOINT_EL2	0
> 
> and change the privilege to EL1 after masking, something like:
> 
> 	ctrl->privilege = reg & 0x3;
> 	if (ctrl->privilege == AARCH64_BREAKPOINT_EL2)
> 		ctrl->privilege = AARCH64_BREAKPOINT_EL1;
> 
> BTW, do we need to check is_kernel_in_hyp_mode() when decoding? Is there
> anything else that could have set this SSC/HMC/PMC fields other than
> encode_ctrl_reg()?

I was being overzealous, and your solution is clearly better. I ended up with the following:

diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index 9732908..c872b2f 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;
+	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.

Was that what you had in mind?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1329328 — Re: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP

FromCatalin Marinas <catalin.marinas@arm.com>
Date2016-02-08 18:00 +0100
SubjectRe: [PATCH v3 21/23] arm64: hw_breakpoint: Allow EL2 breakpoints if running in HYP
Message-ID<qZXkn-7Uw-31@gated-at.bofh.it>
In reply to#1329307
On Mon, Feb 08, 2016 at 04:45:44PM +0000, Marc Zyngier wrote:
> I was being overzealous, and your solution is clearly better. I ended up with the following:
> 
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index 9732908..c872b2f 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
[...]
> @@ -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;

Nitpick, for completeness:

		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.
> 
> Was that what you had in mind?

Looks fine:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web