Some minor stuff to change.

Changes themselves look good, except for one mistake (wrong MSR).

Some small changes to do. Main point is to preserve sysctl_cppc_dump_handler()'s goal which is to always query the hardware (and thus never rely on cached values).

Fine with the idea and approach. Missing are MPASS() on setp in the CPU_WHICH_CPUSET and CPU_WHICH_JAIL cases though.

(Alternative: Don't compile this on 32-bit architectures at all, if if_iwx in reality does not support those.)

Looks good (some minor remarks in inline comments).

Overall looks good, but I have some remarks/questions given that that change now exposes the POWER_SSTATE_TRANSITION_* constants to userland.

1. I think we should consider renaming these constants, perhaps doing something as radical as POWER_SSTATE_TRANSITION_* => POWER_*, or perhaps keeping STATE (instead of SSTATE), before they are made publicly available (after which, we will have to provide them (almost) indefinitely).
2. Is this the granularity we want to expose? I'm OK with what is available right now, and we can later add more to that list in order to allow requesting *precise* states (i.e., not rely on the value of the power_*_stype variables to know which internal state we will finally go into). But just wanted to hear your thoughts.

In D55477#1272999, @gallatin wrote:

I hope you don't just mean having powerd influence the hardware control loop by tweaking the epp sysctl. That would lead to 2 control loops trying to govern the same thing, which often ends in chaos.

Idea is great. Currently, however, this revision depends on D55477 (which is rejected), so it needs to be re-based to become independent.

IOW, that would mean reopening PR 292615.

Mmm, this change from D55253 is deliberate. Autonomous mode does not work on some AMD CPUs, where setting "desired performance" to 0 actually causes lowest performance to be selected. As long as we do not have a user-understandable way to choose a better default, such as PM profile in ACPI's FADT, let's do like all other CPU freq/power drivers do (except hwpstate_intel(4)) and select maximum performance. Let's revise this only as part of actually implementing one.

As said previously, I'm opposed to this approach. So we have to discuss (for other people, this discussion has started through other channels as well, but I'll try to keep this public revision updated as much as possible).

Being out-of-time (and off for the next week), I only reviewed changes to ULE, and the first one in this revision. But I'll also take a look at the rest when coming back.

Adding a task to the list above:

• On resume, we need to re-program the CPPC_REQUEST MSR, else current settings do not apply anymore.

Oh, that was unintended. Already fixed it locally but forgot to update the diff here.

Quoting part of my recent mail on src-committers@:

But since there are uses of 'freebsd32.h' unconditionally on all platforms (in 'kern_umtx.c' at least), and since we also check sizes of structures, I think we should at least ensure that 'struct foo32' on a 32-bit arch is type compatible with 'struct foo' on the same arch.

And I do not see that as unsustainable, on the contrary, it is very simple to achieve: Have all 'foo32' types boil down to exactly 'foo' on 32-bit architectures, which is what we are supposed to do already for structures by compat' design. 'freebsd32_uint64_t' you introduced typically supports that. That's the why of https://reviews.freebsd.org/D55283, which fixes commit 87632ddf67b0 ("openzfs sys/types32.h: use abi_compat.h for time32_t") where defining 'time32_t' to 'in32_t' for 32-bit architectures appears to have been an arbitrary choice of yours, which in practice by luck does not change the whole size of 'struct ffclock_estimate32' because 'struct bintime32''s one does not change either as even if its field 'sec' was incorrectly sized after you commit (this is what D55283 fixes), the 'frac' one is 64-bit and 64-bit aligned on all non-x86 architectures so its offset in 'struct bintime32' stays the same.

So, again, I think the rule of thumb should just be: Type 'foo32' is compatible with 'foo' on 32-bit architectures and has same alignment. That's the only thing that makes sense if 'struct *32' are visible on 32-bit architectures.

In D55045#1263800, @kib wrote:

I prefer to expose one function. There is no harm from returning unused value.

In D55045#1263612, @markj wrote:

I'm not even sure about that now. Binding on each CPU will cause a thread switch, which disables interrupts, and we'll have two of these per core (one for switch-in and one for switch-out), which in terms of duration of disabled interrupts may well prove longer than just the rendez-vous IPI and the small payload of handling the MSR, perhaps even for MSRs that are long to operate on.

Yes, this is the point I was trying to make.

Let me explain the context for this, so that we can discuss if this is the best solution or if we should find something else, both short term and longer term.

In D55009#1261034, @aokblast wrote:

LGTM for me but I think we should merge the intel's one at the same time.