Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov <bp@alien8.de>]

On Tue, Mar 24, 2015 at 04:08:41PM -0600, Toshi Kani wrote:
> This patch adds an additional argument, 'uniform', to
> mtrr_type_lookup(), which returns 1 when a given range is
> covered uniformly by MTRRs, i.e. the range is fully covered
> by a single MTRR entry or the default type.
> 
> pud_set_huge() and pmd_set_huge() are changed to check the
> new 'uniform' flag to see if it is safe to create a huge page
> mapping to the range.  This allows them to create a huge page
> mapping to a range covered by a single MTRR entry of any
> memory type.  It also detects a non-optimal request properly.
> They continue to check with the WB type since the WB type has
> no effect even if a request spans multiple MTRR entries.
> 
> pmd_set_huge() logs a warning message to a non-optimal request
> so that driver writers will be aware of such a case.  Drivers
> should make a mapping request aligned to a single MTRR entry
> when the range is covered by MTRRs.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/include/asm/mtrr.h        |    5 +++--
>  arch/x86/kernel/cpu/mtrr/generic.c |   35 +++++++++++++++++++++++++++--------
>  arch/x86/mm/pat.c                  |    4 ++--
>  arch/x86/mm/pgtable.c              |   25 +++++++++++++++----------
>  4 files changed, 47 insertions(+), 22 deletions(-)

...

> @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>   * Return Values:
>   * MTRR_TYPE_(type)  - The effective MTRR type for the region
>   * MTRR_TYPE_INVALID - MTRR is disabled
> + *
> + * Output Argument:
> + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> + *	     is fully covered by a single MTRR entry or the default type.

I'd call this "single_mtrr". "uniform" could also mean that the resulting
type is uniform, i.e. of the same type but spanning multiple MTRRs.

>   */
> -u8 mtrr_type_lookup(u64 start, u64 end)
> +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
>  {
> -	u8 type, prev_type;
> +	u8 type, prev_type, is_uniform, dummy;
>  	int repeat;
>  	u64 partial_end;
>  
> +	*uniform = 1;
> +

You're setting it here...

>  	if (!mtrr_state_set)
>  		return MTRR_TYPE_INVALID;

... but if you return here, you would've changed the thing uniform
points to needlessly as you're returning an error.

> @@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
>  	 * the variable ranges.
>  	 */
>  	type = mtrr_type_lookup_fixed(start, end);
> -	if (type != MTRR_TYPE_INVALID)
> +	if (type != MTRR_TYPE_INVALID) {
> +		*uniform = 0;
>  		return type;
> +	}
>  
>  	/*
>  	 * Look up the variable ranges.  Look of multiple ranges matching
>  	 * this address and pick type as per MTRR precedence.
>  	 */
> -	type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
> +	type = mtrr_type_lookup_variable(start, end, &partial_end,
> +					 &repeat, &is_uniform);
>  
>  	/*
>  	 * Common path is with repeat = 0.
> @@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
>  	while (repeat) {
>  		prev_type = type;
>  		start = partial_end;
> +		is_uniform = 0;

So I think it would be better if you added an out: label where you do
exit from the function and set return values there.

So something like that, I'm pasting the whole function here so that you
can follow better:

u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
{
        u8 type, prev_type, is_uniform = 1, dummy;
        int repeat;
        u64 partial_end;

        if (!mtrr_state_set)
                return MTRR_TYPE_INVALID;

        if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
                return MTRR_TYPE_INVALID;

        /*
         * Look up the fixed ranges first, which take priority over
         * the variable ranges.
         */
        type = mtrr_type_lookup_fixed(start, end);
        if (type != MTRR_TYPE_INVALID) {
                is_uniform = 0;
                goto out;
        }

        /*
         * Look up the variable ranges.  Look of multiple ranges matching
         * this address and pick type as per MTRR precedence.
         */
        type = mtrr_type_lookup_variable(start, end, &partial_end,
                                         &repeat, &is_uniform);

        /*
         * Common path is with repeat = 0.
         * However, we can have cases where [start:end] spans across some
         * MTRR ranges and/or the default type.  Do repeated lookups for
         * that case here.
         */
        while (repeat) {
                prev_type = type;
                start = partial_end;
                is_uniform = 0;

                type = mtrr_type_lookup_variable(start, end, &partial_end,
                                                 &repeat, &dummy);

                if (check_type_overlap(&prev_type, &type))
                        goto out;

        }

        if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
                type = MTRR_TYPE_WRBACK;

out:
        *uniform = is_uniform;
        return type;
}
---

This way you're setting the uniform pointer in a single location and you're
working with the local variable inside the function.

Much easier to follow.

> +
>  		type = mtrr_type_lookup_variable(start, end, &partial_end,
> -						 &repeat);
> +						 &repeat, &dummy);
>  
> -		if (check_type_overlap(&prev_type, &type))
> +		if (check_type_overlap(&prev_type, &type)) {
> +			*uniform = 0;
>  			return type;
> +		}
>  	}
>  
>  	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
>  		return MTRR_TYPE_WRBACK;
>  
> +	*uniform = is_uniform;
>  	return type;
>  }
>  
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 35af677..372ad42 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -267,9 +267,9 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end,
>  	 * request is for WB.
>  	 */
>  	if (req_type == _PAGE_CACHE_MODE_WB) {
> -		u8 mtrr_type;
> +		u8 mtrr_type, uniform;
>  
> -		mtrr_type = mtrr_type_lookup(start, end);
> +		mtrr_type = mtrr_type_lookup(start, end, &uniform);
>  		if (mtrr_type != MTRR_TYPE_WRBACK)
>  			return _PAGE_CACHE_MODE_UC_MINUS;
>  
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index cfca4cf..3d6edea 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -567,17 +567,18 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
>   * pud_set_huge - setup kernel PUD mapping
>   *
>   * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
>   *
>   * Return 1 on success, and 0 when no PUD was set.
>   */
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>  {
> -	u8 mtrr;
> +	u8 mtrr, uniform;
>  
> -	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> -	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> +	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
> +	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
>  		return 0;
>  
>  	prot = pgprot_4k_2_large(prot);
> @@ -593,18 +594,22 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>   * pmd_set_huge - setup kernel PMD mapping
>   *
>   * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
>   *
>   * Return 1 on success, and 0 when no PMD was set.
>   */
>  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>  {
> -	u8 mtrr;
> +	u8 mtrr, uniform;
>  
> -	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
> -	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> +	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> +	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
> +		pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
> +				addr, addr + PMD_SIZE);
>  		return 0;

So this returns 0, i.e. failure already. Why do we even have to warn?
Caller already knows it failed.

And this warning would flood dmesg needlessly.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--