[MODERATED] Re: [PATCH v5 6/8] L1TFv4 4

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Wed, May 23, 2018 at 02:51:23PM -0700, speck for Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> Subject:  x86, l1tf: Report if too much memory for L1TF
>  workaround
> 
> If the system has more than MAX_PA/2 physical memory the
> invert page workarounds don't protect the system against
> the L1TF attack anymore, because an inverted physical address
> will point to valid memory.
> 
> We cannot do much here, after all users want to use the
> memory, but at least print a warning and report the system as
> vulnerable in sysfs
> 
> Note this is all extremely unlikely to happen on a real machine
> because they typically have far more MAX_PA than DIMM slots

These paragraphs are missing periods

> Some VMs also report fairly small PAs to guest, e.g. only 36bits.
> In this case the threshold will be lower, but applies only
> to the maximum guest size.
> 
> Since this needs to clear a feature bit that has been forced
> earlier add a special "unforce" macro that supports this.

I didn't see an unforce macro, was it from an earlier version of the
patch?

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> squash! x86, l1tf: Report if too much memory for L1TF workaround

git-rebase casualty?

> v2:
> Do force clearing in setup_clear_...
> Rename variables, fix comments, change formatting.
> Use l1tf_pfn_limit()
> ---
>  arch/x86/kernel/cpu/cpuid-deps.c |  1 +
>  arch/x86/kernel/setup.c          | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
> index 2c0bd38a44ab..ffb178fe6356 100644
> --- a/arch/x86/kernel/cpu/cpuid-deps.c
> +++ b/arch/x86/kernel/cpu/cpuid-deps.c
> @@ -118,4 +118,5 @@ void clear_cpu_cap(struct cpuinfo_x86 *c, unsigned int feature)
>  void setup_clear_cpu_cap(unsigned int feature)
>  {
>  	do_clear_cpu_cap(NULL, feature);
> +	clear_bit(feature, (unsigned long *)cpu_caps_set);

The caps code is a jungle, so who knows... but isn't this what
cpu_caps_cleared is for?  Aren't the cpu_caps_set bits supposed to stay
set (for some mysterious reason)?

AFAICT, the cpu_caps_cleared bit already gets set in do_clear_cpu_cap()
-> clear_feature().  So I don't see that clearing the cpu_caps_set bit
actually accomplishes anything on top of that.

>  }
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 443e31c33fa6..99155e9b28c4 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -785,7 +785,26 @@ static void __init trim_low_memory_range(void)
>  {
>  	memblock_reserve(0, ALIGN(reserve_low, PAGE_SIZE));
>  }
> -	
> +
> +static __init void check_maxpa_memory(void)
> +{
> +	u64 half_pa;
> +
> +	if (!boot_cpu_has(X86_BUG_L1TF))
> +		return;
> +
> +	half_pa = (u64)l1tf_pfn_limit() << PAGE_SHIFT;
> +
> +	/*
> +	 * This is extremely unlikely to happen because almost all systems have far
> +	 * more MAX_PA/2 than DIMM slots.

s/DIMM slots/actual RAM/?

Also is MAX_PA a macro or something else?  I don't see it in the code
anywhere but it's mentioned in several places in these patches
(including the printk below).

> +	 */
> +	if (e820__mapped_any(half_pa, ULLONG_MAX - half_pa, E820_TYPE_RAM)) {
> +		pr_warn("System has more than MAX_PA/2 memory. Disabled L1TF workaround\n");
> +		setup_clear_cpu_cap(X86_FEATURE_L1TF_WA);
> +	}
> +}
> +
>  /*
>   * Dump out kernel offset information on panic.
>   */
> @@ -1022,6 +1041,8 @@ void __init setup_arch(char **cmdline_p)
>  	insert_resource(&iomem_resource, &data_resource);
>  	insert_resource(&iomem_resource, &bss_resource);
>  
> +	check_maxpa_memory();
> +

The function should probably have "l1tf" in its name to make its purpose
more clear without having to read it.

>  	e820_add_kernel_range();
>  	trim_bios_range();
>  #ifdef CONFIG_X86_32
> -- 
> 2.14.3

-- 
Josh