Hi Ray, Sorry for later response.
Reviewed-by: Eric Dong <eric.d...@intel.com> Thanks, Eric > -----Original Message----- > From: Ni, Ray > Sent: Monday, January 21, 2019 11:17 PM > To: edk2-devel@lists.01.org > Cc: Ni, Ray <ray...@intel.com>; Dong, Eric <eric.d...@intel.com>; Chiu, > Chasel <chasel.c...@intel.com> > Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set > memory <1MB to UC > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1481 > > Today's MtrrLib contains a bug, for example: > when the original cache setting is WB for [0xF_0000, 0xF_8000) and, a new > request to set [0xF_0000, 0xF_4000) to WP, the cache setting for [0xF_4000, > 0xF_8000) is reset to UC. > > The reason is when MtrrLibSetBelow1MBMemoryAttribute() is called the > WorkingFixedSettings doesn't contain the actual MSR value stored in > hardware, but when writing the fixed MTRRs, the code logic assumes > WorkingFixedSettings contains the actual MSR value. > > The new fix is to change MtrrLibSetBelow1MBMemoryAttribute() to > calculate the correct ClearMasks[] and OrMasks[], and use them directly > when writing the fixed MTRRs. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ray Ni <ray...@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Chasel Chiu <chasel.c...@intel.com> > --- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 59 +++++++++------------------- > 1 file changed, 18 insertions(+), 41 deletions(-) > > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > index 086f7ad8f0..2cf7d092e8 100644 > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > @@ -5,7 +5,7 @@ > Most of services in this library instance are suggested to be invoked by > BSP > only, > except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting to > APs. > > - Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2008 - 2019, Intel Corporation. All rights > + reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at @@ -2099,8 +2099,8 @@ MtrrLibSetMemoryRanges ( > Set the below-1MB memory attribute to fixed MTRR buffer. > Modified flag array indicates which fixed MTRR is modified. > > - @param [in, out] FixedSettings Fixed MTRR buffer. > - @param [out] Modified Flag array indicating which MTRR is > modified. > + @param [in, out] ClearMasks The bits to clear in the fixed MTRR MSR. > + @param [in, out] OrMasks The bits to set in the fixed MTRR MSR. > @param [in] BaseAddress Base address. > @param [in] Length Length. > @param [in] Type Memory type. > @@ -2111,8 +2111,8 @@ MtrrLibSetMemoryRanges ( **/ RETURN_STATUS > MtrrLibSetBelow1MBMemoryAttribute ( > - IN OUT MTRR_FIXED_SETTINGS *FixedSettings, > - OUT BOOLEAN *Modified, > + IN OUT UINT64 *ClearMasks, > + IN OUT UINT64 *OrMasks, > IN PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN MTRR_MEMORY_CACHE_TYPE Type > @@ -2122,36 +2122,17 @@ MtrrLibSetBelow1MBMemoryAttribute ( > UINT32 MsrIndex; > UINT64 ClearMask; > UINT64 OrMask; > - UINT64 ClearMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]; > - UINT64 OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]; > - BOOLEAN LocalModified[ARRAY_SIZE > (mMtrrLibFixedMtrrTable)]; > > ASSERT (BaseAddress < BASE_1MB); > > - SetMem (LocalModified, sizeof (LocalModified), FALSE); > - > - // > - // (Value & ~0 | 0) still equals to (Value) > - // > - SetMem (ClearMasks, sizeof (ClearMasks), 0); > - SetMem (OrMasks, sizeof (OrMasks), 0); > - > MsrIndex = (UINT32)-1; > while ((BaseAddress < BASE_1MB) && (Length != 0)) { > Status = MtrrLibProgramFixedMtrr (Type, &BaseAddress, &Length, > &MsrIndex, &ClearMask, &OrMask); > if (RETURN_ERROR (Status)) { > return Status; > } > - ClearMasks[MsrIndex] = ClearMask; > - OrMasks[MsrIndex] = OrMask; > - Modified[MsrIndex] = TRUE; > - LocalModified[MsrIndex] = TRUE; > - } > - > - for (MsrIndex = 0; MsrIndex < ARRAY_SIZE (mMtrrLibFixedMtrrTable); > MsrIndex++) { > - if (LocalModified[MsrIndex]) { > - FixedSettings->Mtrr[MsrIndex] = (FixedSettings->Mtrr[MsrIndex] & > ~ClearMasks[MsrIndex]) | OrMasks[MsrIndex]; > - } > + ClearMasks[MsrIndex] = ClearMasks[MsrIndex] | ClearMask; > + OrMasks[MsrIndex] = (OrMasks[MsrIndex] & ~ClearMask) | OrMask; > } > return RETURN_SUCCESS; > } > @@ -2213,8 +2194,8 @@ MtrrSetMemoryAttributesInMtrrSettings ( > MTRR_MEMORY_RANGE WorkingVariableMtrr[ARRAY_SIZE > (MtrrSetting->Variables.Mtrr)]; > BOOLEAN VariableSettingModified[ARRAY_SIZE (MtrrSetting- > >Variables.Mtrr)]; > > - BOOLEAN FixedSettingsModified[ARRAY_SIZE > (mMtrrLibFixedMtrrTable)]; > - MTRR_FIXED_SETTINGS WorkingFixedSettings; > + UINT64 ClearMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]; > + UINT64 OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]; > > MTRR_CONTEXT MtrrContext; > BOOLEAN MtrrContextValid; > @@ -2226,10 +2207,6 @@ MtrrSetMemoryAttributesInMtrrSettings ( > // TRUE indicating the accordingly Variable setting needs modificaiton in > OriginalVariableMtrr. > // > SetMem (VariableSettingModified, ARRAY_SIZE (VariableSettingModified), > FALSE); > - // > - // TRUE indicating the accordingly Fixed setting needs modification in > WorkingFixedSettings. > - // > - SetMem (FixedSettingsModified, ARRAY_SIZE (FixedSettingsModified), > FALSE); > > // > // TRUE indicating the caller requests to set variable MTRRs. > @@ -2405,14 +2382,17 @@ MtrrSetMemoryAttributesInMtrrSettings ( > // > // 3. Apply the below-1MB memory attribute settings. > // > - ZeroMem (WorkingFixedSettings.Mtrr, sizeof > (WorkingFixedSettings.Mtrr)); > + // (Value & ~0 | 0) still equals to (Value) // ZeroMem (ClearMasks, > + sizeof (ClearMasks)); ZeroMem (OrMasks, sizeof (OrMasks)); > for (Index = 0; Index < RangeCount; Index++) { > if (Ranges[Index].BaseAddress >= BASE_1MB) { > continue; > } > > Status = MtrrLibSetBelow1MBMemoryAttribute ( > - &WorkingFixedSettings, FixedSettingsModified, > + ClearMasks, OrMasks, > Ranges[Index].BaseAddress, Ranges[Index].Length, > Ranges[Index].Type > ); > if (RETURN_ERROR (Status)) { > @@ -2424,19 +2404,16 @@ MtrrSetMemoryAttributesInMtrrSettings ( > // > // 4. Write fixed MTRRs that have been modified > // > - for (Index = 0; Index < ARRAY_SIZE (FixedSettingsModified); Index++) { > - if (FixedSettingsModified[Index]) { > + for (Index = 0; Index < ARRAY_SIZE (ClearMasks); Index++) { > + if (ClearMasks[Index] != 0) { > if (MtrrSetting != NULL) { > - MtrrSetting->Fixed.Mtrr[Index] = WorkingFixedSettings.Mtrr[Index]; > + MtrrSetting->Fixed.Mtrr[Index] = > + (MtrrSetting->Fixed.Mtrr[Index] & ~ClearMasks[Index]) | > + OrMasks[Index]; > } else { > if (!MtrrContextValid) { > MtrrLibPreMtrrChange (&MtrrContext); > MtrrContextValid = TRUE; > } > - AsmWriteMsr64 ( > - mMtrrLibFixedMtrrTable[Index].Msr, > - WorkingFixedSettings.Mtrr[Index] > - ); > + AsmMsrAndThenOr64 (mMtrrLibFixedMtrrTable[Index].Msr, > + ~ClearMasks[Index], OrMasks[Index]); > } > } > } > -- > 2.20.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
