Re: [PATCH] x86/mm: resize user_pcid_flush_mask for PTI / broadcast TLB flush combination

[Ingo Molnar]

* Rik van Riel <riel@xxxxxxxxxxx> wrote:

> @@ -121,7 +140,11 @@ struct tlb_state {
>  	 * the corresponding user PCID needs a flush next time we
>  	 * switch to it; see SWITCH_TO_USER_CR3.
>  	 */
> +#if defined(CONFIG_X86_TLB_BROADCAST_TLB_FLUSH) && defined(CONFIG_MITIGATION_PAGE_TABLE_ISOLATION)
> +	unsigned long user_pcid_flush_mask[(1 << CR3_AVAIL_PCID_BITS) / BITS_PER_LONG];
> +#else
>  	unsigned short user_pcid_flush_mask;
> +#endif

1)

CONFIG_X86_TLB_BROADCAST_TLB_FLUSH doesn't actually exist, the name is 
CONFIG_BROADCAST_TLB_FLUSH.

This patch could not possibly have been tested on a vanilla kernel on 
actual hardware in the PTI usecase it claims to fix...

2)

Testing aside, this definition and the usage of user_pcid_flush_mask is 
unnecessarily confusing, as it also defines user_pcid_flush_mask when 
it's not actually used by runtime code. user_pcid_flush_mask is only 
used if CONFIG_MITIGATION_PAGE_TABLE_ISOLATION=y, so as an interim step 
we could make this a more obvious:

   #ifdef CONFIG_MITIGATION_PAGE_TABLE_ISOLATION
   # ifdef CONFIG_BROADCAST_TLB_FLUSH
	unsigned long user_pcid_flush_mask[(1 << CR3_AVAIL_PCID_BITS) / BITS_PER_LONG];
   # else
	unsigned short user_pcid_flush_mask;
   # endif
   #endif

And wrap the body of invalidate_user_asid() in an #ifdef 
CONFIG_MITIGATION_PAGE_TABLE_ISOLATION. The entry assembly code is 
already under such an #ifdef.

And once we do that it becomes obvious that the two definitions of 
user_pcid_flush_mask can be merged by doing something like:

   #ifdef CONFIG_BROADCAST_TLB_FLUSH
   # define CR3_AVAIL_PCID_LONGS ((1 << CR3_AVAIL_PCID_BITS) / BITS_PER_LONG)
   #else
   # define CR3_AVAIL_PCID_LONGS 1
   #endif

   #ifdef CONFIG_MITIGATION_PAGE_TABLE_ISOLATION
   unsigned long user_pcid_flush_mask[CR3_AVAIL_PCID_LONGS];
   #endif

(Or so, if I counted my bits and longs right.)

And we can drop the ugly & fragile type cast in invalidate_user_asid():

-	__set_bit(kern_pcid(asid),
-		  (unsigned long *)this_cpu_ptr(&cpu_tlbstate.user_pcid_flush_mask));

+	__set_bit(kern_pcid(asid), this_cpu_ptr(cpu_tlbstate.user_pcid_flush_mask));

And yeah, this means user_pcid_flush_mask is a long on 
!CONFIG_BROADCAST_TLB_FLUSH kernels, while it was a short before.
Literally nobody cares, because it's enabled on all distro kernels that 
have CPU_SUP_AMD:

  config BROADCAST_TLB_FLUSH 
        def_bool y
        depends on CPU_SUP_AMD && 64BIT

Literally no Linux distribution is going to have this option disabled.

Ie. we'd be uglifying and obfuscating the code for a config that people 
aren't actually using...

3)

If we are going to grow user_pcid_flush_mask from 2 bytes to 256 bytes 
then please reorder 'struct tlb_state' for cache efficiency: at minimum 
the ::cr4 shadow should move to before ::user_pcid_flush_mask. But I 
think we should probably move user_pcid_flush_mask to the end of the 
structure, where it does the least damage to cache layout.

Thanks,

	Ingo