Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems

[Borislav Petkov]

On Tue, Jun 20, 2017 at 10:22:16PM -0700, Andy Lutomirski wrote:
> We can use PCID if the CPU has PCID and PGE and we're not on Xen.
> 
> By itself, this has no effect.  The next patch will start using
> PCID.
> 
> Cc: Juergen Gross <jgross@xxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/tlbflush.h |  8 ++++++++
>  arch/x86/kernel/cpu/common.c    | 15 +++++++++++++++
>  arch/x86/xen/enlighten_pv.c     |  6 ++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index 87b13e51e867..57b305e13c4c 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -243,6 +243,14 @@ static inline void __flush_tlb_all(void)
>  		__flush_tlb_global();
>  	else
>  		__flush_tlb();
> +
> +	/*
> +	 * Note: if we somehow had PCID but not PGE, then this wouldn't work --
> +	 * we'd end up flushing kernel translations for the current ASID but
> +	 * we might fail to flush kernel translations for other cached ASIDs.
> +	 *
> +	 * To avoid this issue, we force PCID off if PGE is off.
> +	 */
>  }
>  
>  static inline void __flush_tlb_one(unsigned long addr)
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 904485e7b230..01caf66b270f 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1143,6 +1143,21 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  	setup_smep(c);
>  	setup_smap(c);
>  
> +	/* Set up PCID */
> +	if (cpu_has(c, X86_FEATURE_PCID)) {
> +		if (cpu_has(c, X86_FEATURE_PGE)) {

What are we protecting ourselves here against? Funny virtualization guests?

Because PGE should be ubiquitous by now. Or have you heard something?

> +			cr4_set_bits(X86_CR4_PCIDE);
> +		} else {
> +			/*
> +			 * flush_tlb_all(), as currently implemented, won't
> +			 * work if PCID is on but PGE is not.  Since that
> +			 * combination doesn't exist on real hardware, there's
> +			 * no reason to try to fully support it.
> +			 */
> +			clear_cpu_cap(c, X86_FEATURE_PCID);
> +		}
> +	}

This whole in setup_pcid() I guess, like the rest of the features.

> +
>  	/*
>  	 * The vendor-specific functions might have changed features.
>  	 * Now we do "generic changes."
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index f33eef4ebd12..a136aac543c3 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -295,6 +295,12 @@ static void __init xen_init_capabilities(void)
>  	setup_clear_cpu_cap(X86_FEATURE_ACC);
>  	setup_clear_cpu_cap(X86_FEATURE_X2APIC);
>  
> +	/*
> +	 * Xen PV would need some work to support PCID: CR3 handling as well
> +	 * as xen_flush_tlb_others() would need updating.
> +	 */
> +	setup_clear_cpu_cap(X86_FEATURE_PCID);
> +
>  	if (!xen_initial_domain())
>  		setup_clear_cpu_cap(X86_FEATURE_ACPI);
>  
> -- 
> 2.9.4
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.