> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, February 01, 2019 5:18 PM > To: Wu, Hao A; edk2-devel@lists.01.org > Cc: Zeng, Star > Subject: Re: [edk2] [PATCH v3 10/12] MdeModulePkg/SmmLockBox(PEI): > Remove an ASSERT in RestoreLockBox() > > On 02/01/19 06:47, Hao Wu wrote: > > This commit is out of the scope for BZ-1409. It is a refinement for the > > PEI library instance within SmmLockBoxLib. > > > > For the below ASSERT statement within function RestoreLockBox(): > > Status = SmmCommunicationPpi->Communicate ( > > SmmCommunicationPpi, > > &CommBuffer[0], > > &CommSize > > ); > > if (Status == EFI_NOT_STARTED) { > > // > > // Pei SMM communication not ready yet, so we access SMRAM directly > > // > > DEBUG ((DEBUG_INFO, "SmmLockBoxPeiLib Communicate - (%r)\n", > Status)); > > Status = InternalRestoreLockBoxFromSmram (Guid, Buffer, Length); > > LockBoxParameterRestore->Header.ReturnStatus = (UINT64)Status; > > if (Length != NULL) { > > LockBoxParameterRestore->Length = (UINT64)*Length; > > } > > } > > ASSERT_EFI_ERROR (Status); > > > > It is possible for previous codes to return an error status that is > > possible for happen. One example is that, when the 'if' statement > > 'if (Status == EFI_NOT_STARTED) {' is entered, function > > InternalRestoreLockBoxFromSmram() is possible to return > 'BUFFER_TOO_SMALL' > > if the caller of RestoreLockBox() provides a buffer that is too small to > > hold the content of LockBox. > > > > Thus, this commit will remove the ASSERT here. > > > > Please note that the current implementation of RestoreLockBox() is > > handling the above-mentioned error case properly, so no additional error > > handling codes are needed here. > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Star Zeng <star.z...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Hao Wu <hao.a...@intel.com> > > Reviewed-by: Ray Ni <ray...@intel.com> > > --- > > MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > index 9f73480070..8c3e65bc96 100644 > > --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c > > @@ -623,7 +623,6 @@ RestoreLockBox ( > > LockBoxParameterRestore->Length = (UINT64)*Length; > > } > > } > > - ASSERT_EFI_ERROR (Status); > > > > if (Length != NULL) { > > *Length = (UINTN)LockBoxParameterRestore->Length; > > > > OVMF never reaches this code path because it doesn't include an > EFI_PEI_SMM_COMMUNICATION_PPI instance. Therefore OVMF always goes > through InternalRestoreLockBoxFromSmram(). > > See commit bd3afeb1d62c ("MdeModulePkg: SmmLockBoxPeiLib: work without > EFI_PEI_SMM_COMMUNICATION_PPI", 2015-11-16). > > In that regard, I'm OK with this patch; this alone would suffice for me > to give an Acked-by. > > However, having re-reviewed bd3afeb1d62c now, I see that this patch is > actually technically correct. So I believe I can give an R-b too, > despite OVMF not using the affected code path. > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > Other than that, you might want to review existing callers of this > function, to ensure they don't rely on any such failure being caught > internally to the function (via the ASSERT that's now being removed). > Again, this would only be relevant for platforms that produce an > EFI_PEI_SMM_COMMUNICATION_PPI instance.
Sure. Thanks for the reminder. I will check those callers before pushing this change. Best Regards, Hao Wu > > Thanks > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
