Messages in this thread | | | Date | Mon, 16 Mar 2015 08:58:21 +0100 | From | Ingo Molnar <> | Subject | Re: [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
| |