Tested it on my end and it works. Code also looks good to me. Thanks!

I've read through the man page changes twice and couldn't find anything wrong. LGTM but maybe a read through by someone from docs would be a good idea.

Both seem reasonable to me. I'll aim to run some more tests with this and look it over one more time in detail by Friday and flag if I find anything, but overall looks good to me. Thanks for working on this!

Overall the patch looks good to me. I've run all of this under ASAN + UBSAN (with the full patch set applied) and nothing was flagged in this code as problematic when running the DTrace test suite and the FreeBSD build. I feel like it might be a good idea to properly restructure the code around CTFv2 and CTFv3 to account for other versions in the API rather than relying on the "else" case always being v3, but aside from that LGTM. @markj: what do you think about the alignment warning/error propagation?

The code looks good to me, but it probably needs man page changes like @debdrup mentioned. I also wonder if adding a comment at the top of the header somewhere simply stating what CTFv2 and CTFv3 are and how they differ at a high level makes sense. Perhaps in one or two short sentences so that if someone unfamiliar with CTF wants to simply use the header knows what they're working with without needing to read through a fairly detailed man page? Do you have any thoughts on this?

In D34358#778329, @markj wrote:

In D34358#778319, @domagoj.stolfa_gmail.com wrote:

The original CTF header has a bunch of useful comments describing the format. Perhaps these things should be included in this header too? While I appreciate that this is an import, those comments have helped me quite a bit when dealing with CTF and IMO should be included.

There is a ctf.5 man page that describes the format in much more detail than the comments did. I haven't updated it yet for v3 but will do so. Do you think it is sufficient to reference the man page here?

This looks good to me. Thanks!

The original CTF header has a bunch of useful comments describing the format. Perhaps these things should be included in this header too? While I appreciate that this is an import, those comments have helped me quite a bit when dealing with CTF and IMO should be included.

In D31494#723995, @jhb wrote:

In D31494#723984, @mhorne wrote:

In D31494#720213, @domagoj.stolfa_gmail.com wrote:

Thanks for writing this up @mhorne, much appreciated! I've left a few comments from my experience of setting it up with QEMU + KVM today and the confusion I had while reading the existing documentation, as well as the confusion that I would probably have while reading this.

I wonder if it might make sense to explicitly state that the procedure for virtualization gdb stubs may be different depending on which hypervisor/emulator is being used, and then in follow-up commits (by you or anyone else really) document the procedure for each given hypervisor?

Right, the process for remote gdb is a little different when you are using a hypervisor, mainly because the hypervisor can pause execution automatically when it detects that a client has connected to its gdb stub. For the gdb stub implemented by the kernel, we must force a trap into the debugger in order to detect that a client has detected. That is why this chapter lists extra steps that you did not need.

I didn't come up with any text to really highlight this difference, but I think it will be easier to do once we add a separate [sub]section for remote gdb using bhyve/kvm/whatever. You can suggest specific text for the intro paragraph if you like.

Yes, I think we will want to add a separate section for bare-metal debugging with a hypervisor vs using serial to talk to the in-kernel stub which is what this currently documents.

Thanks for writing this up @mhorne, much appreciated! I've left a few comments from my experience of setting it up with QEMU + KVM today and the confusion I had while reading the existing documentation, as well as the confusion that I would probably have while reading this.

@jhb Any chance you could push this? I've been using this for months and it's worked well for me.

In D30778#692250, @markj wrote:

In D30778#692214, @domagoj.stolfa_gmail.com wrote:

In D30778#691982, @markj wrote:

If there are any tags, e.g., sponsored by, please add them to the review description and I'll commit.

You can add a Signed-off-by: domagoj.stolfa@gmail.com if you'd like

I prefer not to since we don't have a policy around it, at least not yet. If you prefer to have it, then I'll keep it.

In D30778#691982, @markj wrote:

If there are any tags, e.g., sponsored by, please add them to the review description and I'll commit.

This seems reasonable to me.

I've been using this locally and found it *extremely* useful, and frankly was quite surprised when I found out that EV_ENABLE/EV_DISABLE trashed udata originally.

In D30255#679519, @sef wrote:

I still use an 80 character limit, and an 80 character-wide Terminal window.

Usually my opinion means everyone else has moved on :).

Remove a whole bunch of noise compared to the previous diff.

Update the diff with -U999999

@markj 's comments.

Added -x libdir and -x syslibdir as suggested by @markj

In D29435#659665, @markj wrote:

(Please let me know if you'd like me to commit this.)

In D16921#360817, @markj wrote:

In D16921#360786, @domagoj.stolfa_gmail.com wrote:

In D16921#360785, @markj wrote:

I think it would be nicer to instead keep all tracepoints for a given address on a linked list, with a single hash table entry for all of them. fbt_invop() would invoke dtrace_probe() for each probe associated with the tracepoint, which should be fine so long as there aren't many. (With ifuncs there will be 2.)

Perhaps we could even bunch them into a bounded array (or a vector-like implementation) and check it at fbt load-time in order to avoid indirection? A linked list is unlikely to be too painful given all the membars and indirection happening in dynamic variable implementation -- but nonetheless would degrade performance :-).

I think it's acceptable for the time being since, as I pointed out, there are at most 2 probes for a given tracepoint currently. The FBT hash table is itself cache-unfriendly, and I'd rather fix the performance problems there holistically. Here's the distribution of hash chain lengths on a system with a fairly stripped-down kernel:

5395 0
9755 1
8748 2
5244 3
2369 4
 876 5
 280 6
  73 7
  24 8
   3 9
   1 10

In D16921#360785, @markj wrote:

I think it would be nicer to instead keep all tracepoints for a given address on a linked list, with a single hash table entry for all of them. fbt_invop() would invoke dtrace_probe() for each probe associated with the tracepoint, which should be fine so long as there aren't many. (With ifuncs there will be 2.)

Why "additional"? The problem exists regardless since fbt_invop() returns after the first matching tracepoint. The problem was invisible before ifuncs since before that all tracepoints had distinct addresses. This patch just hides the problem in the common case.

I'm a little worried that this may cause additional confusion if someone expects multiple probes to fire at that point. I wonder if a better workaround would be to just add a warning message whenever DTrace is called, or perhaps when someone calls dtrace -l, in the man page or something along the lines until we fix? Is there a reason we can't fix this in 13 and MFC the changes?

Preemptively remove a redundant check for INKERNEL(fp).

In D15359#323676, @andrew wrote:

Why not just exclude it in fbt_provide_module_function? I'd prefer we keep the unwinding code together, maybe with a comment that it may be called from such a context.

In D15359#323666, @markj wrote:

however, in the future a patch that enabled instrumentation of inlined functions could accidentally break DTrace on aarch64.

Was the problem found while working on such a patch? :)

Had a quick skim, but LGTM.

Is this good to land? Thanks!

Address the other comments by @markj and fix a misleading word in the dtrace_probe_exit() comment.

Address comments by @markj.

Is there anything else I can do for this to land?

Should I make any more changes for this to land? As an aside note: I've tried to make the interface as simple as possible to use with the args[0-2] being consistent.

In D14862#312653, @avg wrote:

FWIW, rS253996 is what I did to translate INVARIANTS to DEBUG for ZFS.
Seems like the commit even touched some dtrace modules.

Update the diff based on comments from @markj.

Address the comments by markj@ and add tests in safety/ for jailname and jid.

Update the diff to add more context. No actual code was changed.

In D13877#290934, @dteske wrote:

In D13877#290818, @domagoj.stolfa_gmail.com wrote:

In D13877#290812, @dteske wrote:

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

That's true, however, this exposes a stable interface that people can use without following pointers in the D script and instead simply calling into a builtin variable. I'd imagine this is the same reason why you would want to have execname, errno, uid, gid and others, as they can also be accessible via other, more primitive builtins.

Don't get me wrong, I like the analog to Solaris's zonename and thus think jailname and jid are good additions.

I just wanted to point out that curthread already gives us access to far more details than the few given at the top-level.

Programs like "dwatch" (reviews.freebsd.org/D10006) make it possible to access not-only curthread details but the details of the parent, grandparent, and ancestor process(es), and thus the ability to trace a jail is neither novel nor unique.

If you're ever looking for inspiration on other top-level variables that might be nice-to-haves without having to crawl curthread->td_proc (as dwatch does), give dwatch a trial run.

In D13877#290812, @dteske wrote:

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

Update the diff to reflect changes in HEAD with bhyve identification. This diff no longer requires it.

Update the diff to address the compilation errors due to the way clang 4.0 handles -Wstrict-prototypes.

This diff changes the following:

• Remove DTrace stuff as much as possible, create a prototype instead
• Remove the unnecessary static NULL assignments in the hc_dispatcher
• Reiterate the instructions on some parts of the code

Point out an issue regarding mi_startup.

Address some style(9) issues, fix a comment.

License bhyve_hypercall.S under the FreeBSD preferred license.

Remove the unused include in bhyve_hypercall.h

Address the points discussed at MeetBSD.

Addresses a late noticed typo in the macro HC_CPUID_ID which in fact should be HV_CPUID_ID.

This patch addresses a couple of issues:

• Allow global enabling and disabling of hypercalls. The hypercall exists are by default disabled
• Add the necessary dispatchers and handlers for a hypervisor emulation layer
• Clean up the bhyve_hypercall.h using macros and revert to the calling convention that uses the stack
• Add a tunable which allows setting of the hypervisor mode
• Unimplemented hypercalls now cause an #UD fault
• CPUID_4000_0001 gives information in EAX whether hypercalls are enabled or not

This patch should address some of the concerns brought on by Peter.

In D8100#167647, @grehan wrote:

Thanks for this work - it will be very useful.

Some general high-level comments:

• There are a number of guest users of hypercalls: KVM, Xen, Hyper-V. Since bhyve may emulate some/all of these hypercalls, it's probably worthwhile looking at the call sequences they use to try and co-exist. For example, it may even be useful at some future point to have DTrace support in those hypervisors.
• In a similar vein, 'hypercall.h' should probably be 'bhyve_hypercall.h'
• the hypercall exit should be disabled by default unless a tunable is set. It's an attack point that should only be enabled through hypervisor configuration. While there isn't currently a generic way to set per-guest configuration, there will be at some point, at which time this feature could be shifted over to that instead of having a global enable/disable.
• in addition, a future work item, and not really part of this review, is to have a CPUID space carved out for bhyve that will provide a way for the guest to determine if the hypercall facility is available. See the KVM docs for an example of how this is done.

Thanks for the review Peter. I'll address these issues as soon as possible.

Added some explanations.