Laszlo, I think the function name confused you. MtrrLibApplyVariableMtrrs() is not to apply the MTRR setting to CPU/HW. It's to apply the setting read from CPU/HW to the range array stored in memory. It doesn't have side effect.
The basic idea of MtrrDebugPrintAllMtrrsWorker() is to read the setting from HW/CPU, Convert that setting to range array. Then dump the range array. -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Tuesday, October 17, 2017 3:56 PM To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Fix MtrrDebugPrintAllMtrrsWorker to avoid hang On 10/17/17 03:46, Ruiyu Ni wrote: > ARRAY_SIZE(Mtrrs->Variables.Mtrr) was used in > MtrrDebugPrintAllMtrrsWorker() to parse the MTRR registers. > Instead, the actual variable MTRR count should be used. > Otherwise, the uninitialized random data in MtrrSetting may cause > MtrrLibSetMemoryType() hang. > > Steven Shi found this bug in QEMU when using Q35 chip. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> > Cc: Steven Shi <steven....@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > --- > UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > index 2fd1d0153e..cb22558103 100644 > --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c > @@ -2776,6 +2776,7 @@ MtrrDebugPrintAllMtrrsWorker ( > UINTN RangeCount; > UINT64 MtrrValidBitsMask; > UINT64 MtrrValidAddressMask; > + UINT32 VariableMtrrCount; > MTRR_MEMORY_RANGE Ranges[ > ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE > (Mtrrs->Variables.Mtrr) + 1 > ]; > @@ -2785,6 +2786,8 @@ MtrrDebugPrintAllMtrrsWorker ( > return; > } > > + VariableMtrrCount = GetVariableMtrrCountWorker (); > + > if (MtrrSetting != NULL) { > Mtrrs = MtrrSetting; > } else { > @@ -2802,7 +2805,7 @@ MtrrDebugPrintAllMtrrsWorker ( > DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d] : %016lx\n", Index, > Mtrrs->Fixed.Mtrr[Index])); > } > > - for (Index = 0; Index < ARRAY_SIZE (Mtrrs->Variables.Mtrr); Index++) { > + for (Index = 0; Index < VariableMtrrCount; Index++) { > if (((MSR_IA32_MTRR_PHYSMASK_REGISTER > *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) { > // > // If mask is not valid, then do not display range @@ > -2829,11 +2832,11 @@ MtrrDebugPrintAllMtrrsWorker ( > RangeCount = 1; > > MtrrLibGetRawVariableRanges ( > - &Mtrrs->Variables, ARRAY_SIZE (Mtrrs->Variables.Mtrr), > + &Mtrrs->Variables, VariableMtrrCount, > MtrrValidBitsMask, MtrrValidAddressMask, RawVariableRanges > ); > MtrrLibApplyVariableMtrrs ( > - RawVariableRanges, ARRAY_SIZE (RawVariableRanges), > + RawVariableRanges, VariableMtrrCount, > Ranges, ARRAY_SIZE (Ranges), &RangeCount > ); > > Assuming this patch is not committed yet: Reviewed-by: Laszlo Ersek <ler...@redhat.com> I have another (independent) comment: This function is currently named "MtrrDebugPrintAllMtrrsWorker", and its leading comment says "Worker function prints all MTRRs for debugging." Because of this name and the documentation, I didn't understand initially how the problem could cause a hang, given that none of the printing loops would actually access anything out-of-bounds. Some of the information printed would be garbage, but it should not cause a hang. That's when I noticed that the function actually *applies* MTRR settings, by calling MtrrLibApplyVariableMtrrs(). Even worse, the MtrrLibApplyVariableMtrrs() and MtrrLibApplyFixedMtrrs() function calls are wrapped by DEBUG_CODE(). This means that in a DEBUG/NOOPT build, the function will apply MTRR settings, and in a RELEASE build, it won't. I think this is wrong and should be fixed. A debug function (esp. one that behaves differently in DEBUG/NOOPT vs. RELEASE) should have no side effects. The current situation is similar to: ASSERT (FunctionWithSideEffects () == EXPECTED_RETURN_VALUE); which we all know is wrong. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
