Would appreciate a:
Suggested by: olce
tag line at commit. Thanks!

• Disable interrupts on MSR_OP_LOCAL and non-atomic MSR manipulation.
• Trim and update comments.

In D54833#1256748, @kevans wrote:

rer: removing the prlabel, @csjp noted (paraphrasing, maybe poorly) that that's a MAC-philosophical thing that policy (writers?) shouldn't have to have 'that' much knowledge of kernel internals. It turns out that they often do anyways, but the historically consistent thing is that object labels get passed along with the labels to avoid the policy reaching into the object when it's something that they'll want to act on.

In D54833#1255914, @kevans wrote:

prison_created is done after we know that the jail's going to stick around because I wanted the prison state to be 'final' (post-OSD) before we do any label propagation stuff... maybe that wasn't quite the right call.

The /* Symmetry with prison_created */ comment is indeed welcome to clear the slight but apparently necessary confusion coming from the hook prison_cleanup being called in mac_prison_destroy() while prison_created is not called from mac_prison_init(). :-)

Alternative: Leave sysctl_handle_bool() as is, and created another one, as well as accompanying macros SYSCTL_ADD_xxx() and co., to define booleans compatible with ints. In addition, these could unconditionally output/input an integer, easing back and forth internal transition between int and bool as this would preserve the ABI.

Remove useless initialization of temp_int in the SYSCTL_IN() case.

Update commit message

Update commit message

Prevent users to set hw.acpi.s4bios to true if S4BIOS is not supported.

• Impacts of new prerequisite (D54926)
• Slightly tweak the description of 'hw.acpi.s4bios'

In D48387#1254885, @olce wrote:

(Style: I still see a few spurious whitespaces at end of lines; that could be fixed while in tree.)

Looks good!

I think this warrants a "Relnotes: yes" tag.

Looks good.

In D54831#1253606, @kib wrote:

Mmm... That kind of problem would be automatically avoided if not exporting settings for both 4BSD and ULE, which also changes the sysctl hierarchy.

I want users that tweak schedulers, to put any schedulers' tweaks into their /etc/sysctl.conf, without causing errors if some of the scheduler is not enabled or not configured.
Such users must be intelligent enough to understand that settings for non-active schedulers are not active.
Also I want to be very clear which setting belong to which scheduler.
So I prefer the new scheme.

Almost there! One bug (BUS_PROBE_DEFAULT) and one strongly suggested change (potential bug) around sc->dsm_sets |= DSM_SET_MS;, the rest is mostly cosmetic.

In D54831#1253559, @kib wrote:

Make kern.sched sysctls working when 4BSD is selected:

In D54831#1253557, @kib wrote:

I do not see what the code fragment in cpu_switch.S has to do with sleepqueue. It can only observe either runq lock, or blocked lock, if ever.

In D54831#1253408, @kib wrote:

In D54831#1253398, @olce wrote:

I'd like also that we investigate why the loop on the blocked lock was elided for 4BSD.

Simply because threads are never blocked because there is only one global scheduler lock on 4BSD instead of per-runq lock on ULE. So on ULE you are not guaranteed that the sched_switch() finished with removing the thread from CPU while cpu_switch() already tries to switch to the new context. The blocked state makes this race closed without the need to somehow lock several runqs. On 4BSD it is impossible (global lock).

Minsoo beat me to it.

In D54803#1252956, @lwhsu wrote:

BTW, I suggest that we land this patch first, and continue the discussion at https://bugs.freebsd.org/292615

In D54803#1252819, @cracauer wrote:

In D54803#1252813, @olce wrote:

@cracauer: Default setting for EPP is 50. This was done to mimic what we already do in CPPC support for Intel processors. Can you try setting all dev.hwpstate_amdX.epp to 0 and see if you're recovering your initial behavior (and then possibly play with more values between 0 and 50)?

I tried 0, 10, 25, 37 and 50. The CPU speed stayed at the 11x slower speed in benchmarks.

In D54803#1252817, @lwhsu wrote:

@khng mentions that there is if (resource_disabled(HWP_AMD_CLASSNAME, 0)) return; in hwpstate_identify() so maybe setting hint.hwpstate_amd.disabled="1" in hint file also works?

In D54803#1252816, @cracauer wrote:

I don't do anything to load cpufreq manually, it comes up with the GENERIC kernel.

I'm fine with the patch as it is. It is going to be useful in cases the hardware doesn't do self-tuning properly.

This approach looks weird, and I'm still unclear on what is really needed. More generally, I'd like to know why we have to do that, and I question its safety.

I have a couple of comments, but really we should first determine what to do with D54410, please see there.

In D48387#1251616, @obiwac wrote:

Fix issues pointed out by olce@ and apply some suggestions. Main changes:

• Change DSM order again to match Windows & Linux.
• Hold DSM set info in new struct dsm_set, so we can hold revision ID and DSM name.
• Print out DSM name & revision.
• Tunable to change Intel DSM set revision ID to use for debugging.

In D54409#1250752, @obiwac wrote:

Can totally use smp_rendezvous_cpus() for this instead of adding 2 new IPIs. I thought I had run into a problem using rendezvous when I first implemented this, but I just tried it again and see no problem doing it this way.

Ah, sorry... I'm pretty sure I had seen that problem during the review of D48734 at some point but forgot about it...

Oh, you're right, jails are more of an object than a subject.

We should also avoid the ugly pollution of struct acpi_softc (I completely understand this was done to get something working more quickly). Rather, let's define a new interface, called something like acpi_lpi (defined in a new acpi_lpi_if.m file), with two methods, acpi_lpi_enter() and acpi_lpi_exit(), which would be called before/after entering suspend-to-idle in acpi_EnterSleepState() (e.g., on all direct children of acpi; we'll see that in D48735). There are examples of interfaces under sys/dev/acpica, acpi_if.m and acpi_bus_if.m. The .m files are processed by sys/tools/makeobjops.awk. There's a bit more doc on kobj(9) (which device(9) relies on) in the Architecture Handbook.

Looks good (if you could move the comment about the sleep button, that would be great, see inline comment).

I'm pretty new to device management in FreeBSD, but it looks to me that allocating some memory + acpi_set_private() in DEVICE_PROBE() (here, acpi_spmc_probe()) and acpi_get_private() + deallocation in DEVICE_ATTACH() is a bad pattern, as it will cause memory leaks and in fact even panics in most cases if there are multiple candidate drivers. Since there is for now only a single driver, could you please instead fill up the acpi_spmc_softc directly and have acpi_spmc_probe() return 0 on success (which guarantees preservation of the softc up to DEVICE_ATTACH())? Let's also avoid possible bad pattern spreading by copy-pasting (incidentally, I see the same pattern in acpi_ec.c).

Looks good, except for one thing (see the inline comment in do_sleep()).

I'm not too sure about changing the type of a sysctl knob that has existed for a long time. The only compatibility problem that I can see doing this is potentially breaking reporting in an application that would call sysctl(3) (or sysctlbyname(3)) directly, passing an unitialized integer and then reading from it, as only the first byte would have been filled. Setting the boolean would still work (except on big-endian arches). The use of an old sysctl(8) utility is not affected.

"modified" => "going to be modified"

Cater to comments.

In D54553#1247786, @brooks wrote:

I'm also skeptical (and think both SMALL_SIZE and LARGE_SIZE are over aligned on e.g., armv7), but I'm wary of making functional changes here.

I fail to see any usefulness of CACHE_LARGE_PAD when it was introduced, which corresponds to aligning CACHE_ZONE_LARGE_SIZE to struct namecache_ts. So I'd just drop the corresponding roundup2(), with the benefit of not requiring a struct namecache_ts alignment for CACHE_ZONE_LARGE_SIZE (but then you have to change the corresponding static assertion).