Add support for Intel Processor Trace (PT) to the Hardware Trace Framework (HWT).
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| 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;? |
| sys/amd64/pt/pt.h | ||
|---|---|---|
| 4 | A SPDX-License-Identifier is desired, and "All rights reserved" is not really needed for a new file. | |
Comment Actions
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. |
Comment Actions
Address review comments:
- review is now split into multiple parts
- fixed copyright header issues
- hoisted single-line comments on top of functions
| 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. | |
| 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. | |
Comment Actions
Update driver to register the interrupt handler using the new dynamic NMI handler registration interface.
| 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? | |
Comment Actions
Address @kib 's comments:
- Parts of the diff were split into separate reviews
- Removed redundant defines and includes