From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EDDE6208EB3E1 for ; Mon, 18 Feb 2019 21:33:38 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Feb 2019 21:33:38 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,387,1544515200"; d="scan'208";a="145372149" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 18 Feb 2019 21:33:37 -0800 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 18 Feb 2019 21:33:36 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 18 Feb 2019 21:33:36 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.207]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.172]) with mapi id 14.03.0415.000; Tue, 19 Feb 2019 13:33:34 +0800 From: "Dong, Eric" To: "Ni, Ray" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set memory <1MB to UC Thread-Index: AQHUsZwCnG/f0BmR00S3PA3bA2NDp6Xmxewg Date: Tue, 19 Feb 2019 05:33:34 +0000 Message-ID: References: <20190121151643.47824-1-ruiyu.ni@intel.com> In-Reply-To: <20190121151643.47824-1-ruiyu.ni@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set memory <1MB to UC X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Feb 2019 05:33:39 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray, Sorry for later response. =20 Reviewed-by: Eric Dong Thanks, Eric > -----Original Message----- > From: Ni, Ray > Sent: Monday, January 21, 2019 11:17 PM > To: edk2-devel@lists.01.org > Cc: Ni, Ray ; Dong, Eric ; Chiu, > Chasel > Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set > memory <1MB to UC >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1481 >=20 > Today's MtrrLib contains a bug, for example: > when the original cache setting is WB for [0xF_0000, 0xF_8000) and, a n= ew > request to set [0xF_0000, 0xF_4000) to WP, the cache setting for [0xF_40= 00, > 0xF_8000) is reset to UC. >=20 > 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. >=20 > The new fix is to change MtrrLibSetBelow1MBMemoryAttribute() to > calculate the correct ClearMasks[] and OrMasks[], and use them directly > when writing the fixed MTRRs. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Chasel Chiu > --- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 59 +++++++++------------------- > 1 file changed, 18 insertions(+), 41 deletions(-) >=20 > 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 invoke= d by BSP > only, > except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR settin= g to > APs. >=20 > - Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> + Copyright (c) 2008 - 2019, Intel Corporation. All rights > + reserved.
> 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. >=20 > - @param [in, out] FixedSettings Fixed MTRR buffer. > - @param [out] Modified Flag array indicating which MTRR is mod= ified. > + @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 (mMtrrLibFixedMtrrTabl= e)]; > - UINT64 OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]= ; > - BOOLEAN LocalModified[ARRAY_SIZE (mMtrrLibFixedMtrrT= able)]; >=20 > ASSERT (BaseAddress < BASE_1MB); >=20 > - SetMem (LocalModified, sizeof (LocalModified), FALSE); > - > - // > - // (Value & ~0 | 0) still equals to (Value) > - // > - SetMem (ClearMasks, sizeof (ClearMasks), 0); > - SetMem (OrMasks, sizeof (OrMasks), 0); > - > MsrIndex =3D (UINT32)-1; > while ((BaseAddress < BASE_1MB) && (Length !=3D 0)) { > Status =3D MtrrLibProgramFixedMtrr (Type, &BaseAddress, &Length, > &MsrIndex, &ClearMask, &OrMask); > if (RETURN_ERROR (Status)) { > return Status; > } > - ClearMasks[MsrIndex] =3D ClearMask; > - OrMasks[MsrIndex] =3D OrMask; > - Modified[MsrIndex] =3D TRUE; > - LocalModified[MsrIndex] =3D TRUE; > - } > - > - for (MsrIndex =3D 0; MsrIndex < ARRAY_SIZE (mMtrrLibFixedMtrrTable); > MsrIndex++) { > - if (LocalModified[MsrIndex]) { > - FixedSettings->Mtrr[MsrIndex] =3D (FixedSettings->Mtrr[MsrIndex] & > ~ClearMasks[MsrIndex]) | OrMasks[MsrIndex]; > - } > + ClearMasks[MsrIndex] =3D ClearMasks[MsrIndex] | ClearMask; > + OrMasks[MsrIndex] =3D (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 (MtrrSett= ing- > >Variables.Mtrr)]; >=20 > - BOOLEAN FixedSettingsModified[ARRAY_SIZE > (mMtrrLibFixedMtrrTable)]; > - MTRR_FIXED_SETTINGS WorkingFixedSettings; > + UINT64 ClearMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTabl= e)]; > + UINT64 OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)]= ; >=20 > 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); >=20 > // > // 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 =3D 0; Index < RangeCount; Index++) { > if (Ranges[Index].BaseAddress >=3D BASE_1MB) { > continue; > } >=20 > Status =3D 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 =3D 0; Index < ARRAY_SIZE (FixedSettingsModified); Index++)= { > - if (FixedSettingsModified[Index]) { > + for (Index =3D 0; Index < ARRAY_SIZE (ClearMasks); Index++) { > + if (ClearMasks[Index] !=3D 0) { > if (MtrrSetting !=3D NULL) { > - MtrrSetting->Fixed.Mtrr[Index] =3D WorkingFixedSettings.Mtrr[Ind= ex]; > + MtrrSetting->Fixed.Mtrr[Index] =3D > + (MtrrSetting->Fixed.Mtrr[Index] & ~ClearMasks[Index]) | > + OrMasks[Index]; > } else { > if (!MtrrContextValid) { > MtrrLibPreMtrrChange (&MtrrContext); > MtrrContextValid =3D TRUE; > } > - AsmWriteMsr64 ( > - mMtrrLibFixedMtrrTable[Index].Msr, > - WorkingFixedSettings.Mtrr[Index] > - ); > + AsmMsrAndThenOr64 (mMtrrLibFixedMtrrTable[Index].Msr, > + ~ClearMasks[Index], OrMasks[Index]); > } > } > } > -- > 2.20.1.windows.1