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

321

[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.

371

Please use calloc() here.

490

What should happen here?

638

What is this memory barrier for?

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

756

Can this be const?

818

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

836

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

882

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

884

Do you need to check the return value?

941

Shouldn't this be if (!initialized)?

sys/x86/x86/local_apic.c

910

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

[lwhsu]

sys/amd64/pt/pt.h

3

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

212

Why not bool?

219

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

371

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?

490

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

638

The barrier is here for the NMI interrupt handler.

756

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)