Re: [tip:x86/mm] x86/mm/mtrr: Clean up mtrr_type_lookup()

Peter Zijlstra
2015-07-31T13:18:13Z

On Wed, May 27, 2015 at 07:19:05AM -0700, tip-bot for Toshi Kani wrote:
> +/**
> + * 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)
> {

> int repeat;
> u64 partial_end;
>
> + 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.
> + */
> + if ((start < 0x100000) &&
> + (mtrr_state.have_fixed) &&
> + (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> + return mtrr_type_lookup_fixed(start, end);
> +
> + /*
> + * Look up the variable ranges. Look of multiple ranges matching
> + * this address and pick type as per MTRR precedence.
> + */
> + type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
>
> /*
> * Common path is with repeat = 0.
> * However, we can have cases where [start:end] spans across some
> + * MTRR ranges and/or the default type. Do repeated lookups for
> + * that case here.
> */
> while (repeat) {
> prev_type = type;
> start = partial_end;
> + type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
>
> if (check_type_overlap(&prev_type, &type))
> return type;
> }
>
> + if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
> + return MTRR_TYPE_WRBACK;
> +
> return type;
> }

So I got staring at this MTRR horror show because I _really_ _Really_
want to kill stop_machine_from_inactive_cpu().

But I wondered about these lookup functions, should they not have an
assertion that preemption is disabled?

Using these functions with preemption enabled is racy against MTRR
updates. And if that race is ok, at the very least explain that it is
indeed racy and why this is not a problem.