On June 3, 2025 10:23:42 AM PDT, Dave Hansen <dave.han...@intel.com> wrote:
>On 5/13/25 13:37, Sohil Mehta wrote:
>...
>> + * Vector 2 is reserved for external NMIs related to the Local APIC -
>> + * LINT1. Some third-party chipsets may send NMI messages with a
>> + * hardcoded vector of 2, which would result in bit 2 being set in the
>> + * NMI-source bitmap.
>
>This doesn't actually say what problem this causes. Is this better?
>
> Third-party chipsets send NMI messages with a fixed vector of 2.
> Using vector 2 for some other purpose would cause confusion
> between those Local APIC messages and the other purpose. Avoid
> using it.
>
>> + * The vectors are in no particular priority order. Add new vector
>> + * assignments sequentially in the list below.
>> + */
>> +#define NMIS_VECTOR_NONE 0 /* Reserved - Set for all unidentified
>> sources */
>> +#define NMIS_VECTOR_TEST 1 /* NMI selftest */
>> +#define NMIS_VECTOR_EXTERNAL 2 /* Reserved - Match External
>> NMI vector 2 */
>> +#define NMIS_VECTOR_SMP_STOP 3 /* Panic stop CPU */
>> +#define NMIS_VECTOR_BT 4 /* CPU backtrace */
>> +#define NMIS_VECTOR_KGDB 5 /* Kernel debugger */
>> +#define NMIS_VECTOR_MCE 6 /* MCE injection */
>> +#define NMIS_VECTOR_PMI 7 /* PerfMon counters */
>> +
>> +#define NMIS_VECTORS_MAX 16 /* Maximum number of NMI-source vectors
>> */
>
>Would an enum fit here?
>
>You could also add a:
>
> NMIS_VECTOR_COUNT
>
>as the last entry and then just:
>
> BUILD_BUG_ON(NMIS_VECTOR_COUNT >= 16);
>
>somewhere.
>
>I guess it's a little annoying that you need NMIS_VECTOR_EXTERNAL to
>have a fixed value of 2, but I do like way the enum makes the type explicit.
>
>
>> static int __init register_nmi_cpu_backtrace_handler(void)
>> {
>> - register_nmi_handler(NMI_LOCAL, nmi_cpu_backtrace_handler, 0,
>> "arch_bt", 0);
>> + register_nmi_handler(NMI_LOCAL, nmi_cpu_backtrace_handler, 0,
>> "arch_bt", NMIS_VECTOR_BT);
>> return 0;
>> }
>
>... Oh you replaced _most_ of the random 0's in this patch. That helps
>for sure.
The "may" here is important. *Most* sources are expected to send these as
either a programmable MSI or as LINT1, which is programmable in the APIC, but
since the vector hasn't mattered, we can't rule out that some hardware might
have been built to send an MSI with a hard-coded vector, in which case it is
most likely they send either 0, 2 or 255. 0 and 255 will set the unknown source
bit (0), but 2 is a legitimate source so it is defensive programming to leave
it fallow.
Note also that although the current implementation is limited to 15 sources (+
the unknown/lost source bit), the architecture allows for up to 63.
