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

108

The include should be

#include <amd64/pt/pt.h>

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

133

This should go into fpu.h.

199

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

sys/amd64/pt/pt.h

54

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

59

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

77

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

59

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)