Re: [PATCH v1.1 6/6] x86/apic: Convert the TSC deadline errata table to X86_MATCH_*()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/07/2025 3:06 pm, Jan Beulich wrote:
> On 18.07.2025 12:55, Andrew Cooper wrote:
>> On 18/07/2025 11:23 am, Andrew Cooper wrote:
>>> On 18/07/2025 11:19 am, Jan Beulich wrote:
>>>> On 18.07.2025 12:07, Andrew Cooper wrote:
>>>>> With the ability to match on steppings, introduce a new X86_MATCH_VFMS()
>>>>> helper to match a specific stepping, and use it to rework 
>>>>> deadline_match[].
>>>>>
>>>>> Notably this removes the overloading of driver_data possibly being a 
>>>>> function
>>>>> pointer, and removes the latent bug where the target functions are missing
>>>>> ENDBR instructions owing to the lack of the cf_check attribute.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> Thanks.
>> Actually, this isn't as no-functional-change as I thought.
>>
>> X86_FEATURE_TSC_DEADLINE has been swapped for X86_FEATURE_ANY in the table.
>>
>> check_deadline_errata() is called unconditionally, without checking for
>> TSC_DEADLINE, yet the rows in the table are the CPUs for which an
>> erratum is known, so they all have the feature.
>>
>> It does make a difference if e.g. one were to boot with
>> cpuid=no-tsc-deadline.  Previously we'd have exited early, while now
>> we'll emit the warning.
>>
>> We could switch back to using TSC_DEADLINE (requiring a more complicated
>> X86_MATCH_*() wrapper), although a better option would be to predicate
>> the call to check_deadline_errata() with a feature check, because it's a
>> much more recent addition to AMD CPUs, and there's no point searching
>> the errata list on CPUs which lack the feature.
> To be honest in this case I'd be fine with either adjustment. Switching the
> feature back is more consistent with the overall purpose of X86_MATCH_*(),
> but as you say a table with every entry having the same feature named isn't
> very useful to go through when the feature isn't there.
>
> One option to keep things table based, yet still avoiding to run through
> the entire table, would be to allow for a "negative" entry (which here
> would simply be placed first in the table).

Except that would be an example of { ANY, ANY, ANY, ANY,
ALT_NOT(TSC_DEADLINE) } which we can't express currently, and we also
said we didn't want to get into the habit of.

I'm going with one extra boot_cpu_has() check, and an expanded commit
message.

~Andrew