| 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
| ||
|
Message-ID: <20150506134127.GE22949@pd.tnic>
Date: Wed, 6 May 2015 15:41:27 +0200
From: Borislav Petkov <bp@...en8.de>
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 v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()
On Tue, Mar 24, 2015 at 04:08:40PM -0600, Toshi Kani 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, which
> do not have to be repeated. Therefore, this patch creates
> separate functions, mtrr_type_lookup_fixed() and
> mtrr_type_lookup_variable(), to handle the fixed and variable
> ranges respectively.
>
> The patch also updates the function headers to clarify the
> return values and output argument. It 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 | 137 +++++++++++++++++++++++-------------
> 1 file changed, 86 insertions(+), 51 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 8bd1298..3652e2b 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -102,55 +102,69 @@ static int check_type_overlap(u8 *prev, u8 *curr)
> return 0;
> }
>
> -/*
> - * Error/Semi-error returns:
> - * MTRR_TYPE_INVALID - 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_fixed - look up memory type in MTRR fixed entries
> + *
> + * MTRR fixed entries are divided into the following ways:
> + * 0x00000 - 0x7FFFF : This range is divided into eight 64KB sub-ranges
> + * 0x80000 - 0xBFFFF : This range is divided into sixteen 16KB sub-ranges
> + * 0xC0000 - 0xFFFFF : This range is divided into sixty-four 4KB sub-ranges
No need for those - simply a pointer to either the SDM or APM manuals'
section suffices as they both describe it good.
> + *
> + * Return Values:
> + * MTRR_TYPE_(type) - Matched memory type
> + * MTRR_TYPE_INVALID - Unmatched or fixed entries are disabled
> */
> -static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> +{
> + int idx;
> +
> + if (start >= 0x100000)
> + return MTRR_TYPE_INVALID;
> +
> + if (!(mtrr_state.have_fixed) ||
> + !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> + return MTRR_TYPE_INVALID;
> +
> + if (start < 0x80000) { /* 0x0 - 0x7FFFF */
> + idx = 0;
> + idx += (start >> 16);
> + return mtrr_state.fixed_ranges[idx];
> +
> + } else if (start < 0xC0000) { /* 0x80000 - 0xBFFFF */
> + idx = 1 * 8;
> + idx += ((start - 0x80000) >> 14);
> + return mtrr_state.fixed_ranges[idx];
> + }
> +
> + /* 0xC0000 - 0xFFFFF */
> + idx = 3 * 8;
> + idx += ((start - 0xC0000) >> 12);
> + return mtrr_state.fixed_ranges[idx];
> +}
> +
> +/**
> + * mtrr_type_lookup_variable - look up memory type in MTRR variable entries
> + *
> + * Return Value:
> + * MTRR_TYPE_(type) - Matched memory type or 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_variable(u64 start, u64 end, u64 *partial_end,
> + int *repeat)
> {
> int i;
> u64 base, mask;
> u8 prev_match, curr_match;
>
> *repeat = 0;
> - if (!mtrr_state_set)
> - return MTRR_TYPE_INVALID;
> -
> - if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
> - return MTRR_TYPE_INVALID;
>
> /* Make end inclusive end, instead of exclusive */
> end--;
>
> - /* Look in fixed ranges. Just return the type as per start */
> - if ((start < 0x100000) &&
> - (mtrr_state.have_fixed) &&
> - (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
> - 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];
> - }
> - }
> -
> - /*
> - * Look in variable ranges
> - * Look of multiple ranges matching this address and pick type
> - * as per MTRR precedence
> - */
> prev_match = MTRR_TYPE_INVALID;
> for (i = 0; i < num_var_ranges; ++i) {
> unsigned short start_state, end_state, inclusive;
> @@ -179,7 +193,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);
> @@ -208,21 +223,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 != MTRR_TYPE_INVALID)
> return prev_match;
>
> return mtrr_state.def_type;
> }
>
> -/*
> - * Returns the effective MTRR type for the region
> - * Error return:
> - * MTRR_TYPE_INVALID - when MTRR is not enabled
> +/**
> + * mtrr_type_lookup - look up memory type in MTRR
> + *
> + * Return Values:
> + * MTRR_TYPE_(type) - The effective MTRR type for the region
> + * MTRR_TYPE_INVALID - MTRR is disabled
> */
> u8 mtrr_type_lookup(u64 start, u64 end)
> {
> @@ -230,22 +242,45 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> int repeat;
> u64 partial_end;
>
> - type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
> + 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)
> + return type;
Huh, why are we not looking at start?
I mean, fixed MTRRs cover the first 1MB so we can simply do:
if ((start < 0x100000) &&
(mtrr_state.have_fixed) &&
(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
return mtrr_type_lookup_fixed(start, end);
and for all the other ranges we would do the variable lookup:
type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
...
?
Although I don't know what the code is supposed to do when a region
starts in the fixed range and overlaps its end, i,e, something like
that:
[ start ... 0x100000 ... end ]
The current code would return a fixed range index and that would be not
really correct.
OTOH, this has been like this forever so maybe we don't care...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
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