[RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Candle Sun]

From: Candle Sun <candle.sun@unisoc.com>

When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.

From ARMv8 specification, different debug architecture versions defined:
* 0110 ARMv8, v8 Debug architecture.
* 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
* 1000 ARMv8.2, v8.2 Debug architecture.

So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
returns -ENODEV, and arch_hw_breakpoint_init() will fail.

Signed-off-by: Candle Sun <candle.sun@unisoc.com>
Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
---
 arch/arm/include/asm/hw_breakpoint.h | 2 ++
 arch/arm/kernel/hw_breakpoint.c      | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index ac54c06..9137ef6 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -53,6 +53,8 @@ static inline void decode_ctrl_reg(u32 reg,
 #define ARM_DEBUG_ARCH_V7_MM	4
 #define ARM_DEBUG_ARCH_V7_1	5
 #define ARM_DEBUG_ARCH_V8	6
+#define ARM_DEBUG_ARCH_V8_1	7
+#define ARM_DEBUG_ARCH_V8_2	8
 
 /* Breakpoint */
 #define ARM_BREAKPOINT_EXECUTE	0
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b0c195e..cb99612 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -246,6 +246,8 @@ static int enable_monitor_mode(void)
 	case ARM_DEBUG_ARCH_V7_ECP14:
 	case ARM_DEBUG_ARCH_V7_1:
 	case ARM_DEBUG_ARCH_V8:
+	case ARM_DEBUG_ARCH_V8_1:
+	case ARM_DEBUG_ARCH_V8_2:
 		ARM_DBG_WRITE(c0, c2, 2, (dscr | ARM_DSCR_MDBGEN));
 		isb();
 		break;
-- 
2.7.4

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Will Deacon]

On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> From: Candle Sun <candle.sun@unisoc.com>
> 
> When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
> arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.
> 
> From ARMv8 specification, different debug architecture versions defined:
> * 0110 ARMv8, v8 Debug architecture.
> * 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
> * 1000 ARMv8.2, v8.2 Debug architecture.
> 
> So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
> returns -ENODEV, and arch_hw_breakpoint_init() will fail.
> 
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
>  arch/arm/include/asm/hw_breakpoint.h | 2 ++
>  arch/arm/kernel/hw_breakpoint.c      | 2 ++
>  2 files changed, 4 insertions(+)

How did you test this?

Will

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Candle Sun]

Hi Will,
Sorry for not instant respond.


On Mon, Sep 30, 2019 at 11:34 PM Will Deacon <will@kernel.org> wrote:
>
> On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
> > arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.
> >
> > From ARMv8 specification, different debug architecture versions defined:
> > * 0110 ARMv8, v8 Debug architecture.
> > * 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
> > * 1000 ARMv8.2, v8.2 Debug architecture.
> >
> > So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
> > returns -ENODEV, and arch_hw_breakpoint_init() will fail.
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> >  arch/arm/include/asm/hw_breakpoint.h | 2 ++
> >  arch/arm/kernel/hw_breakpoint.c      | 2 ++
> >  2 files changed, 4 insertions(+)
>
> How did you test this?
>
> Will

We have the SoC with A55 cores. On one Android project, for saving memory usage,
we let A55 run in aarch32 mode.
While the following failures occue on Android CtsBionicTestCases:
--sys_ptrace#watchpoint_imprecisede
--sys_ptrace#hardware_breakpoint
--sys_ptrace#watchpoint_stress

The code snippet for testing is:

static void check_hw_feature_supported(pid_t child, HwFeature feature) {
#if defined(__arm__)
  long capabilities;
  long result = ptrace(PTRACE_GETHBPREGS, child, 0, &capabilities);
  if (result == -1) {
    EXPECT_EQ(EIO, errno);
    GTEST_SKIP() << "Hardware debug support disabled at kernel
configuration time";
  }
  uint8_t hb_count = capabilities & 0xff;
  capabilities >>= 8;
  uint8_t wp_count = capabilities & 0xff;
  capabilities >>= 8;
  uint8_t max_wp_size = capabilities & 0xff;
  if (max_wp_size == 0) {
    GTEST_SKIP() << "Kernel reports zero maximum watchpoint size";
  } else if (feature == HwFeature::Watchpoint && wp_count == 0) {
    GTEST_SKIP() << "Kernel reports zero hardware watchpoints";
  } else if (feature == HwFeature::Breakpoint && hb_count == 0) {
    GTEST_SKIP() << "Kernel reports zero hardware breakpoints";
  }
#elif defined(__aarch64__)
  user_hwdebug_state dreg_state;
  iovec iov;
  iov.iov_base = &dreg_state;
  iov.iov_len = sizeof(dreg_state);

  long result = ptrace(PTRACE_GETREGSET, child,
                       feature == HwFeature::Watchpoint ?
NT_ARM_HW_WATCH : NT_ARM_HW_BREAK, &iov);
  if (result == -1) {
    ASSERT_EQ(EINVAL, errno);
  }
  if ((dreg_state.dbg_info & 0xff) == 0) GTEST_SKIP() << "hardware
support missing";
#else
  // We assume watchpoints and breakpoints are always supported on x86.
  UNUSED(child);
  UNUSED(feature);
#endif
}

The max_wp_size field returned by __ptrace() from kernel is zero,
which causes the test failures.

After futher analysis, we found max_watchpoint_len variable is not
right initialized in kernel
arch_hw_breakpoint_init() function. Missing the case of ARM_DEBUG_ARCH_V8_2 in
enable_monitor_mode() directly aborts the arch_hw_breakpoint_int().

Candle
Best regards

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Candle Sun]

Will,
Is the patch useful for you? Would you please give me some suggestions?
Thank you.

Regards,
Candle


On Tue, Oct 8, 2019 at 3:20 PM Candle Sun <candlesea@gmail.com> wrote:
>
> Hi Will,
> Sorry for not instant respond.
>
>
> On Mon, Sep 30, 2019 at 11:34 PM Will Deacon <will@kernel.org> wrote:
> >
> > On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> > > From: Candle Sun <candle.sun@unisoc.com>
> > >
> > > When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
> > > arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.
> > >
> > > From ARMv8 specification, different debug architecture versions defined:
> > > * 0110 ARMv8, v8 Debug architecture.
> > > * 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
> > > * 1000 ARMv8.2, v8.2 Debug architecture.
> > >
> > > So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
> > > returns -ENODEV, and arch_hw_breakpoint_init() will fail.
> > >
> > > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > > ---
> > >  arch/arm/include/asm/hw_breakpoint.h | 2 ++
> > >  arch/arm/kernel/hw_breakpoint.c      | 2 ++
> > >  2 files changed, 4 insertions(+)
> >
> > How did you test this?
> >
> > Will
>
> We have the SoC with A55 cores. On one Android project, for saving memory usage,
> we let A55 run in aarch32 mode.
> While the following failures occue on Android CtsBionicTestCases:
> --sys_ptrace#watchpoint_imprecisede
> --sys_ptrace#hardware_breakpoint
> --sys_ptrace#watchpoint_stress
>
> The code snippet for testing is:
>
> static void check_hw_feature_supported(pid_t child, HwFeature feature) {
> #if defined(__arm__)
>   long capabilities;
>   long result = ptrace(PTRACE_GETHBPREGS, child, 0, &capabilities);
>   if (result == -1) {
>     EXPECT_EQ(EIO, errno);
>     GTEST_SKIP() << "Hardware debug support disabled at kernel
> configuration time";
>   }
>   uint8_t hb_count = capabilities & 0xff;
>   capabilities >>= 8;
>   uint8_t wp_count = capabilities & 0xff;
>   capabilities >>= 8;
>   uint8_t max_wp_size = capabilities & 0xff;
>   if (max_wp_size == 0) {
>     GTEST_SKIP() << "Kernel reports zero maximum watchpoint size";
>   } else if (feature == HwFeature::Watchpoint && wp_count == 0) {
>     GTEST_SKIP() << "Kernel reports zero hardware watchpoints";
>   } else if (feature == HwFeature::Breakpoint && hb_count == 0) {
>     GTEST_SKIP() << "Kernel reports zero hardware breakpoints";
>   }
> #elif defined(__aarch64__)
>   user_hwdebug_state dreg_state;
>   iovec iov;
>   iov.iov_base = &dreg_state;
>   iov.iov_len = sizeof(dreg_state);
>
>   long result = ptrace(PTRACE_GETREGSET, child,
>                        feature == HwFeature::Watchpoint ?
> NT_ARM_HW_WATCH : NT_ARM_HW_BREAK, &iov);
>   if (result == -1) {
>     ASSERT_EQ(EINVAL, errno);
>   }
>   if ((dreg_state.dbg_info & 0xff) == 0) GTEST_SKIP() << "hardware
> support missing";
> #else
>   // We assume watchpoints and breakpoints are always supported on x86.
>   UNUSED(child);
>   UNUSED(feature);
> #endif
> }
>
> The max_wp_size field returned by __ptrace() from kernel is zero,
> which causes the test failures.
>
> After futher analysis, we found max_watchpoint_len variable is not
> right initialized in kernel
> arch_hw_breakpoint_init() function. Missing the case of ARM_DEBUG_ARCH_V8_2 in
> enable_monitor_mode() directly aborts the arch_hw_breakpoint_int().
>
> Candle
> Best regards

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Uwe Kleine-König]

Hello,

just noticed a typo in the subject line while going through my lakml
mailbox:

	s/architecutre/architecture/

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Candle Sun]

Thanks Uwe for pointing out my typing error.

Will,
Is the patch ok? Do I need to send another version?

Candle


Candle

On Fri, Oct 11, 2019 at 2:00 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> just noticed a typo in the subject line while going through my lakml
> mailbox:
>
>         s/architecutre/architecture/
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Will Deacon]

On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> From: Candle Sun <candle.sun@unisoc.com>
> 
> When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
> arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.
> 
> From ARMv8 specification, different debug architecture versions defined:
> * 0110 ARMv8, v8 Debug architecture.
> * 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
> * 1000 ARMv8.2, v8.2 Debug architecture.
> 
> So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
> returns -ENODEV, and arch_hw_breakpoint_init() will fail.
> 
> Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> ---
>  arch/arm/include/asm/hw_breakpoint.h | 2 ++
>  arch/arm/kernel/hw_breakpoint.c      | 2 ++
>  2 files changed, 4 insertions(+)

[...]

> diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
> index ac54c06..9137ef6 100644
> --- a/arch/arm/include/asm/hw_breakpoint.h
> +++ b/arch/arm/include/asm/hw_breakpoint.h
> @@ -53,6 +53,8 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define ARM_DEBUG_ARCH_V7_MM	4
>  #define ARM_DEBUG_ARCH_V7_1	5
>  #define ARM_DEBUG_ARCH_V8	6
> +#define ARM_DEBUG_ARCH_V8_1	7
> +#define ARM_DEBUG_ARCH_V8_2	8

Looks like you can also add:

#define ARM_DEBUG_ARCH_V8_4	9

and treat that the same way. With that, and a fix to $SUBJECT:

Acked-by: Will Deacon <will@kernel.org>

Please put this into the patch system [1].

Will

[1] https://www.arm.linux.org.uk/developer/patches/

Re: [RESEND PATCH] ARM/hw_breakpoint: add ARMv8.1/ARMv8.2 debug architecutre versions support in enable_monitor_mode()

[Candle Sun]

On Tue, Oct 22, 2019 at 8:25 PM Will Deacon <will@kernel.org> wrote:
>
> On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> > From: Candle Sun <candle.sun@unisoc.com>
> >
> > When ARMv8.1/ARMv8.2 cores are used in AArch32 mode,
> > arch_hw_breakpoint_init() in arch/arm/kernel/hw_breakpoint.c will be used.
> >
> > From ARMv8 specification, different debug architecture versions defined:
> > * 0110 ARMv8, v8 Debug architecture.
> > * 0111 ARMv8.1, v8 Debug architecture, with Virtualization Host Extensions.
> > * 1000 ARMv8.2, v8.2 Debug architecture.
> >
> > So missing ARMv8.1/ARMv8.2 cases will cause enable_monitor_mode() function
> > returns -ENODEV, and arch_hw_breakpoint_init() will fail.
> >
> > Signed-off-by: Candle Sun <candle.sun@unisoc.com>
> > Signed-off-by: Nianfu Bai <nianfu.bai@unisoc.com>
> > ---
> >  arch/arm/include/asm/hw_breakpoint.h | 2 ++
> >  arch/arm/kernel/hw_breakpoint.c      | 2 ++
> >  2 files changed, 4 insertions(+)
>
> [...]
>
> > diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
> > index ac54c06..9137ef6 100644
> > --- a/arch/arm/include/asm/hw_breakpoint.h
> > +++ b/arch/arm/include/asm/hw_breakpoint.h
> > @@ -53,6 +53,8 @@ static inline void decode_ctrl_reg(u32 reg,
> >  #define ARM_DEBUG_ARCH_V7_MM 4
> >  #define ARM_DEBUG_ARCH_V7_1  5
> >  #define ARM_DEBUG_ARCH_V8    6
> > +#define ARM_DEBUG_ARCH_V8_1  7
> > +#define ARM_DEBUG_ARCH_V8_2  8
>
> Looks like you can also add:
>
> #define ARM_DEBUG_ARCH_V8_4     9
>
> and treat that the same way. With that, and a fix to $SUBJECT:
>
> Acked-by: Will Deacon <will@kernel.org>
>
> Please put this into the patch system [1].
>
> Will
>
> [1] https://www.arm.linux.org.uk/developer/patches/

Thanks, Will.
I will do it ASAP.

Best regards,
Candle