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 <[email protected]>
> ---
> 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.
--
On Wed, 2015-05-06 at 15:41 +0200, Borislav Petkov wrote:
> 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 <[email protected]>
> > ---
> > 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.
Ingo asked me to describe this info here in his review...
> > + *
> > + * 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);
mtrr_type_lookup_fixed() checks the above conditions at entry, and
returns immediately with TYPE_INVALID. I think it is safer to have such
checks in mtrr_type_lookup_fixed() in case there will be multiple
callers.
> 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...
Right, and there is more. As the original code had comment "Just return
the type as per start", which I noticed that I had accidentally removed,
the code only returns the type of the start address. The fixed ranges
have multiple entries with different types. Hence, a given range may
overlap with multiple fixed entries. I will restore the comment in the
function header to clarify this limitation.
Thanks,
-Toshi
On Wed, May 06, 2015 at 10:00:30AM -0600, Toshi Kani wrote:
> Ingo asked me to describe this info here in his review...
Ok.
> mtrr_type_lookup_fixed() checks the above conditions at entry, and
> returns immediately with TYPE_INVALID. I think it is safer to have such
> checks in mtrr_type_lookup_fixed() in case there will be multiple
> callers.
This is not what I mean - I mean to call mtrr_type_lookup_fixed() based
on @start and not unconditionally, like you do.
And there most likely won't be multiple callers because we're phasing
out MTRR use.
And even if there are, they better look at how this function is being
called before calling it. Which I seriously doubt - it is a static
function which you *just* came up with.
> Right, and there is more. As the original code had comment "Just return
> the type as per start", which I noticed that I had accidentally removed,
> the code only returns the type of the start address. The fixed ranges
> have multiple entries with different types. Hence, a given range may
> overlap with multiple fixed entries. I will restore the comment in the
> function header to clarify this limitation.
Ok, let's cleanup this function first and then consider fixing other
possible bugs which haven't been fixed since forever. Again, we might
not even need to address them because we won't be using MTRRs once we
switch to PAT completely, which is what Luis is working on.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Thu, 2015-05-07 at 00:49 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 10:00:30AM -0600, Toshi Kani wrote:
> > Ingo asked me to describe this info here in his review...
>
> Ok.
>
> > mtrr_type_lookup_fixed() checks the above conditions at entry, and
> > returns immediately with TYPE_INVALID. I think it is safer to have such
> > checks in mtrr_type_lookup_fixed() in case there will be multiple
> > callers.
>
> This is not what I mean - I mean to call mtrr_type_lookup_fixed() based
> on @start and not unconditionally, like you do.
>
> And there most likely won't be multiple callers because we're phasing
> out MTRR use.
>
> And even if there are, they better look at how this function is being
> called before calling it. Which I seriously doubt - it is a static
> function which you *just* came up with.
Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
the previous code review. Anyway, let me make sure if I understand your
comment correctly. Do the following changes look right to you?
1) Change the caller responsible for the condition checks.
if ((start < 0x100000) &&
(mtrr_state.have_fixed) &&
(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
return mtrr_type_lookup_fixed(start, end);
2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
are done by the caller. Keep the check with '(start >= 0x100000)' to
assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.
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;
> > Right, and there is more. As the original code had comment "Just return
> > the type as per start", which I noticed that I had accidentally removed,
> > the code only returns the type of the start address. The fixed ranges
> > have multiple entries with different types. Hence, a given range may
> > overlap with multiple fixed entries. I will restore the comment in the
> > function header to clarify this limitation.
>
> Ok, let's cleanup this function first and then consider fixing other
> possible bugs which haven't been fixed since forever. Again, we might
> not even need to address them because we won't be using MTRRs once we
> switch to PAT completely, which is what Luis is working on.
Right.
Thanks,
-Toshi
On Wed, May 06, 2015 at 05:42:10PM -0600, Toshi Kani wrote:
> Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
> the previous code review. Anyway, let me make sure if I understand your
> comment correctly. Do the following changes look right to you?
>
> 1) Change the caller responsible for the condition checks.
>
> if ((start < 0x100000) &&
> (mtrr_state.have_fixed) &&
> (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> return mtrr_type_lookup_fixed(start, end);
>
> 2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
> are done by the caller. Keep the check with '(start >= 0x100000)' to
> assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.
That is a good defensive measure.
> 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;
Yeah, that's what I mean.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Thu, 2015-05-07 at 09:52 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 05:42:10PM -0600, Toshi Kani wrote:
> > Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
> > the previous code review. Anyway, let me make sure if I understand your
> > comment correctly. Do the following changes look right to you?
> >
> > 1) Change the caller responsible for the condition checks.
> >
> > if ((start < 0x100000) &&
> > (mtrr_state.have_fixed) &&
> > (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> > return mtrr_type_lookup_fixed(start, end);
> >
> > 2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
> > are done by the caller. Keep the check with '(start >= 0x100000)' to
> > assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.
>
> That is a good defensive measure.
>
> > 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;
>
> Yeah, that's what I mean.
Thanks for the clarification! Will change accordingly.
-Toshi