lkml.org 
[lkml]   [2015]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping
    From
    Date
    On Mon, 2015-05-11 at 23:42 +0200, Borislav Petkov wrote:
    > On Mon, May 11, 2015 at 02:38:46PM -0600, Toshi Kani wrote:
    > > MTRRs disabled is not an error case as it could be a normal
    > > configuration on some platforms / BIOS setups.
    >
    > Normal how? PAT-only systems? Examples please...

    BIOS initializes and enables MTRRs at POST. While the most (if not all)
    BIOSes do it today, I do not think the x86 arch requires BIOS to enable
    them.

    Here is a quote from Intel SDM:
    ===
    11.11.5 MTRR Initialization

    On a hardware reset, the P6 and more recent processors clear the valid
    flags in variable-range MTRRs and clear the E flag in the
    IA32_MTRR_DEF_TYPE MSR to disable all MTRRs. All other bits in the MTRRs
    are undefined.

    Prior to initializing the MTRRs, software (normally the system BIOS)
    must initialize all fixed-range and variablerange MTRR register fields
    to 0. Software can then initialize the MTRRs according to known types of
    memory, including memory on devices that it auto-configures.
    Initialization is expected to occur prior to booting the operating
    system.
    ===

    > > I clarified it in the above comment that uniform is set for any return
    > > value.
    >
    > Hell no!
    >
    > u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
    > {
    >
    > ...
    >
    > *uniform = 1;
    >
    > if (!mtrr_state_set)
    > return MTRR_TYPE_INVALID;
    >
    > if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
    > return MTRR_TYPE_INVALID;
    >
    >
    > This is wrong and the fact that I still need to persuade you about it
    > says a lot.
    >
    > If you want to be able to state that a type is uniform even if MTRRs are
    > disabled, you need to define another retval which means exactly that.

    There may not be any type conflict with MTRR_TYPE_INVALID.

    > Or add an inline function called mtrr_enabled() and call it in the
    > mtrr_type_lookup() callers.
    >
    > Or whatever.
    >
    > I don't want any confusing states with two return types and people
    > having to figure out what it exactly means and digging into the code
    > and scratching heads WTF is that supposed to mean.

    I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
    uniform case.

    Thanks,
    -Toshi





    \
     
     \ /
      Last update: 2015-05-12 01:01    [W:2.608 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site