amd64: Intel Processor Trace support

[bnovkov]

Summary

Add support for Intel Processor Trace (PT) to the Hardware Trace Framework (HWT).

[markj]

sys/amd64/pt/pt.c

322

[picking a random example] Rather than one-line comments which describe what the code is doing, it is much more helpful to readers to have some high-level description of what's happening in this function.

372

Please use calloc() here.

491

What should happen here?

639

What is this memory barrier for?

Usually we prefer to use atomic(9) acq/rel barriers.

757

Can this be const?

819

This shouldn't be necessary, assuming we're in a hardware interrupt context.

837

Why pass this flag? This code isn't checking the return value.

883

calloc() would be a bit nicer than malloc() here.

885

Do you need to check the return value?

942

Shouldn't this be if (!initialized)?

sys/x86/x86/local_apic.c

910 ↗

(On Diff #142307)

Why not just if (refcount_release(...)) return;?

[lwhsu]

sys/amd64/pt/pt.h

4

A SPDX-License-Identifier is desired, and "All rights reserved" is not really needed for a new file.

[kib]

Please split the review. At least, specialreg.h additions are easy and should go separately. Same for the nmi interrupt handler refactoring.
Then the refcounting for perf interrupt can go. Then we will see about the rest.

sys/amd64/amd64/trap.c

211 ↗

(On Diff #142307)

212 ↗

(On Diff #142307)

Why not bool?

219 ↗

(On Diff #142307)

This line violates every single style(9) requirement, at once.

[bnovkov]

Address review comments:

• review is now split into multiple parts
• fixed copyright header issues
• hoisted single-line comments on top of functions

[bnovkov]

sys/amd64/pt/pt.c

372

Sorry if I'm missing something, but as far as I know (and what my grepping efforts turned up) calloc isn't defined in the kernel.
I've substituted malloc with mallocarray, were you thinking of defining a calloc-like macro on top of the file?

491

Thanks for catching this, it slipped by. This was meant to clear any features flags that aren't supported by the driver.

639

The barrier is here for the NMI interrupt handler.

757

At the moment no due to how things are setup in hwt(4) code, but I'll see what I can do about it.

[bnovkov]

Update driver to register the interrupt handler using the new dynamic NMI handler registration interface.

[bnovkov]

Tweak PCINT interrupt handling to avoid crashes when running alongside hwpmc(4).

[kib]

sys/amd64/pt/pt.c

107

The include should be

#include <amd64/pt/pt.h>

but see above about the need of the header at all.

132

This should go into fpu.h.

198

These two should go in amd64/include/cpufunc.h. Please prepare a dedicated review for this and fpu.h.

sys/amd64/pt/pt.h

53

This symbol should go into specialreg.h and follow the naming conventions from the file.

58

Isn't the define from x86/include/fpu.h enough?

76

Why this declare needs to be in the header. Event more, why do we need this header?

[bnovkov]

Address @kib 's comments:

• Parts of the diff were split into separate reviews
• Removed redundant defines and includes

[bnovkov]

sys/amd64/pt/pt.h

58

Isn't the define from x86/include/fpu.h enough?

It is, I managed to miss the definition, thanks!

As for the header, it is used by the userspace decoder tool to configure the driver (usr.sbin/hwt/hwt_pt.c in D40466)

[bnovkov]

Update driver to use XSAVE interfaces added in D47394.

[bnovkov]

Adjust calls to xsave_* routines after changes in D48466.

[kib]

sys/amd64/pt/pt.c

66

This header' inclusion is excessive.

69

systm.h already includes queue.h, types.h, and param.h at least

80

Order headers alphabetically. sys/lock.h must go before sys/mutex.h anyway.

84

Why do you need sleepqueue.h ?

107

Might be even machine/pt/pt.h

251

252

This is spelled fpu_enable()

272

if ((cr0 & CR0_TS) != 0)
    fpu_disable().

283

What are the invariants? Do we own some spinlock, or are we in critical section, or pinned?

287

Why do we need to toggle XSAVE?

357

So vm->npages is number of pages + 1?

sys/amd64/pt/pt.h

53

Why is this symbol needed for supposedly user/kernel interface header?

[bnovkov]

Address @kib 's comments.

sys/amd64/pt/pt.c

84

Why do you need sleepqueue.h ?

Ah, this is a leftover include from previous versions, removed.

283

This function is invoked as an smp_rendezvous action, maybe we should add a td_critnest >= 0 assertion here?

287

This was also left over from previous attempts and the driver works fine without it, removed.

[bnovkov]

Remove remaining magic numbers.

[bnovkov]

@kib do you have any additional comments on the patch?

[kib]

Sorry, I somehow lost your previous phab reply.

sys/amd64/pt/pt.c

68

All three headers (cdefs, types, param) can be removed, systm provides them.

84

Could you please order includes alphabetically (but systm should go first).

896

Do you want to free pt_pcpu{,_ctx}, and deregister nmi handler on error? initialized is only set to true if everything is ok, so you are leaking resources there, at least for my eye.

sys/amd64/pt/pt.h

53

Why cannot this define be moved into pt.c?

[bnovkov]

Address @kib 's comments:

• Prune unused includes and sort the remaining ones
• Free resources if there's an error during module initialization
• Move the supported flags mask to pt.c

[bnovkov]

sys/amd64/pt/pt.c

896

Good catch, thanks!
All resources get freed on error now.

sys/amd64/pt/pt.h

53

Yeah, it makes more sense for it to be in pt.c. Moved, thanks!

[ziaee]

sys/amd64/pt/pt.h

4

In fact, it is preferred to omit the license text entirely, check out style(9) or https://docs.freebsd.org/en/articles/license-guide/