x86/local_apic.c: Fiddle with thermal LVT slot only if supported

[olce]

Summary

The thermal LVT slot does not necessarily exist.

According to Intel's Software Developers Manual, for Intel processors
supporting 64-bit operation (amd64), probably even the earliest ones
should have a local APIC with such a slot (the slot was introduced with
Pentium 4 and Xeon processors according to the manual, and the 64-bit
implementation in some later versions of them). AMD's Architecture
Programmer's Manual also seems to imply that all AMD processors
supporting amd64 should have the slot too. So this change may not be
needed when i386's code is dropped, but it does not hurt to have it, and
it might ease possible MFCs.

Change the signature of lapic_enable_thermal() so that it can report
failure (if there is no local APIC or if there is no thermal LVT slot).

[bnovkov]

sys/x86/x86/local_apic.c

1677–1679

The separate maxlvt variable here feels a bit redundant, but it seems that a similar pattern is present in lapic_{en,dis}able_pcint.
It would make more sense to roll this into the if statement since lapic_maxlvt is descriptive enough, but I won't insist.

[kib]

I thought that the intent was that the thermal handler is installed only when the CPU supports the interrupt.

[olce]

In D58086#1332368, @kib wrote:

I thought that the intent was that the thermal handler is installed only when the CPU supports the interrupt.

Main problem is that lapic_setup() is executed no matter what, and without this change it unconditionally does a lapic_write32() to the thermal slot, which I think is wrong.

I don't see how a CPU could support ITD without its APIC supporting a thermal interrupt, but the SDM also gives explicit conditions on the presence of the thermal interrupt and those do not refer to ITD at all. The thermal interrupt can be triggered by at least 3 entirely different components, and this is a general API, so... (it's also not general enough as only one handler can be registered, but for now that's fine).

sys/x86/x86/local_apic.c

1677–1679

To me personally, either way is fine, as the assignment is stuck to the if. But I agree the variable is not used for anything else and the compact form is easier to read. Please see D58110.

[olce]

More compact tests on the max LVT slot, based on D58110.

[bnovkov]

LGTM, provided that @kib has no complaints.

[kib]

In D58086#1332506, @olce wrote:

In D58086#1332368, @kib wrote:

I thought that the intent was that the thermal handler is installed only when the CPU supports the interrupt.

Main problem is that lapic_setup() is executed no matter what, and without this change it unconditionally does a lapic_write32() to the thermal slot, which I think is wrong.

Then there are two parts of the change. One is to gate setup (and some other code) with the check for LVT_THERMAL presence. Another is the change to lapic_enable_thermal() to return bool.

IMO the first part is fine, the second is out of line of existing code. As I said, it is the caller duty to ensure that CPU supports thermal interrupt.

[olce]

In D58086#1332799, @kib wrote:

the second is out of line of existing code.

lapic_enable_pcint() does exactly the same.

In D58086#1332799, @kib wrote:

As I said, it is the caller duty to ensure that CPU supports thermal interrupt.

Depends on what you mean exactly here.

The canonical way to detect the thermal interrupt is supported is the number of slots in the local APIC. There seem to be other possible ways according to the SDM (whether that's explicitly written as for the ACPI feature flag, or implied when some components are present, such as ITD), but these look ad-hoc and not as "grounded", so I'd like to avoid them.

I certainly don't think it is up to callers to know about the details on the number of slots and to draw conclusions from this, that's just a layer violation without any practical gain. That said, if you mean you'd rather have a query function exposed by local_apic.c which says to callers whether the slot is supported, and that then there are supposed to call only lapic_{enable,disable}_thermal() after they have checked the query function returned true, I could be fine with that, although I do not see much point in forcing callers to call two separate functions when I expect that most callers will call lapic_enable_*() essentially once (e.g., calling it on resume doesn't matter).