On Tue, Mar 24, 2015 at 04:08:41PM -0600, Toshi Kani wrote:
> This patch adds an additional argument, 'uniform', to
> mtrr_type_lookup(), which returns 1 when a given range is
> covered uniformly by MTRRs, i.e. the range is fully covered
> by a single MTRR entry or the default type.
>
> pud_set_huge() and pmd_set_huge() are changed to check the
> new 'uniform' flag to see if it is safe to create a huge page
> mapping to the range. This allows them to create a huge page
> mapping to a range covered by a single MTRR entry of any
> memory type. It also detects a non-optimal request properly.
> They continue to check with the WB type since the WB type has
> no effect even if a request spans multiple MTRR entries.
>
> pmd_set_huge() logs a warning message to a non-optimal request
> so that driver writers will be aware of such a case. Drivers
> should make a mapping request aligned to a single MTRR entry
> when the range is covered by MTRRs.
>
> Signed-off-by: Toshi Kani <[email protected]>
> ---
> arch/x86/include/asm/mtrr.h | 5 +++--
> arch/x86/kernel/cpu/mtrr/generic.c | 35 +++++++++++++++++++++++++++--------
> arch/x86/mm/pat.c | 4 ++--
> arch/x86/mm/pgtable.c | 25 +++++++++++++++----------
> 4 files changed, 47 insertions(+), 22 deletions(-)
...
> @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> * Return Values:
> * MTRR_TYPE_(type) - The effective MTRR type for the region
> * MTRR_TYPE_INVALID - MTRR is disabled
> + *
> + * Output Argument:
> + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> + * is fully covered by a single MTRR entry or the default type.
I'd call this "single_mtrr". "uniform" could also mean that the resulting
type is uniform, i.e. of the same type but spanning multiple MTRRs.
> */
> -u8 mtrr_type_lookup(u64 start, u64 end)
> +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
> {
> - u8 type, prev_type;
> + u8 type, prev_type, is_uniform, dummy;
> int repeat;
> u64 partial_end;
>
> + *uniform = 1;
> +
You're setting it here...
> if (!mtrr_state_set)
> return MTRR_TYPE_INVALID;
... but if you return here, you would've changed the thing uniform
points to needlessly as you're returning an error.
> @@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> * the variable ranges.
> */
> type = mtrr_type_lookup_fixed(start, end);
> - if (type != MTRR_TYPE_INVALID)
> + if (type != MTRR_TYPE_INVALID) {
> + *uniform = 0;
> return type;
> + }
>
> /*
> * 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);
> + type = mtrr_type_lookup_variable(start, end, &partial_end,
> + &repeat, &is_uniform);
>
> /*
> * Common path is with repeat = 0.
> @@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> while (repeat) {
> prev_type = type;
> start = partial_end;
> + is_uniform = 0;
So I think it would be better if you added an out: label where you do
exit from the function and set return values there.
So something like that, I'm pasting the whole function here so that you
can follow better:
u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
{
u8 type, prev_type, is_uniform = 1, dummy;
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.
*/
type = mtrr_type_lookup_fixed(start, end);
if (type != MTRR_TYPE_INVALID) {
is_uniform = 0;
goto out;
}
/*
* 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, &is_uniform);
/*
* 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;
is_uniform = 0;
type = mtrr_type_lookup_variable(start, end, &partial_end,
&repeat, &dummy);
if (check_type_overlap(&prev_type, &type))
goto out;
}
if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
type = MTRR_TYPE_WRBACK;
out:
*uniform = is_uniform;
return type;
}
---
This way you're setting the uniform pointer in a single location and you're
working with the local variable inside the function.
Much easier to follow.
> +
> type = mtrr_type_lookup_variable(start, end, &partial_end,
> - &repeat);
> + &repeat, &dummy);
>
> - if (check_type_overlap(&prev_type, &type))
> + if (check_type_overlap(&prev_type, &type)) {
> + *uniform = 0;
> return type;
> + }
> }
>
> if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
> return MTRR_TYPE_WRBACK;
>
> + *uniform = is_uniform;
> return type;
> }
>
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 35af677..372ad42 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -267,9 +267,9 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end,
> * request is for WB.
> */
> if (req_type == _PAGE_CACHE_MODE_WB) {
> - u8 mtrr_type;
> + u8 mtrr_type, uniform;
>
> - mtrr_type = mtrr_type_lookup(start, end);
> + mtrr_type = mtrr_type_lookup(start, end, &uniform);
> if (mtrr_type != MTRR_TYPE_WRBACK)
> return _PAGE_CACHE_MODE_UC_MINUS;
>
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index cfca4cf..3d6edea 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -567,17 +567,18 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
> * pud_set_huge - setup kernel PUD mapping
> *
> * MTRR can override PAT memory types with 4KB granularity. Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR. MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
> *
> * Return 1 on success, and 0 when no PUD was set.
> */
> int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> {
> - u8 mtrr;
> + u8 mtrr, uniform;
>
> - mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> - if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> + mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
> + if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
> return 0;
>
> prot = pgprot_4k_2_large(prot);
> @@ -593,18 +594,22 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> * pmd_set_huge - setup kernel PMD mapping
> *
> * MTRR can override PAT memory types with 4KB granularity. Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR. MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
> *
> * Return 1 on success, and 0 when no PMD was set.
> */
> int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> {
> - u8 mtrr;
> + u8 mtrr, uniform;
>
> - mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
> - if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> + mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> + if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
> + pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
> + addr, addr + PMD_SIZE);
> return 0;
So this returns 0, i.e. failure already. Why do we even have to warn?
Caller already knows it failed.
And this warning would flood dmesg needlessly.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Sat, 2015-05-09 at 11:08 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:41PM -0600, Toshi Kani wrote:
:
> > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > * Return Values:
> > * MTRR_TYPE_(type) - The effective MTRR type for the region
> > * MTRR_TYPE_INVALID - MTRR is disabled
> > + *
> > + * Output Argument:
> > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > + * is fully covered by a single MTRR entry or the default type.
>
> I'd call this "single_mtrr". "uniform" could also mean that the resulting
> type is uniform, i.e. of the same type but spanning multiple MTRRs.
Actually, that is the intend of "uniform" and the same type but spanning
multiple MTRRs should set "uniform" to 1. The patch does not check such
case for simplicity since we do not need to maximize the performance
with MTRRs for every corner case since they are legacy and their use is
expected to be phased out. It makes sure that a type conflict with
MTRRs is detected so that huge page mappings are made safely.
Also, in most of the cases, "uniform" is set to 1 because there is no
MTRR entry that covers the range, i.e. the default type.
> > */
> > -u8 mtrr_type_lookup(u64 start, u64 end)
> > +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
> > {
> > - u8 type, prev_type;
> > + u8 type, prev_type, is_uniform, dummy;
> > int repeat;
> > u64 partial_end;
> >
> > + *uniform = 1;
> > +
>
> You're setting it here...
>
> > if (!mtrr_state_set)
> > return MTRR_TYPE_INVALID;
>
> ... but if you return here, you would've changed the thing uniform
> points to needlessly as you're returning an error.
We need to set "uniform" to 1 when MTRRs are disabled since there is no
type conflict with MTRRs.
> > @@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> > * the variable ranges.
> > */
> > type = mtrr_type_lookup_fixed(start, end);
> > - if (type != MTRR_TYPE_INVALID)
> > + if (type != MTRR_TYPE_INVALID) {
> > + *uniform = 0;
> > return type;
> > + }
> >
> > /*
> > * 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);
> > + type = mtrr_type_lookup_variable(start, end, &partial_end,
> > + &repeat, &is_uniform);
> >
> > /*
> > * Common path is with repeat = 0.
> > @@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> > while (repeat) {
> > prev_type = type;
> > start = partial_end;
> > + is_uniform = 0;
>
> So I think it would be better if you added an out: label where you do
> exit from the function and set return values there.
>
> So something like that, I'm pasting the whole function here so that you
> can follow better:
:
>
> This way you're setting the uniform pointer in a single location and you're
> working with the local variable inside the function.
>
> Much easier to follow.
With the label, the above check will be:
if (!mtrr_state_set) {
is_uniform = 1;
type = MTRR_TYPE_INVALID;
goto out;
}
I can follow your suggestion of using the label if you still prefer
using it.
> > */
> > int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> > {
> > - u8 mtrr;
> > + u8 mtrr, uniform;
> >
> > - mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
> > - if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> > + mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> > + if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
> > + pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
> > + addr, addr + PMD_SIZE);
> > return 0;
>
> So this returns 0, i.e. failure already. Why do we even have to warn?
> Caller already knows it failed.
>
> And this warning would flood dmesg needlessly.
The warning was suggested by reviewers in the previous review so that
driver writers will notice the issue. Returning 0 here will lead
ioremap() to use 4KB mappings, but does not cause ioremap() to fail.
Thanks,
-Toshi
On Mon, May 11, 2015 at 01:25:16PM -0600, Toshi Kani wrote:
> > > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > > * Return Values:
> > > * MTRR_TYPE_(type) - The effective MTRR type for the region
> > > * MTRR_TYPE_INVALID - MTRR is disabled
> > > + *
> > > + * Output Argument:
> > > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > > + * is fully covered by a single MTRR entry or the default type.
> >
> > I'd call this "single_mtrr". "uniform" could also mean that the resulting
> > type is uniform, i.e. of the same type but spanning multiple MTRRs.
>
> Actually, that is the intend of "uniform" and the same type but spanning
> multiple MTRRs should set "uniform" to 1. The patch does not check such
So why does it say "is fully covered by a single MTRR entry or the
default type." - the stress being on *single*
You need to make up your mind.
> We need to set "uniform" to 1 when MTRRs are disabled since there is no
> type conflict with MTRRs.
No, this is wrong.
When we return an *error*, "uniform" should be *undefined* because MTRRs
are disabled and callers should be checking whether it returned an error
first and only *then* look at uniform.
> The warning was suggested by reviewers in the previous review so that
> driver writers will notice the issue.
No, we don't flood dmesg so that driver writers notice stuff. We better
fix the callers.
> Returning 0 here will lead
> ioremap() to use 4KB mappings, but does not cause ioremap() to fail.
I guess a pr_warn_once() should be better then. Flooding dmesg with
error messages for which the user can't really do anything about doesn't
bring us anything.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Mon, 2015-05-11 at 22:18 +0200, Borislav Petkov wrote:
> On Mon, May 11, 2015 at 01:25:16PM -0600, Toshi Kani wrote:
> > > > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > > > * Return Values:
> > > > * MTRR_TYPE_(type) - The effective MTRR type for the region
> > > > * MTRR_TYPE_INVALID - MTRR is disabled
> > > > + *
> > > > + * Output Argument:
> > > > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > > > + * is fully covered by a single MTRR entry or the default type.
> > >
> > > I'd call this "single_mtrr". "uniform" could also mean that the resulting
> > > type is uniform, i.e. of the same type but spanning multiple MTRRs.
> >
> > Actually, that is the intend of "uniform" and the same type but spanning
> > multiple MTRRs should set "uniform" to 1. The patch does not check such
>
> So why does it say "is fully covered by a single MTRR entry or the
> default type." - the stress being on *single*
>
> You need to make up your mind.
I will clarify the comment as follows.
===
uniform - Set to 1 when the region is not covered with multiple memory
types by MTRRs. It is set for any return value.
NOTE: The current code sets 'uniform' to 1 when the region is fully
covered by a single MTRR entry or fully uncovered. However, it does not
detect a uniform case that the region is covered by the same type but
spanning multiple MTRR entries for simplicity.
===
> > We need to set "uniform" to 1 when MTRRs are disabled since there is no
> > type conflict with MTRRs.
>
> No, this is wrong.
>
> When we return an *error*, "uniform" should be *undefined* because MTRRs
> are disabled and callers should be checking whether it returned an error
> first and only *then* look at uniform.
MTRRs disabled is not an error case as it could be a normal
configuration on some platforms / BIOS setups. I clarified it in the
above comment that uniform is set for any return value.
> > The warning was suggested by reviewers in the previous review so that
> > driver writers will notice the issue.
>
> No, we don't flood dmesg so that driver writers notice stuff. We better
> fix the callers.
>
> > Returning 0 here will lead
> > ioremap() to use 4KB mappings, but does not cause ioremap() to fail.
>
> I guess a pr_warn_once() should be better then. Flooding dmesg with
> error messages for which the user can't really do anything about doesn't
> bring us anything.
OK, I will change it to pr_warn_once().
Thanks,
-Toshi
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...
> 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.
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.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
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
On Mon, May 11, 2015 at 04:09:39PM -0600, Toshi Kani wrote:
> There may not be any type conflict with MTRR_TYPE_INVALID.
Because...?
Let me guess: you cannot change this function to return a signed value
which is the type when positive and an error when negative?
> I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
> uniform case.
That would be, of course, also wrong.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2015-05-12 at 09:28 +0200, Borislav Petkov wrote:
> On Mon, May 11, 2015 at 04:09:39PM -0600, Toshi Kani wrote:
> > There may not be any type conflict with MTRR_TYPE_INVALID.
>
> Because...?
Because you cannot have a memory type conflict with MTRRs when MTRRs are
disabled. mtrr_type_lookup() returns MTRR_TYPE_INVALID when MTRRs are
disabled. This is stated in the comments of mtrr_type_lookup() and the
MTRR_TYPE_INVALID definition itself.
BIOS can disable MTRRs, or VM may choose not to implement MTRRs. The OS
needs to handle this case as a valid config, and this is not an error
case.
> Let me guess: you cannot change this function to return a signed value
> which is the type when positive and an error when negative?
No, that is not the reason.
> > I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
> > uniform case.
>
> That would be, of course, also wrong.
I am confused... In your previous comments, you mentioned that:
| 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 type conflict when MTRRs are disabled. There is no
point of defining a new return value.
| Or add an inline function called mtrr_enabled() and call it in the
| mtrr_type_lookup() callers.
MTRR_TYPE_INVALID means MTRRs disabled. So, the caller checking with
this value is the same as checking with mtrr_enabled() you suggested.
Thanks,
-Toshi
On Tue, May 12, 2015 at 08:30:30AM -0600, Toshi Kani wrote:
> MTRR_TYPE_INVALID means MTRRs disabled. So, the caller checking with
> this value is the same as checking with mtrr_enabled() you suggested.
So then you don't have to set *uniform = 1 on entry to
mtrr_type_lookup(). And change the retval test
if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
to
if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) && (mtrr != MTRR_TYPE_WRBACK))
You can put the MTRR_TYPE_INVALID first so that it shortcuts.
You need the distinction between MTRRs *disabled* and an MTRR region
being {non-,}uniform.
If MTRRs are disabled, uniform doesn't *mean* *anything* because it is
undefined. When MTRRs are disabled, the range is *not* covered by MTRRs
because, well, them MTRRs are disabled.
And it might be fine for *your* use case to set *uniform even when MTRRs
are disabled but it might matter in the future. So we better design it
correct from the beginning.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2015-05-12 at 18:31 +0200, Borislav Petkov wrote:
> On Tue, May 12, 2015 at 08:30:30AM -0600, Toshi Kani wrote:
> > MTRR_TYPE_INVALID means MTRRs disabled. So, the caller checking with
> > this value is the same as checking with mtrr_enabled() you suggested.
>
> So then you don't have to set *uniform = 1 on entry to
> mtrr_type_lookup(). And change the retval test
>
> if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
>
> to
> if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) && (mtrr != MTRR_TYPE_WRBACK))
Yes, that's what I was thinking as well. Will do.
> You can put the MTRR_TYPE_INVALID first so that it shortcuts.
>
> You need the distinction between MTRRs *disabled* and an MTRR region
> being {non-,}uniform.
>
> If MTRRs are disabled, uniform doesn't *mean* *anything* because it is
> undefined. When MTRRs are disabled, the range is *not* covered by MTRRs
> because, well, them MTRRs are disabled.
>
> And it might be fine for *your* use case to set *uniform even when MTRRs
> are disabled but it might matter in the future. So we better design it
> correct from the beginning.
I think it is a matter of how "uniform" is defined, but your point is
taken and I will change it accordingly.
Thanks,
-Toshi