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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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?

But also, why temporary variable? Is there something else that would set
non-zero value that should be preserved?

> +        break;
> +    }
> +
> +    if ( msr == 0 )
> +    {
> +        printk(XENLOG_WARNING "LER disabled: failed to identy MSRs\n");
> +        return;
> +    }
> +
> +    ler_msr = msr;
> +    setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
> +}
> +
>  extern void (*const autogen_entrypoints[X86_NR_VECTORS])(void);
>  void __init trap_init(void)
>  {
> @@ -2226,6 +2216,8 @@ void __init trap_init(void)
>          }
>      }
>  
> +    init_ler();
> +
>      /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
>      this_cpu(gdt_l1e) =
>          l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
> -- 
> 2.39.5
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature