On Tue, Mar 24, 2015 at 04:08:38PM -0600, Toshi Kani wrote:
> 'mtrr_state.enabled' contains the FE (fixed MTRRs enabled)
> and E (MTRRs enabled) flags in MSR_MTRRdefType. Intel SDM,
> section 11.11.2.1, defines these flags as follows:
> - All MTRRs are disabled when the E flag is clear.
> The FE flag has no affect when the E flag is clear.
> - The default type is enabled when the E flag is set.
> - MTRR variable ranges are enabled when the E flag is set.
> - MTRR fixed ranges are enabled when both E and FE flags
> are set.
>
> MTRR state checks in __mtrr_type_lookup() do not match with
> SDM. Hence, this patch makes the following changes:
> - The current code detects MTRRs disabled when both E and
> FE flags are clear in mtrr_state.enabled. Fix to detect
> MTRRs disabled when the E flag is clear.
> - The current code does not check if the FE bit is set in
> mtrr_state.enabled when looking into the fixed entries.
> Fix to check the FE flag.
> - The current code returns the default type when the E flag
> is clear in mtrr_state.enabled. However, the default type
> is also disabled when the E flag is clear. Fix to remove
> the code as this case is handled as MTRR disabled with
> the 1st change.
>
> In addition, this patch defines the E and FE flags in
> mtrr_state.enabled as follows.
> - FE flag: MTRR_STATE_MTRR_FIXED_ENABLED
> - E flag: MTRR_STATE_MTRR_ENABLED
>
> print_mtrr_state() is also updated accordingly.
>
> Signed-off-by: Toshi Kani <[email protected]>
> ---
> arch/x86/include/uapi/asm/mtrr.h | 4 ++++
> arch/x86/kernel/cpu/mtrr/generic.c | 15 ++++++++-------
> 2 files changed, 12 insertions(+), 7 deletions(-)
You missed a spot in the conversion in
arch/x86/kernel/cpu/mtrr/cleanup.c::x86_get_mtrr_mem_range():
There we have
if (base < (1<<(20-PAGE_SHIFT)) && mtrr_state.have_fixed &&
(mtrr_state.enabled & 1)) {
which should be mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED.
> diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
> index d0acb65..66ba88d 100644
> --- a/arch/x86/include/uapi/asm/mtrr.h
> +++ b/arch/x86/include/uapi/asm/mtrr.h
> @@ -88,6 +88,10 @@ struct mtrr_state_type {
> mtrr_type def_type;
> };
>
> +/* Bit fields for enabled in struct mtrr_state_type */
> +#define MTRR_STATE_MTRR_FIXED_ENABLED 0x01
> +#define MTRR_STATE_MTRR_ENABLED 0x02
> +
> #define MTRRphysBase_MSR(reg) (0x200 + 2 * (reg))
> #define MTRRphysMask_MSR(reg) (0x200 + 2 * (reg) + 1)
Please add those to arch/x86/include/asm/mtrr.h instead. They have no
place in the uapi header.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Wed, 2015-05-06 at 13:47 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:38PM -0600, Toshi Kani wrote:
> > 'mtrr_state.enabled' contains the FE (fixed MTRRs enabled)
> > and E (MTRRs enabled) flags in MSR_MTRRdefType. Intel SDM,
> > section 11.11.2.1, defines these flags as follows:
> > - All MTRRs are disabled when the E flag is clear.
> > The FE flag has no affect when the E flag is clear.
> > - The default type is enabled when the E flag is set.
> > - MTRR variable ranges are enabled when the E flag is set.
> > - MTRR fixed ranges are enabled when both E and FE flags
> > are set.
> >
> > MTRR state checks in __mtrr_type_lookup() do not match with
> > SDM. Hence, this patch makes the following changes:
> > - The current code detects MTRRs disabled when both E and
> > FE flags are clear in mtrr_state.enabled. Fix to detect
> > MTRRs disabled when the E flag is clear.
> > - The current code does not check if the FE bit is set in
> > mtrr_state.enabled when looking into the fixed entries.
> > Fix to check the FE flag.
> > - The current code returns the default type when the E flag
> > is clear in mtrr_state.enabled. However, the default type
> > is also disabled when the E flag is clear. Fix to remove
> > the code as this case is handled as MTRR disabled with
> > the 1st change.
> >
> > In addition, this patch defines the E and FE flags in
> > mtrr_state.enabled as follows.
> > - FE flag: MTRR_STATE_MTRR_FIXED_ENABLED
> > - E flag: MTRR_STATE_MTRR_ENABLED
> >
> > print_mtrr_state() is also updated accordingly.
> >
> > Signed-off-by: Toshi Kani <[email protected]>
> > ---
> > arch/x86/include/uapi/asm/mtrr.h | 4 ++++
> > arch/x86/kernel/cpu/mtrr/generic.c | 15 ++++++++-------
> > 2 files changed, 12 insertions(+), 7 deletions(-)
>
> You missed a spot in the conversion in
> arch/x86/kernel/cpu/mtrr/cleanup.c::x86_get_mtrr_mem_range():
>
> There we have
>
> if (base < (1<<(20-PAGE_SHIFT)) && mtrr_state.have_fixed &&
> (mtrr_state.enabled & 1)) {
>
> which should be mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED.
Right. I will also check both MTRR_STATE_MTRR_FIXED_ENABLED &
MTRR_STATE_MTRR_FIXED_ENABLED bits here.
> > diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
> > index d0acb65..66ba88d 100644
> > --- a/arch/x86/include/uapi/asm/mtrr.h
> > +++ b/arch/x86/include/uapi/asm/mtrr.h
> > @@ -88,6 +88,10 @@ struct mtrr_state_type {
> > mtrr_type def_type;
> > };
> >
> > +/* Bit fields for enabled in struct mtrr_state_type */
> > +#define MTRR_STATE_MTRR_FIXED_ENABLED 0x01
> > +#define MTRR_STATE_MTRR_ENABLED 0x02
> > +
> > #define MTRRphysBase_MSR(reg) (0x200 + 2 * (reg))
> > #define MTRRphysMask_MSR(reg) (0x200 + 2 * (reg) + 1)
>
> Please add those to arch/x86/include/asm/mtrr.h instead. They have no
> place in the uapi header.
I have a question. Those bits define the bit field of enabled in struct
mtrr_state_type, which is defined in this header. Is it OK to only move
those definitions to other header?
Thanks,
-Toshi
On Wed, May 06, 2015 at 09:23:31AM -0600, Toshi Kani wrote:
> I have a question. Those bits define the bit field of enabled in struct
> mtrr_state_type, which is defined in this header. Is it OK to only move
> those definitions to other header?
I think we shouldn't expose stuff to userspace if we don't have to
because then we're stuck with it. Userspace has managed so far without
those defines so I don't see why we should export them now.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Thu, 2015-05-07 at 00:39 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 09:23:31AM -0600, Toshi Kani wrote:
> > I have a question. Those bits define the bit field of enabled in struct
> > mtrr_state_type, which is defined in this header. Is it OK to only move
> > those definitions to other header?
>
> I think we shouldn't expose stuff to userspace if we don't have to
> because then we're stuck with it. Userspace has managed so far without
> those defines so I don't see why we should export them now.
OK, I will move those bits definition to arch/x86/include/asm/mtrr.h.
Thanks for the clarification,
-Toshi