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@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