hwpmc_amd: check extended CPUID bits
AbandonedPublic
Actions

Authored by mhorne on Aug 1 2023, 4:24 PM.

Details

Reviewers

jkoshy
gnn
ali_mashtizadeh.com

Group Reviewers

pmc

Summary

There are three types of performance counters defined by the spec, with
three CPUID bits allocated to indicate support. Add detection of this to
the initialization routine.

Support for the "Core" performance counters is required, and if this is
not detected then the class will not attach. Let the other two types of
counters become optional.

This fixes an issue where use of hwpmc in bhyve could easily trigger
panics, as all MSR reads and writes were intercepted to be a no-op.
vmm(4) clears the relevant bits from CPUID, so now this behaves as
expected.

PR: 268943

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 52935
Build 49826: arc lint + arc unit

Event Timeline

mhorne created this revision.Aug 1 2023, 4:24 PM

Herald added a reviewer: gnn. · View Herald TranscriptAug 1 2023, 4:24 PM

Herald added a subscriber: imp. · View Herald Transcript

mhorne requested review of this revision.Aug 1 2023, 4:24 PM

Harbormaster completed remote builds in B52935: Diff 125431.Aug 1 2023, 4:24 PM

mhorne added a parent revision: D41276: hwpmc_amd: tighten up MSR definitions.Aug 1 2023, 4:24 PM

mhorne added a child revision: D41278: hwpmc_amd: improve assertions in amd_pcpu_fini().

jkoshy added inline comments.Aug 2 2023, 7:47 AM

sys/dev/hwpmc/hwpmc_amd.c
394	IIRC, case ranges are a GNU and Clang extension. I'm not sure if they are allowed by the current style(9).

mhorne added inline comments.Aug 2 2023, 12:46 PM

sys/dev/hwpmc/hwpmc_amd.c
394	I recently questioned this too in another review. There is a large enough precedent for the use of this extension within the src tree.

jkoshy accepted this revision.Aug 9 2023, 5:09 AM

This revision is now accepted and ready to land.Aug 9 2023, 5:09 AM

I want to have this fixed soon, but need to look a little deeper. It is difficult to determine exactly when these CPUID bits were added to the spec, and what CPUs are affected.

I just started looking through the backlog I think we can abandon this change now?

Herald added a reviewer: ali_mashtizadeh.com. · View Herald TranscriptMar 22 2026, 5:24 PM

In D41277#1281501, @ali_mashtizadeh.com wrote:

I just started looking through the backlog I think we can abandon this change now?

Yes. Your change set was the more complete and correct version of what I implemented here. However, the PR is still relevant, and I will try to put forward a new change addressing it.

mhorne abandoned this revision.Mar 23 2026, 2:08 PM