acpi_spmc(4): Gracefully support only the Microsoft DSM
ClosedPublic
Actions

Authored by olce on Mon, May 4, 8:32 PM.

Details

Reviewers

obiwac
imp

Commits

rG921a6d6c38f5: acpi_spmc(4): Gracefully support a standalone Microsoft DSM

Summary

In acpi_spmc_get_constraints(), stop assuming that if there is no AMD
DSM, then the Intel one is present. Although this is likely to be the
overwhelming majority of cases on amd64, there is no technical reason
nor constraint in our code that really needs assuming that. On (so far
hypothetical) machines with only the Microsoft DSM, this assumption
would cause a cryptic and irrelevant error message (and, prior to the
previous commit, a panic on INVARIANTS).

Warn the user if both the Intel and AMD DSMs are present, and use the
constraints reported by the Intel one (see the comment for why).

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 72898
Build 69781: arc lint + arc unit

Event Timeline

olce created this revision.Mon, May 4, 8:32 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, May 4, 8:32 PM

olce requested review of this revision.Mon, May 4, 8:32 PM

Harbormaster completed remote builds in B72798: Diff 177204.Mon, May 4, 8:33 PM

olce added a parent revision: D56812: acpi_spmc(4): INVARIANTS: Do not panic on getting constraints failure.Mon, May 4, 8:40 PM

olce added a child revision: D56814: acpi_spmc(4): In-kernel strdup() cannot fail, remove dead code.

imp accepted this revision.Thu, May 7, 2:38 AM

This revision is now accepted and ready to land.Thu, May 7, 2:38 AM

Tolerate the presence of both AMD and Intel DSMs by favoring Intel's (should never happen).
Mention that min_d_state is a single field for a piece of information provided by both, so we cannot store constraints from both (that's not the only reason though, and I might update the text again on commit).

This revision now requires review to proceed.Thu, May 7, 8:11 PM

Harbormaster completed remote builds in B72898: Diff 177403.Thu, May 7, 8:11 PM

obiwac accepted this revision.Mon, May 11, 8:59 AM

obiwac added inline comments.

sys/dev/acpica/acpi_spmc.c
624–626	maybe a comment to tell the user to check if hell has frozen over ;)
630	in this situation, maybe we wanna have `get_constraints_succeeded` compile on non-`INVARIANTS`, and then just return early from `acpi_spmc_check_constraints()` if this is false? in case we just have the microsoft dsm

This revision is now accepted and ready to land.Mon, May 11, 8:59 AM

obiwac added inline comments.Mon, May 11, 9:03 AM

sys/dev/acpica/acpi_spmc.c
630	I know currently this will still work because `sc->constraints_count == 0`, but then since `violation == false`, it will print that the constraints are respected. We should probably just not print anything in that situation

olce marked 3 inline comments as done.Mon, May 11, 4:10 PM

olce added inline comments.

sys/dev/acpica/acpi_spmc.c
624–626	Ha ha
630	I see, but then it seems a bit more useful to just handle the `sc->constraints_count == 0` instead, in case some firmware manufacturer does not report any constraints. Going to add an early bail out to the relevant commit.