lkml.org 
[lkml]   [2015]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()
    From
    Date
    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 <toshi.kani@hp.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.

    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




    \
     
     \ /
      Last update: 2015-05-06 18:41    [W:2.439 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site