Re: [PATCH for-4.20] x86/traps: Rework LER initialisation and support Zen5/Diamond Rapids

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/12/2024 10:25 pm, Marek Marczykowski-Górecki wrote:
> On Tue, Dec 31, 2024 at 07:20:02PM +0000, Andrew Cooper wrote:
>> AMD have always used the architectural MSRs for LER.  As the first processor
>> to support LER was the K7 (which was 32bit), we can assume it's presence
>> unconditionally in 64bit mode.
>>
>> Intel are about to run out of space in Family 6 and start using 19.  It is
>> only the Pentium 4 which uses non-architectural LER MSRs.
>>
>> percpu_traps_init(), which runs on every CPU, contains a lot of code which
>> should be init-only, and is the only reason why opt_ler can't be in initdata.
>>
>> Write a brand new init_ler() which expects all future Intel and AMD CPUs to
>> continue using the architectural MSRs, and does all setup together.  Call it
>> from trap_init(), and remove the setup logic percpu_traps_init() except for
>> the single path configuring MSR_IA32_DEBUGCTLMSR.
>>
>> Leave behind a warning if the user asked for LER and Xen couldn't enable it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>
>> For 4.20.  This is needed for Zen5 CPUs (already available) as well as Intel
>> CPUs coming shortly.  It also moves some non-init logic to being init-only.
>> ---
> ...
>
>> @@ -2201,6 +2155,42 @@ void __init init_idt_traps(void)
>>          this_cpu(compat_gdt) = boot_compat_gdt;
>>  }
>>  
>> +static void __init init_ler(void)
>> +{
>> +    unsigned int msr = 0;
>> +
>> +    if ( !opt_ler )
>> +        return;
>> +
>> +    /*
>> +     * Intel Pentium 4 is the only known CPU to not use the architectural 
>> MSR
>> +     * indicies.
>> +     */
>> +    switch ( boot_cpu_data.x86_vendor )
>> +    {
>> +    case X86_VENDOR_INTEL:
>> +        if ( boot_cpu_data.x86 == 0xf )
>> +        {
>> +            ler_msr = MSR_P4_LER_FROM_LIP;
> msr = 
>
> and ...
>
>> +            break;
>> +        }
>> +        fallthrough;
>> +    case X86_VENDOR_AMD:
>> +    case X86_VENDOR_HYGON:
>> +        ler_msr = MSR_IA32_LASTINTFROMIP;
> ... here?

Yes.  Serves me right for a last minute refactor.

> But also, why temporary variable?

Code gen (ler_msr is a global so checking it at the end needs to come
from memory), and defence in depth (it introduces a path through the
function where we failed to find the MSR but still turned LER on anyway
if there was an unexpected value there already).

~Andrew