Please consider using this format for conformance with our policy and consistency with C style comments. You can see the policy here: https://docs.freebsd.org/en/articles/license-guide/

Please do not infer this as legal advice, but it must have a license declaration or we can not use it because all rights are reserved, with or without that line -- https://en.wikipedia.org/wiki/All_rights_reserved -- therefore our style guides no longer recommend it.

The .Os macro shouldn't have any argument

Please consider using Dq, according to the reference manual on this language, mdoc(7). Qq quotes are a pain in some of the myriad output formats manpages scale to.

https://man.freebsd.org/cgi/man.cgi?query=mdoc&apropos=0&sektion=7&manpath=FreeBSD+15.1-RELEASE+and+Ports.quarterly&format=html

ditto

ditto

It might make sense to spell these out here to assist with skimming, but I'm not sure.

ditto

ditto

We removed the hyphen on the opening line from our style guides, and the copyright line to come first when using SPDX only license declarations -- https://docs.freebsd.org/en/articles/license-guide/

Forgot the blank comment line at the top.

These newlines seems to have run together.

Yes, I'll do that.

I'm confused the module uses priv_check already shouldn't we just concern ourselves with the USER/SYSTEM CAPS here? All system mode counters have this enforced already.

https://github.com/freebsd/freebsd-src/blob/200de1b70e2b4f809d1d3a4c430db80b24124468/sys/dev/hwpmc/hwpmc_mod.c#L3451

The idea is to treat those MSRs as privileged, like the RAPL case. If the sysctl knob security.bsd.unprivileged_syspmcs is set to 1, unprivileged users will have access to those MSRs, and there are known attacks that could use them to break cryptographic code, such as Hertzbleed. The operating frequency is an indirect measurement of power consumption.

Implementing it this way makes the security model stronger, I think. It allows access to the MSRs only to the root user while the sysctl security.bsd.unprivileged_syspmcs is 1. An administrator can, for example, allow access to the TSC without exposing those counters. What do you think?

This nclasses thing has really become an anti-pattern. IMO it could be easily reworked to be friendlier to this type of dynamic class.

Not for you to fix in this change, however :)

I think it is better to mimic the pattern of the RAPL class below, rather than IBS above.

That is, call pmc_perf_initialize() unconditionally, and fold the cpu_power_ecx check inside that function. If unsupported, return an error, but make this not fatal.

Basically, we don't want to abort the basic AMD counter initialization if the PERF class somehow fails.

Perhaps?

Not really needed; i386 is EOL on 16-CURRENT and 15-STABLE.

I'm not sure if we can reuse 0x1 yet. Better to be safe and place this at the end of the list.

The only thing the pmc_perf_initialize() function does is allocating memory and initializing it. Even though there's the check, it shouldn't return error in any circumstance. I updated the code to include the validation in the perf functions.

Done!

Done! I've also removed the other i386 changes.

It makes sense, I moved it to be after RAPL.

Sometimes trying to be too specific makes it harder to read simplify if you can.

The ratio of APERF to MPERF scaled by the processor’s nominal frequency, yields the average effective frequency.

Both counters expose the unmodified contents of the underlying MSRs.

Probably you can simplify the above text since it’s repetitive.

I would just turn line 53 into its own sentence and remove 47-52 to eliminate repetition.

3rd time

Last bit (after the comma) about it being read only should probably be removed it’s confusing because you only expose a read-only counter.

What’s the second condition? The read-only bit doesn’t matter:
https://github.com/freebsd/freebsd-src/blob/de1aa3dab23c06fec962a14da3e7b4755c5880cf/sys/x86/x86/tsc.c#L243

Doesn’t p go between 44/45

Nit move next to read

Nit, move write just below read

I think we meant to keep these alphabetical

Reading it again, I agree! Thanks! The read-only part was really inadequate in this case.

The read-only bit stated in the manual was inadequate. The two conditions don't have anything to do with it. For the PERF class to be available, two conditions need to be met: the CPUID bit is advertised, and two consecutive reads return different values. As stated in the comment, the CPUID bit might be set but the counters don't increase in some scenarios, making them unfit, so the implementation depends on the tsc_perf_stat variable being 1, which tests that second condition.