Re: [RFC PATCH] x86/amd: Add support for AMD TCE

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2025 14:44, Teddy Astie wrote:
> AMD Translation Cache Extension is a flag that can be enabled in the EFER MSR 
> to optimize
> some TLB flushes. Enable this flag if supported by hardware.
> 
> AMD Architecture Developer Manual describe this feature as follow
>> Setting this bit to 1 changes how the INVLPG, INVLPGB, and INVPCID 
>> instructions operate
>> on TLB entries. When this bit is 0, these instructions remove the target PTE 
>> from the
>> TLB as well as all upper-level table entries that are cached in the TLB, 
>> whether or not
>> they are associated with the target PTE. When this bit is set, these 
>> instructions will
>> remove the target PTE and only those upper-level entries that lead to the 
>> target PTE in
>> the page table hierarchy, leaving unrelated upper-level entries intact. This 
>> may provide
>> a performance benefit.
> 
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> ---
> RFC:
>  - is this change actually safe ?

Well, before getting here with reading I was already about to ask this very
question. It's really you who needs to prove it.

>  - should we add a tce/no-tce option to opt-in/out this feature ?

Unless we're entirely certain we got this right and didn't overlook any
corner case, perhaps better to do so.

>  - is this flag enabled at the right moment during boot ?

If (as you appear to take as a base assumption) our code is safe towards
this behavioral change, then it would be largely irrelevant when you set
the bit. So to answer this question the first point above needs sorting.


> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -114,6 +114,7 @@ static inline bool boot_cpu_has(unsigned int feat)
>  #define cpu_has_xop             boot_cpu_has(X86_FEATURE_XOP)
>  #define cpu_has_skinit          boot_cpu_has(X86_FEATURE_SKINIT)
>  #define cpu_has_fma4            boot_cpu_has(X86_FEATURE_FMA4)
> +#define cpu_has_tce             boot_cpu_has(X86_FEATURE_TCE)

If you add this, ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -2008,6 +2008,13 @@ void asmlinkage __init noreturn __start_xen(void)
>      if ( cpu_has_pku )
>          set_in_cr4(X86_CR4_PKE);
>  
> +    if ( boot_cpu_has(X86_FEATURE_TCE) )

... the please also use it.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -372,6 +372,9 @@ void asmlinkage start_secondary(void *unused)
>  
>      microcode_update_one();
>  
> +    if ( boot_cpu_has(X86_FEATURE_TCE) )
> +        write_efer(read_efer() | EFER_TCE);

Same here. But I wonder if you couldn't set the bit in trampoline_efer.

Jan