Re: [PATCH v4 7/8] L1TFv4 2

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 9 May 2018, speck for ak_at_linux.intel.com wrote:
  
> +static inline u64 maxpa_pfn_bit(int offset)

What is offset for? All callsites I found so far are handing in '1'.

Aside of that the function and the argument name are not really
intuitive. It suggests that it returns a bit number (at least that's how I
read it in the first place) with an address offset from the maximal
physical address while it returns the PFN of

    maximal physical address >> offset

> +{
> +	return BIT_ULL(boot_cpu_data.x86_phys_bits - offset - PAGE_SHIFT);
> +}

> +unsigned long max_swapfile_size(void)
> +{
> +	unsigned long pages;
> +
> +	pages = generic_max_swapfile_size();
> +
> +	if (boot_cpu_has(X86_BUG_L1TF)) {
> +		/* Limit the swap file size to MAX_PA/2 for L1TF workaround */
> +		pages = min_t(unsigned long, maxpa_pfn_bit(1), pages);

So here you use it for comparison as the maximum number of pages which fit
into MAX_PA/2. But in the next patch you use it to check whether the
returned bit mask is set in a pfn. 

> +/*
> +	if ((pfn & maxpa_pfn_bit(1)) && !capable(CAP_SYS_ADMIN))
> +		return false;

Both use cases are correct, but I had to look more than twice.

Aside of that returning u64 from that function is wrong. It works on 64 bit
but on 32bit 

    pages = min_t(unsigned long, maxpa_pfn_bit(1), pages);

will barf because unsigned long != u64.

I'd suggest to make it something like this:

static inline unsigned long l1tf_pfn_limit(void)
{
	return BIT_UL(boot_cpu_data.x86_phys_bits - offset - PAGE_SHIFT)  - 1;
}

and at the call sites then do:

	pages = min_t(unsigned long, l1tf_pfn_limit()  + 1, pages);

and

	if (pfn > l1tf_pnf_limit() && !capable(CAP_SYS_ADMIN))

which makes it obvious what that is about. 

Hmm?

Thanks,

	tglx