amd64: Intel Processor Trace support
Needs ReviewPublic
Actions

Authored by bnovkov on Wed, Aug 21, 2:48 PM.

Details

Reviewers

markj
kib
br

Group Reviewers

x86

Summary

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

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

br requested review of this revision.Wed, Aug 21, 2:48 PM

br created this revision.

br foisted this revision upon bnovkov.

br added a reviewer: br.

br added a child revision: D40466: Hardware Trace (HWT) framework.

br edited the summary of this revision. (Show Details)Wed, Aug 21, 2:57 PM

br edited reviewers, added: x86; removed: br.

br added reviewers: markj, kib.Wed, Aug 21, 2:59 PM

markj added inline comments.Thu, Aug 22, 2:04 PM

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 added a subscriber: lwhsu.Thu, Aug 22, 3:41 PM

lwhsu added inline comments.

sys/amd64/pt/pt.h
4	A `SPDX-License-Identifier` is desired, and "All rights reserved" is not really needed for a new file.

br added a reviewer: br.Thu, Aug 22, 9:02 PM

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 added parent revisions: D46421: x86: Refactor kernel-mode NMI handling , D46419: x86: Add definitions for some Intel Processor Trace bits.Fri, Aug 23, 4:35 PM

Address review comments:

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

bnovkov marked 4 inline comments as done.Fri, Aug 23, 4:52 PM

bnovkov added inline comments.

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.