lkml.org 
[lkml]   [2015]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 4/5] mtrr, x86: Clean up mtrr_type_lookup()

    * Toshi Kani <toshi.kani@hp.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, 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.

    Nice cleanup!

    I also suggest adding a small table to the comments before the
    function, that lists the fixed purpose MTRRs and their address ranges
    - to make it more obvious what the magic hexadecimal constants within
    the code are doing.

    > +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
    > +{
    > + int idx;
    > +
    > + if (start >= 0x100000)
    > + return 0xFF;

    Btw., as a separate cleanup patch, we should probably also change
    '0xFF' (which is sometimes written as 0xff) to be some sufficiently
    named constant, and explain its usage somewhere?

    > + if (!(mtrr_state.have_fixed) ||
    > + !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))

    Btw., can MTRR_STATE_MTRR_FIXED_ENABLED ever be set in
    mtrr_state.enabled, without mtrr_state.have_fixed being set?

    AFAICS get_mtrr_state() will only ever fill in mtrr_state with fixed
    MTRRs if mtrr_state.have_fixed != 0 - but I might be mis-reading the
    (rather convoluted) flow of code ...

    Thanks,

    Ingo


    \
     
     \ /
      Last update: 2015-03-16 09:01    [W:3.988 / U:0.828 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site