Re: [RFC] Systemtap translator support for hardware breakpoints on

[fche at redhat dot com (Frank Ch. Eigler)]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

> [...]
> This patch enables systemtap translator to probe hardware breakpoints
> on x86 and x86_64 (based on mainline kernel).

Looks like a good start.

In order to merge this, it is essential to have a testsuite, posted
results on a variety of architectures and kernel versions (kfail for
older kernels), and documentation.


> Syntax :
> probe kernel.data(ADDRESS).write
> probe kernel.data(ADDRESS).rw
> probe kernel.data(ADDRESS).length(LEN).write
> probe kernel.data(ADDRESS).length(LEN).rw
> probe kernel.data("SYMBOL_NAME").write
> probe kernel.data("SYMBOL_NAME").rw

(I wonder if simple .read or .write would be sufficient as suffixes,
and let a script render both as 

   probe kernel.data(ADDRESS).write, probe kernel.data(ADDRESS).read

and let the translator collapse such duplication into the internal RW case.)


> +  // Warn of misconfigured kernels
> +  s.op->newline() << "#ifndef CONFIG_HAVE_HW_BREAKPOINT";
> +  s.op->newline() << "#error \"Need CONFIG_HAVE_HW_BREAKPOINT!\"";
> +  s.op->newline() << "#endif";

Instead or in addition, the translator could make the whole probe point
type conditional on session.kernel_config("CONFIG_HAVE_HW_BREAKPOINT").

> +// Define macros for access type
> +  s.op->newline() << "#define HWBKPT_READ 0";
> +  s.op->newline() << "#define HWBKPT_WRITE 1";
> +  s.op->newline() << "#define HWBKPT_RW 2";

(Are these necessary?  Could the translator not emit symbols like
HW_BREAKPOINT_W directly?)


> +  s.op->newline() << "static struct stap_hwbkpt_probe {";
> +  s.op->newline() << "int registered_p:1;";
> +// registered_p = -1 signifies an invalid probe that must not be registered
> +// registered_p =  0 signifies a probe that failed registration
> +// registered_p =  1 signifies a probe that got registered successfully
 
A one-bit value can't represent three different values.


> +      if (p->symbol_name.size())
> +      s.op->line() << " .address=(unsigned long)0x0" << "ULL,";
 
(You probably don't need explicit initialization to zero.)


> +  s.op->newline() << "static int enter_hwbkpt_probe (struct perf_event *bp,";
> [...]
> +  s.op->newline() << "for ( i=0; i<" << hwbkpt_probes_vector.size() << "; i++) {";
> +  s.op->newline() << "struct perf_event_attr *hp = & stap_hwbkpt_probe_array[i];";
> +  s.op->newline() << "if (bp->attr.bp_addr==hp->bp_addr && bp->attr.bp_type==hp->bp_type && bp->attr.bp_len==hp->bp_len ) {";
> [...]

To what extent does this handle multiple hw breakpoints on the same address?


> [...]
> +void
> +hwbkpt_derived_probe_group::emit_module_init (systemtap_session& s)
> +{ [...]
> +  s.op->newline() << "const char *hwbkpt_symbol_name = addr ? NULL : sdp->symbol;";
> +  s.op->newline() << "	hw_breakpoint_init(hp);";

Normally we use newline(1) or (-1) to indent instead of explicit
spaces like this.

> +  s.op->newline() << "	if (addr) ";
> +  s.op->newline() << "	    hp->bp_addr = (unsigned long) addr;";
> +  s.op->newline() << "	else ";
> +  s.op->newline() << "	   hp->bp_addr = kallsyms_lookup_name(hwbkpt_symbol_name);";

Is kallsyms_lookup_name actually module-exported these days?  Should the
translator try to resolve symbols at translate time?  Wildcards?


> +  s.op->newline() << "#ifdef CONFIG_X86";
> +  s.op->newline() << "  switch(sdp->len) {";
> +  s.op->newline() << "	case HW_BREAKPOINT_LEN_1:";
> +  s.op->newline() << "    hp->bp_len = HW_BREAKPOINT_LEN_1;";
> [...]

Is this sort of switch really necessary?  Why not just set

    hp->bp_len = sdp->len;

> +  s.op->newline() << "	default:";
> +  s.op->newline() << "  	_stp_warn(\"Probe %s registration skipped : unsupported length. Supported lengths for x86 are 1,2,4 {8 for x86_84 only} \",sdp->pp);";
> +  s.op->newline() << "		sdp->registered_p = -1;";
> +  s.op->newline() << "		stap_hwbkpt_ret_array[i] = ERR_PTR(-EINVAL);";

Why is this necessary ...



> +  s.op->newline() << "    case HWBKPT_READ:";
> +  s.op->newline() << "  	_stp_warn(\"Probe %s registration skipped : READ-ONLY hardware breakpoints not supported on x86\",sdp->pp);";
> +  s.op->newline() << "		sdp->registered_p = -1;";
> +  s.op->newline() << "		break;";

... and this too ...

> +  s.op->newline() << "if (sdp->registered_p >= 0) {";
> +  s.op->newline() << " stap_hwbkpt_ret_array[i] = register_wide_hw_breakpoint( hp, (void *)&enter_hwbkpt_probe );";

... considering that register_wide_hw_breakpoint() would surely
check its inputs and reject requests deemed inappropriate for
the current architecture?  

Note also that registered_p always >= 0 for your actual int:1 data
type.  Consider simply == 0 for unregistered, == 1 for registered.


> +  s.op->newline() << " if (IS_ERR(stap_hwbkpt_ret_array[i])) {";
> +  s.op->newline() << "     rc = PTR_ERR(stap_hwbkpt_ret_array[i]);";
> +  s.op->newline() << "      sdp->registered_p = 0;";
> +  s.op->newline(1) << "	    _stp_warn(\" Hwbkpt Probe %s : Registration error rc= %d, Addr = %p, name = %s\", probe_point, rc, addr, hwbkpt_symbol_name);";
> +  s.op->newline(-1) << "  }";
> +  s.op->newline() << "    else sdp->registered_p = 1;";
> +  s.op->newline() << "}";
> +  s.op->newline(-1) << "}"; // for loop

Not so simple: if you get a failure on the sixth registration, you have to
unregister the prior ones.


- FChE