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@xxxxxxxxx> wrote:
>
> Hi Will,
> Sorry for not instant respond.
>
>
> On Mon, Sep 30, 2019 at 11:34 PM Will Deacon <will@xxxxxxxxxx> wrote:
> >
> > On Thu, Sep 26, 2019 at 03:38:28PM +0800, Candle Sun wrote:
> > > From: Candle Sun <candle.sun@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Signed-off-by: Nianfu Bai <nianfu.bai@xxxxxxxxxx>
> > > ---
> > >  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