| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 13 Mar 2015 13:37:23 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Toshi Kani <toshi.kani@...com>
Cc: akpm@...ux-foundation.org, hpa@...or.com, tglx@...utronix.de,
mingo@...hat.com, linux-mm@...ck.org, x86@...nel.org,
linux-kernel@...r.kernel.org, dave.hansen@...el.com,
Elliott@...com, pebolle@...cali.nl
Subject: Re: [PATCH v2 3/4] mtrr, x86: Clean up mtrr_type_lookup()
* Toshi Kani <toshi.kani@...com> wrote:
> MTRRs contain fixed and variable entries. mtrr_type_lookup()
> may repeatedly call __mtrr_type_lookup() to handle a request
> that overlaps with variable entries. However,
> __mtrr_type_lookup() also handles the fixed entries and other
> conditions, which do not have to be repeated. This patch moves
> such code from __mtrr_type_lookup() to mtrr_type_lookup().
>
> This patch also changes the 'else if (start < 0x1000000)',
> which checks a fixed range but has an extra zero in the address,
> to 'else' with no condition.
>
> Lastly, the patch updates the function headers to clarify the
> return values and output argument. It also updates comments to
> clarify that the repeating is necessary to handle overlaps with
> the default type, since overlaps with multiple entries alone
> can be handled without such repeating.
>
> There is no functional change in this patch.
>
> Signed-off-by: Toshi Kani <toshi.kani@...com>
> ---
> arch/x86/kernel/cpu/mtrr/generic.c | 102 +++++++++++++++++++-----------------
> 1 file changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index a82e370..ef34a4f 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -102,12 +102,16 @@ static int check_type_overlap(u8 *prev, u8 *curr)
> return 0;
> }
>
> -/*
> - * Error/Semi-error returns:
> - * 0xFF - when MTRR is not enabled
> - * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
> - * corresponds only to [start:*partial_end].
> - * Caller has to lookup again for [*partial_end:end].
> +/**
> + * __mtrr_type_lookup - look up memory type in MTRR variable entries
> + *
> + * Return Value:
> + * memory type - Matched memory type or the default memory type (unmatched)
> + *
> + * Output Argument:
> + * repeat - Set to 1 when [start:end] spanned across MTRR range and type
> + * returned corresponds only to [start:*partial_end]. Caller has
> + * to lookup again for [*partial_end:end].
> */
> static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> {
> @@ -116,42 +120,10 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> u8 prev_match, curr_match;
>
> *repeat = 0;
> - if (!mtrr_state_set)
> - return 0xFF;
> -
> - if (!mtrr_state.enabled)
> - return 0xFF;
>
> /* Make end inclusive end, instead of exclusive */
> end--;
>
> - /* Look in fixed ranges. Just return the type as per start */
> - if (mtrr_state.have_fixed && (start < 0x100000)) {
> - int idx;
> -
> - if (start < 0x80000) {
> - idx = 0;
> - idx += (start >> 16);
> - return mtrr_state.fixed_ranges[idx];
> - } else if (start < 0xC0000) {
> - idx = 1 * 8;
> - idx += ((start - 0x80000) >> 14);
> - return mtrr_state.fixed_ranges[idx];
> - } else if (start < 0x1000000) {
> - idx = 3 * 8;
> - idx += ((start - 0xC0000) >> 12);
> - return mtrr_state.fixed_ranges[idx];
> - }
> - }
> -
> - /*
> - * Look in variable ranges
> - * Look of multiple ranges matching this address and pick type
> - * as per MTRR precedence
> - */
> - if (!(mtrr_state.enabled & 2))
> - return mtrr_state.def_type;
> -
> prev_match = 0xFF;
> for (i = 0; i < num_var_ranges; ++i) {
> unsigned short start_state, end_state, inclusive;
> @@ -180,7 +152,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> * Return the type for first region and a pointer to
> * the start of second region so that caller will
> * lookup again on the second region.
> - * Note: This way we handle multiple overlaps as well.
> + * Note: This way we handle overlaps with multiple
> + * entries and the default type properly.
> */
> if (start_state)
> *partial_end = base + get_mtrr_size(mask);
> @@ -209,21 +182,18 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> return curr_match;
> }
>
> - if (mtrr_tom2) {
> - if (start >= (1ULL<<32) && (end < mtrr_tom2))
> - return MTRR_TYPE_WRBACK;
> - }
> -
> if (prev_match != 0xFF)
> return prev_match;
>
> return mtrr_state.def_type;
> }
>
> -/*
> - * Returns the effective MTRR type for the region
> - * Error return:
> - * 0xFF - when MTRR is not enabled
> +/**
> + * mtrr_type_lookup - look up memory type in MTRR
> + *
> + * Return Values:
> + * memory type - The effective MTRR type for the region
> + * 0xFF - MTRR is disabled
> */
> u8 mtrr_type_lookup(u64 start, u64 end)
> {
> @@ -231,12 +201,43 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> int repeat;
> u64 partial_end;
>
> + if (!mtrr_state_set || !mtrr_state.enabled)
> + return 0xFF;
> +
> + /* Look in fixed ranges. Just return the type as per start */
> + if (mtrr_state.have_fixed && (start < 0x100000)) {
> + int idx;
> +
> + if (start < 0x80000) {
> + idx = 0;
> + idx += (start >> 16);
> + return mtrr_state.fixed_ranges[idx];
> + } else if (start < 0xC0000) {
> + idx = 1 * 8;
> + idx += ((start - 0x80000) >> 14);
> + return mtrr_state.fixed_ranges[idx];
> + } else {
> + idx = 3 * 8;
> + idx += ((start - 0xC0000) >> 12);
> + return mtrr_state.fixed_ranges[idx];
> + }
> + }
So why not put this into a separate helper function - named
mtrr_type_lookup_fixed()? It has little relation to variable ranges.
> +
> + /*
> + * Look in variable ranges
> + * Look of multiple ranges matching this address and pick type
> + * as per MTRR precedence
> + */
> + if (!(mtrr_state.enabled & 2))
> + return mtrr_state.def_type;
> +
> type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
And this then should be named mtrr_type_lookup_variable() or so?
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists