Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Apr 02, 2015 at 05:52:16PM -0600, Toshi Kani wrote:
> On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
> > On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> > > On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > > > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > > > :
> > > > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > > > }
> > > > >
> > > > > if (mtrr_if) {
> > > > > + mtrr_enabled = true;
> > > > > set_num_var_ranges();
> > > > > init_table();
> > > > > if (use_intel()) {
> > > > get_mtrr_state();
> > > >
> > > > After setting mtrr_enabled to true, get_mtrr_state() reads
> > > > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > > > MTRRs are enabled or not on the system. So, potentially, we could have
> > > > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > > > to disabled when MTRRs are disabled by BIOS.
> > >
> > > Thanks for the review, in this case then we should update mtrr_enabled to
> > > false.
> > >
> > > > ps.
> > > > I recently cleaned up this part of the MTRR code in the patch below,
> > > > which is currently available in the -mm & -next trees.
> > > > https://lkml.org/lkml/2015/3/24/1063
> > >
> > > Great I will rebase and work with that and try to address this
> > > consideration you have raised.
> >
> > OK I'll mesh in this change as well in my next respin:
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
> > b/arch/x86/kernel/cpu/mtrr/generic.c
> > index a83f27a..ecf7cb9 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
> > }
> >
> > /* Grab all of the MTRR state for this CPU into *state */
> > -void __init get_mtrr_state(void)
> > +bool __init get_mtrr_state(void)
> > {
> > struct mtrr_var_range *vrs;
> > unsigned long flags;
> > @@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
> >
> > post_set();
> > local_irq_restore(flags);
> > +
> > + return !!mtrr_state.enabled;
>
> This should be:
> return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
>
> because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
> MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks, I've used
return !!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED);
Amended.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, 2015-04-02 at 23:49 +0200, Luis R. Rodriguez wrote:
> On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > > :
> > > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > > }
> > > >
> > > > if (mtrr_if) {
> > > > + mtrr_enabled = true;
> > > > set_num_var_ranges();
> > > > init_table();
> > > > if (use_intel()) {
> > > get_mtrr_state();
> > >
> > > After setting mtrr_enabled to true, get_mtrr_state() reads
> > > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > > MTRRs are enabled or not on the system. So, potentially, we could have
> > > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > > to disabled when MTRRs are disabled by BIOS.
> >
> > Thanks for the review, in this case then we should update mtrr_enabled to
> > false.
> >
> > > ps.
> > > I recently cleaned up this part of the MTRR code in the patch below,
> > > which is currently available in the -mm & -next trees.
> > > https://lkml.org/lkml/2015/3/24/1063
> >
> > Great I will rebase and work with that and try to address this
> > consideration you have raised.
>
> OK I'll mesh in this change as well in my next respin:
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
> b/arch/x86/kernel/cpu/mtrr/generic.c
> index a83f27a..ecf7cb9 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
> }
>
> /* Grab all of the MTRR state for this CPU into *state */
> -void __init get_mtrr_state(void)
> +bool __init get_mtrr_state(void)
> {
> struct mtrr_var_range *vrs;
> unsigned long flags;
> @@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
>
> post_set();
> local_irq_restore(flags);
> +
> + return !!mtrr_state.enabled;
This should be:
return mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED;
because the MTRR_STATE_MTRR_FIXED_ENABLED flag is ignored when the
MTRR_STATE_MTRR_ENABLED flag is clear.
Thanks,
-Toshi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Apr 2, 2015 at 4:02 PM, Luis R. Rodriguez wrote: > --- > It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT > and end up with a system with MTRR functionality disabled > PAT functionality enabled. This is missing a conjunction or something in "MTRR functionality disabled PAT functionality." Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Sat, Mar 28, 2015 at 12:56:30AM +0100, Luis R. Rodriguez wrote:
> On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> > On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> > :
> > > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > > }
> > >
> > > if (mtrr_if) {
> > > + mtrr_enabled = true;
> > > set_num_var_ranges();
> > > init_table();
> > > if (use_intel()) {
> > get_mtrr_state();
> >
> > After setting mtrr_enabled to true, get_mtrr_state() reads
> > MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> > MTRRs are enabled or not on the system. So, potentially, we could have
> > a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> > to disabled when MTRRs are disabled by BIOS.
>
> Thanks for the review, in this case then we should update mtrr_enabled to
> false.
>
> > ps.
> > I recently cleaned up this part of the MTRR code in the patch below,
> > which is currently available in the -mm & -next trees.
> > https://lkml.org/lkml/2015/3/24/1063
>
> Great I will rebase and work with that and try to address this
> consideration you have raised.
OK I'll mesh in this change as well in my next respin:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
b/arch/x86/kernel/cpu/mtrr/generic.c
index a83f27a..ecf7cb9 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -438,7 +438,7 @@ static void __init print_mtrr_state(void)
}
/* Grab all of the MTRR state for this CPU into *state */
-void __init get_mtrr_state(void)
+bool __init get_mtrr_state(void)
{
struct mtrr_var_range *vrs;
unsigned long flags;
@@ -482,6 +482,8 @@ void __init get_mtrr_state(void)
post_set();
local_irq_restore(flags);
+
+ return !!mtrr_state.enabled;
}
/* Some BIOS's are messed up and don't set all MTRRs the same! */
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index ea5f363..f96195e 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -734,22 +742,25 @@ void __init mtrr_bp_init(void)
}
if (mtrr_if) {
+ mtrr_enabled = true;
set_num_var_ranges();
init_table();
if (use_intel()) {
- get_mtrr_state();
+ /* BIOS may override */
+ mtrr_enabled = get_mtrr_state();
if (mtrr_cleanup(phys_addr)) {
changed_by_mtrr_cleanup = 1;
@@ -745,11 +755,14 @@ void __init mtrr_bp_init(void)
}
}
}
+
+ if (!mtrr_enabled)
+ pr_info("mtrr: system does not support MTRR\n");
}
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index df5e41f..951884d 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -51,7 +51,7 @@ void set_mtrr_prepare_save(struct set_mtrr_context *ctxt);
void fill_mtrr_var_range(unsigned int index,
u32 base_lo, u32 base_hi, u32 mask_lo, u32 mask_hi);
-void get_mtrr_state(void);
+bool get_mtrr_state(void);
extern void set_mtrr_ops(const struct mtrr_ops *ops);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Apr 02, 2015 at 03:28:51PM -0500, Bjorn Helgaas wrote:
> On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez wrote:
> > On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas wrote:
> >>
> >> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez wrote:
> >>
> >> > I'll rephrase this to:
> >> >
> >> > ---
> >> > It is possible to enable CONFIG_MTRR and up with it
> >> > disabled at run time and yet CONFIG_X86_PAT continues
> >> > to kick through with all functionally enabled. This
> >> > can happen for instance on Xen where MTRR is not
> >> > supported but PAT is, this can happen now on Linux as
> >> > of commit 47591df50 by Juergen introduced as of v3.19.
> >>
> >> I still can't parse this. What does "up with it disabled at run time"
> >> mean?
> >
> > It means that technically even if your CPU/BIOS/system did support
> > MTRR if you use a kernel with MTRR support enabled you might end up
> > with a situation where under one situation MTRR might be enabled and
> > at another run time scenario with the same exact kernel and system you
> > will end up with MTRR disabled. Such is the case for example when
> > booting with Xen, which disables the CPU bits on the hypervisor code.
> > If you boot the same system without Xen you'll get MTRR.
>
> Your text is missing some words. You seem to be using "up" as a verb,
> but it's not a verb. Maybe you meant "end up"?
Indeed.
> Even then, it
> wouldn't make sense for CONFIG_MTRR to be "disabled at run time"
> because CONFIG_MTRR is a compile-time switch. The MTRR
> *functionality* could certainly be disabled at run-time, but not
> CONFIG_MTRR itself.
I'll clarify.
> >> And "... continues to kick through"? Probably some idiomatic
> >> usage I'm just too old to understand :)
> >
> > That means for example that in both the above circumstances even if
> > MTRR went disabled at run time with Xen, the kernel went through with
> > getting PAT enabled.
>
> "CONFIG_X86_PAT continues to kick through" doesn't seem a very precise
> way of describing this. But maybe it's enough for experts in this
> area (which I'm not).
I've rephrased this to:
---
It is possible to enable CONFIG_MTRR and CONFIG_X86_PAT
and end up with a system with MTRR functionality disabled
PAT functionality enabled. This can happen for instance
on Xen where MTRR is not supported but PAT is. This can
happen on Linux as of commit 47591df50 ("xen: Support Xen
pv-domains using PAT") by Juergen, introduced as of v3.19.
---
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Apr 2, 2015 at 3:20 PM, Luis R. Rodriguez wrote: > On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas wrote: >> >> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez wrote: >> >> > I'll rephrase this to: >> > >> > --- >> > It is possible to enable CONFIG_MTRR and up with it >> > disabled at run time and yet CONFIG_X86_PAT continues >> > to kick through with all functionally enabled. This >> > can happen for instance on Xen where MTRR is not >> > supported but PAT is, this can happen now on Linux as >> > of commit 47591df50 by Juergen introduced as of v3.19. >> >> I still can't parse this. What does "up with it disabled at run time" >> mean? > > It means that technically even if your CPU/BIOS/system did support > MTRR if you use a kernel with MTRR support enabled you might end up > with a situation where under one situation MTRR might be enabled and > at another run time scenario with the same exact kernel and system you > will end up with MTRR disabled. Such is the case for example when > booting with Xen, which disables the CPU bits on the hypervisor code. > If you boot the same system without Xen you'll get MTRR. Your text is missing some words. You seem to be using "up" as a verb, but it's not a verb. Maybe you meant "end up"? Even then, it wouldn't make sense for CONFIG_MTRR to be "disabled at run time" because CONFIG_MTRR is a compile-time switch. The MTRR *functionality* could certainly be disabled at run-time, but not CONFIG_MTRR itself. >> And "... continues to kick through"? Probably some idiomatic >> usage I'm just too old to understand :) > > That means for example that in both the above circumstances even if > MTRR went disabled at run time with Xen, the kernel went through with > getting PAT enabled. "CONFIG_X86_PAT continues to kick through" doesn't seem a very precise way of describing this. But maybe it's enough for experts in this area (which I'm not). Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Apr 2, 2015 at 1:13 PM, Bjorn Helgaas wrote:
>
> On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez wrote:
>
> > I'll rephrase this to:
> >
> > ---
> > It is possible to enable CONFIG_MTRR and up with it
> > disabled at run time and yet CONFIG_X86_PAT continues
> > to kick through with all functionally enabled. This
> > can happen for instance on Xen where MTRR is not
> > supported but PAT is, this can happen now on Linux as
> > of commit 47591df50 by Juergen introduced as of v3.19.
>
> I still can't parse this. What does "up with it disabled at run time"
> mean?
It means that technically even if your CPU/BIOS/system did support
MTRR if you use a kernel with MTRR support enabled you might end up
with a situation where under one situation MTRR might be enabled and
at another run time scenario with the same exact kernel and system you
will end up with MTRR disabled. Such is the case for example when
booting with Xen, which disables the CPU bits on the hypervisor code.
If you boot the same system without Xen you'll get MTRR.
> And "... continues to kick through"? Probably some idiomatic
> usage I'm just too old to understand :)
That means for example that in both the above circumstances even if
MTRR went disabled at run time with Xen, the kernel went through with
getting PAT enabled.
> Please use the conventional citation format:
>
> 47591df50512 ("xen: Support Xen pv-domains using PAT")
>
> A one-character typo in a SHA1 makes it completely useless, so it's
> nice to have the summary line both for readability and a bit of
> redundancy.
Sure, fixed.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Thu, Mar 26, 2015 at 6:35 PM, Luis R. Rodriguez wrote:
> I'll rephrase this to:
>
> ---
> It is possible to enable CONFIG_MTRR and up with it
> disabled at run time and yet CONFIG_X86_PAT continues
> to kick through with all functionally enabled. This
> can happen for instance on Xen where MTRR is not
> supported but PAT is, this can happen now on Linux as
> of commit 47591df50 by Juergen introduced as of v3.19.
I still can't parse this. What does "up with it disabled at run time"
mean? And "... continues to kick through"? Probably some idiomatic
usage I'm just too old to understand :)
Please use the conventional citation format:
47591df50512 ("xen: Support Xen pv-domains using PAT")
A one-character typo in a SHA1 makes it completely useless, so it's
nice to have the summary line both for readability and a bit of
redundancy.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Fri, Mar 27, 2015 at 02:40:17PM -0600, Toshi Kani wrote:
> On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
> :
> > @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> > }
> >
> > if (mtrr_if) {
> > + mtrr_enabled = true;
> > set_num_var_ranges();
> > init_table();
> > if (use_intel()) {
> get_mtrr_state();
>
> After setting mtrr_enabled to true, get_mtrr_state() reads
> MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
> MTRRs are enabled or not on the system. So, potentially, we could have
> a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
> to disabled when MTRRs are disabled by BIOS.
Thanks for the review, in this case then we should update mtrr_enabled to false.
> ps.
> I recently cleaned up this part of the MTRR code in the patch below,
> which is currently available in the -mm & -next trees.
> https://lkml.org/lkml/2015/3/24/1063
Great I will rebase and work with that and try to address this
consideration you have raised.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Fri, 2015-03-20 at 16:17 -0700, Luis R. Rodriguez wrote:
:
> @@ -734,6 +742,7 @@ void __init mtrr_bp_init(void)
> }
>
> if (mtrr_if) {
> + mtrr_enabled = true;
> set_num_var_ranges();
> init_table();
> if (use_intel()) {
get_mtrr_state();
After setting mtrr_enabled to true, get_mtrr_state() reads
MSR_MTRRdefType and sets 'mtrr_state.enabled', which also indicates if
MTRRs are enabled or not on the system. So, potentially, we could have
a case that mtrr_enabled is set to true, but mtrr_state.enabled is set
to disabled when MTRRs are disabled by BIOS.
Thanks,
-Toshi
ps.
I recently cleaned up this part of the MTRR code in the patch below,
which is currently available in the -mm & -next trees.
https://lkml.org/lkml/2015/3/24/1063
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Wed, Mar 25, 2015 at 03:59:41PM -0400, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote: > > From: "Luis R. Rodriguez" > > > > It is possible to enable CONFIG_MTRR and up with it > > disabled at run time and yet CONFIG_X86_PAT continues > > to kick through fully functionally. This can happen > > s/fully/full/ ? I'll rephrase this to: --- It is possible to enable CONFIG_MTRR and up with it disabled at run time and yet CONFIG_X86_PAT continues to kick through with all functionally enabled. This can happen for instance on Xen where MTRR is not supported but PAT is, this can happen now on Linux as of commit 47591df50 by Juergen introduced as of v3.19. --- Which BTW I had also mentioned on the cover letter that this is a good time to address if we want to make PAT then a first class citizen, to detangle it from depending on MTRR. If so I can do that later. > > Technically we should assume the proper CPU > > bits would be set to disable MTRR but we can't > > always rely on this. At least on the Xen Hypervisor > > for instance only X86_FEATURE_MTRR was disabled > > as of Xen 4.4 through Xen commit 586ab6a [0], > > but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR, > > or X86_FEATURE_CYRIX_ARR for instance. > > Oh, could you send an patch for that to Xen please? Done. > > x86 mtrr code relies on quite a bit of checks for > > mtrr_if being set to check to see if MTRR did get > > set up, instead of using that lets provide a generic > > setter which when set we know MTRR is enabled. This > > s/we know MTRR is enabled/will let us know that MTRR is enabled/ Amended. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On 03/25/2015 08:59 PM, Konrad Rzeszutek Wilk wrote:
On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote:
From: "Luis R. Rodriguez"
It is possible to enable CONFIG_MTRR and up with it
disabled at run time and yet CONFIG_X86_PAT continues
to kick through fully functionally. This can happen
s/fully/full/ ?
for instance on Xen where MTRR is not supported but
PAT is, this can happen now on Linux as of commit
47591df50 by Juergen introduced as of v3.19.
s/3.19/4.0/
No, 3.19 is correct.
Juergen
Technically we should assume the proper CPU
bits would be set to disable MTRR but we can't
always rely on this. At least on the Xen Hypervisor
for instance only X86_FEATURE_MTRR was disabled
as of Xen 4.4 through Xen commit 586ab6a [0],
but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR,
or X86_FEATURE_CYRIX_ARR for instance.
Oh, could you send an patch for that to Xen please?
x86 mtrr code relies on quite a bit of checks for
mtrr_if being set to check to see if MTRR did get
set up, instead of using that lets provide a generic
setter which when set we know MTRR is enabled. This
s/we know MTRR is enabled/will let us know that MTRR is enabled/
also adds a few checks where they were not before
which could potentially safeguard ourselves against
incorrect usage of MTRR where this was not desirable.
Where possible match error codes as if MTRR was
disabled on arch/x86/include/asm/mtrr.h.
Lastly, since disabling MTRR can happen at run time
and we could end up with PAT enabled best record now
on our logs when MTRR is disabled.
[0] ~/devel/xen (git::stable-4.5)$ git describe --contains 586ab6a
4.4.0-rc1~18
Cc: Andy Lutomirski
Cc: Suresh Siddha
Cc: Venkatesh Pallipadi
Cc: Ingo Molnar
Cc: Thomas Gleixner
Cc: Juergen Gross
Cc: Daniel Vetter
Cc: Dave Airlie
Cc: Antonino Daplas
Cc: Jean-Christophe Plagniol-Villard
Cc: Tomi Valkeinen
Cc: Dave Hansen
Cc: venkatesh.pallip...@intel.com
Cc: Stefan Bader
Cc: konrad.w...@oracle.com
Cc: ville.syrj...@linux.intel.com
Cc: david.vra...@citrix.com
Cc: jbeul...@suse.com
Cc: toshi.k...@hp.com
Cc: bhelg...@google.com
Cc: Roger Pau Monné
Cc: linux-fb...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-de...@lists.xensource.com
Signed-off-by: Luis R. Rodriguez
---
arch/x86/include/asm/mtrr.h| 2 ++
arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
arch/x86/kernel/cpu/mtrr/generic.c | 5 +++--
arch/x86/kernel/cpu/mtrr/if.c | 3 +++
arch/x86/kernel/cpu/mtrr/main.c| 31 ++-
5 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f768f62..cade917 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -31,6 +31,7 @@
* arch_phys_wc_add and arch_phys_wc_del.
*/
# ifdef CONFIG_MTRR
+extern int mtrr_enabled;
extern u8 mtrr_type_lookup(u64 addr, u64 end);
extern void mtrr_save_fixed_ranges(void *);
extern void mtrr_save_state(void);
@@ -50,6 +51,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
extern int amd_special_default_mtrr(void);
extern int phys_wc_to_mtrr_index(int handle);
# else
+static const int mtrr_enabled;
static inline u8 mtrr_type_lookup(u64 addr, u64 end)
{
/*
diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c
b/arch/x86/kernel/cpu/mtrr/cleanup.c
index 5f90b85..784dc55 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -880,7 +880,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)
* Make sure we only trim uncachable memory on machines that
* support the Intel MTRR architecture:
*/
- if (!is_cpu(INTEL) || disable_mtrr_trim)
+ if (!is_cpu(INTEL) || disable_mtrr_trim || !mtrr_enabled)
return 0;
rdmsr(MSR_MTRRdefType, def, dummy);
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
b/arch/x86/kernel/cpu/mtrr/generic.c
index 09c82de..df321b2 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -116,7 +116,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64
*partial_end, int *repeat)
u8 prev_match, curr_match;
*repeat = 0;
- if (!mtrr_state_set)
+ /* generic_mtrr_ops is only set for generic_mtrr_ops */
+ if (!mtrr_state_set || !mtrr_enabled)
return 0xFF;
if (!mtrr_state.enabled)
@@ -290,7 +291,7 @@ static void get_fixed_ranges(mtrr_type *frs)
void mtrr_save_fixed_ranges(void *info)
{
- if (cpu_has_mtrr)
+ if (mtrr_enabled && cpu_has_mtrr)
get_fixed_ranges(mtrr_state.fixed_ranges);
}
diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index d76f13d..e9e001a 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -436,6 +436,9 @@ static int __init mtrr_if_init(void)
{
struct cpuinfo_x86 *c = &boot_cpu_data;
+ if (!mtrr_enabled)
+
Re: [PATCH v1 02/47] x86: mtrr: generalize run time disabling of MTRR
On Fri, Mar 20, 2015 at 04:17:52PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez"
>
> It is possible to enable CONFIG_MTRR and up with it
> disabled at run time and yet CONFIG_X86_PAT continues
> to kick through fully functionally. This can happen
s/fully/full/ ?
> for instance on Xen where MTRR is not supported but
> PAT is, this can happen now on Linux as of commit
> 47591df50 by Juergen introduced as of v3.19.
s/3.19/4.0/
>
> Technically we should assume the proper CPU
> bits would be set to disable MTRR but we can't
> always rely on this. At least on the Xen Hypervisor
> for instance only X86_FEATURE_MTRR was disabled
> as of Xen 4.4 through Xen commit 586ab6a [0],
> but not X86_FEATURE_K6_MTRR, X86_FEATURE_CENTAUR_MCR,
> or X86_FEATURE_CYRIX_ARR for instance.
Oh, could you send an patch for that to Xen please?
>
> x86 mtrr code relies on quite a bit of checks for
> mtrr_if being set to check to see if MTRR did get
> set up, instead of using that lets provide a generic
> setter which when set we know MTRR is enabled. This
s/we know MTRR is enabled/will let us know that MTRR is enabled/
> also adds a few checks where they were not before
> which could potentially safeguard ourselves against
> incorrect usage of MTRR where this was not desirable.
>
> Where possible match error codes as if MTRR was
> disabled on arch/x86/include/asm/mtrr.h.
>
> Lastly, since disabling MTRR can happen at run time
> and we could end up with PAT enabled best record now
> on our logs when MTRR is disabled.
>
> [0] ~/devel/xen (git::stable-4.5)$ git describe --contains 586ab6a
> 4.4.0-rc1~18
>
> Cc: Andy Lutomirski
> Cc: Suresh Siddha
> Cc: Venkatesh Pallipadi
> Cc: Ingo Molnar
> Cc: Thomas Gleixner
> Cc: Juergen Gross
> Cc: Daniel Vetter
> Cc: Dave Airlie
> Cc: Antonino Daplas
> Cc: Jean-Christophe Plagniol-Villard
> Cc: Tomi Valkeinen
> Cc: Dave Hansen
> Cc: venkatesh.pallip...@intel.com
> Cc: Stefan Bader
> Cc: konrad.w...@oracle.com
> Cc: ville.syrj...@linux.intel.com
> Cc: david.vra...@citrix.com
> Cc: jbeul...@suse.com
> Cc: toshi.k...@hp.com
> Cc: bhelg...@google.com
> Cc: Roger Pau Monné
> Cc: linux-fb...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: xen-de...@lists.xensource.com
> Signed-off-by: Luis R. Rodriguez
> ---
> arch/x86/include/asm/mtrr.h| 2 ++
> arch/x86/kernel/cpu/mtrr/cleanup.c | 2 +-
> arch/x86/kernel/cpu/mtrr/generic.c | 5 +++--
> arch/x86/kernel/cpu/mtrr/if.c | 3 +++
> arch/x86/kernel/cpu/mtrr/main.c| 31 ++-
> 5 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
> index f768f62..cade917 100644
> --- a/arch/x86/include/asm/mtrr.h
> +++ b/arch/x86/include/asm/mtrr.h
> @@ -31,6 +31,7 @@
> * arch_phys_wc_add and arch_phys_wc_del.
> */
> # ifdef CONFIG_MTRR
> +extern int mtrr_enabled;
> extern u8 mtrr_type_lookup(u64 addr, u64 end);
> extern void mtrr_save_fixed_ranges(void *);
> extern void mtrr_save_state(void);
> @@ -50,6 +51,7 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
> extern int amd_special_default_mtrr(void);
> extern int phys_wc_to_mtrr_index(int handle);
> # else
> +static const int mtrr_enabled;
> static inline u8 mtrr_type_lookup(u64 addr, u64 end)
> {
> /*
> diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c
> b/arch/x86/kernel/cpu/mtrr/cleanup.c
> index 5f90b85..784dc55 100644
> --- a/arch/x86/kernel/cpu/mtrr/cleanup.c
> +++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
> @@ -880,7 +880,7 @@ int __init mtrr_trim_uncached_memory(unsigned long
> end_pfn)
>* Make sure we only trim uncachable memory on machines that
>* support the Intel MTRR architecture:
>*/
> - if (!is_cpu(INTEL) || disable_mtrr_trim)
> + if (!is_cpu(INTEL) || disable_mtrr_trim || !mtrr_enabled)
> return 0;
>
> rdmsr(MSR_MTRRdefType, def, dummy);
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
> b/arch/x86/kernel/cpu/mtrr/generic.c
> index 09c82de..df321b2 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -116,7 +116,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64
> *partial_end, int *repeat)
> u8 prev_match, curr_match;
>
> *repeat = 0;
> - if (!mtrr_state_set)
> + /* generic_mtrr_ops is only set for generic_mtrr_ops */
> + if (!mtrr_state_set || !mtrr_enabled)
> return 0xFF;
>
> if (!mtrr_state.enabled)
> @@ -290,7 +291,7 @@ static void get_fixed_ranges(mtrr_type *frs)
>
> void mtrr_save_fixed_ranges(void *info)
> {
> - if (cpu_has_mtrr)
> + if (mtrr_enabled && cpu_has_mtrr)
> get_fixed_ranges(mtrr_state.fixed_ranges);
> }
>
> diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
> index d76f13d..e9e001a 100644
> --- a/arch/x86/kernel/cpu/mtrr/if.c
> +++ b/arch/x86/kernel/cpu/mtrr/if.c
> @@ -436,6 +436,9 @@
