Hi Mike, All I did was copy the existing error handling in the scenario where we are unable to set up the buffer for any reason:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c#L191 I agree that seems a little odd, but I haven’t investigated the ramifications of what this unhappy path is from an overall capsule flow perspective. Regardless of that… I feel like figuring that out and making the capsule flow fail completely either in the case of a memory allocation failure or a SetVariable() failure should probably be a different patch series. Don’t let perfect be the enemy of good. Thanks, Nate From: Kinney, Michael D <michael.d.kin...@intel.com> Date: Thursday, November 3, 2022 at 2:23 PM To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>, devel@edk2.groups.io <devel@edk2.groups.io>, Kinney, Michael D <michael.d.kin...@intel.com> Cc: Gao, Liming <gaolim...@byosoft.com.cn>, Jiang, Guomin <guomin.ji...@intel.com>, Wang, Jian J <jian.j.w...@intel.com> Subject: RE: [PATCH V4] MdeModulePkg: Memory Corruption Error in CapsuleRuntimeDxe Also...would it be a simpler policy to fail the capsule update all together if any of the 3 allocations fail? That way, there is no case where is "may fail". Mike > -----Original Message----- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Thursday, November 3, 2022 2:21 PM > To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; > devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Jiang, Guomin > <guomin.ji...@intel.com>; Wang, Jian J <jian.j.w...@intel.com> > Subject: RE: [PATCH V4] MdeModulePkg: Memory Corruption Error in > CapsuleRuntimeDxe > > Hi Nate, > > The "may fail" messages look a bit odd. Is this due to the fact that > CapsuleRuntimeDxe is in X64 mode, > but this module does not know if PEI Phase will process the capsule in IA32 > or X64 execution mode? > > We have a PCD that is set if the DXE IPL needs to switch modes. Can we use > that information? > > These "may fail" messages will only be generated if there is enough memory to > allocate the capsule > image, but not the page tables and/or stack. Correct? > > Thanks, > > Mike > > > -----Original Message----- > > From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com> > > Sent: Tuesday, October 25, 2022 3:30 PM > > To: devel@edk2.groups.io > > Cc: Gao, Liming <gaolim...@byosoft.com.cn>; Jiang, Guomin > > <guomin.ji...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > > Kinney, Michael D <michael.d.kin...@intel.com> > > Subject: [PATCH V4] MdeModulePkg: Memory Corruption Error in > > CapsuleRuntimeDxe > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4112 > > > > In AllocateReservedMemoryBelow4G(), if gBS->AllocatePages() > > returns an error, and ASSERTs are disabled, then the > > function will overwrite memory from 0xFFFFFFFF -> (0xFFFFFFFF + Size). > > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Guomin Jiang <guomin.ji...@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Signed-off-by: Nate DeSimone <nathaniel.l.desim...@intel.com> > > --- > > .../X64/SaveLongModeContext.c | 25 ++++++++++++++++--- > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > diff --git > > a/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c > > b/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c > > index dab297dd0a..a8c5de8764 100644 > > --- a/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c > > +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/X64/SaveLongModeContext.c > > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > @param Size Size of memory to allocate. > > > > @return Allocated Address for output. > > + @return NULL - Memory allocation failed. > > > > **/ > > VOID * > > @@ -59,7 +60,15 @@ AllocateReservedMemoryBelow4G ( > > Pages, > > &Address > > ); > > - ASSERT_EFI_ERROR (Status); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "ERROR AllocateReservedMemoryBelow4G(): %r\n", > > Status)); > > + return NULL; > > + } > > + > > + if (Address == 0) { > > + DEBUG ((DEBUG_ERROR, "ERROR AllocateReservedMemoryBelow4G(): > > AllocatePages() returned NULL")); > > + return NULL; > > + } > > > > Buffer = (VOID *)(UINTN)Address; > > ZeroMem (Buffer, Size); > > @@ -159,14 +168,23 @@ PrepareContextForCapsulePei ( > > DEBUG ((DEBUG_INFO, "CapsuleRuntimeDxe X64 TotalPagesNum - 0x%x > > pages\n", TotalPagesNum)); > > > > LongModeBuffer.PageTableAddress = > > (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedMemoryBelow4G > > (EFI_PAGES_TO_SIZE > > (TotalPagesNum)); > > - ASSERT (LongModeBuffer.PageTableAddress != 0); > > + if (LongModeBuffer.PageTableAddress == 0) { > > + DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be > > saved, ")); > > + DEBUG ((DEBUG_ERROR, "PageTableAddress allocation failed. Capsule in > > PEI may fail!\n")); > > + return; > > + } > > > > // > > // Allocate stack > > // > > LongModeBuffer.StackSize = PcdGet32 > > (PcdCapsulePeiLongModeStackSize); > > LongModeBuffer.StackBaseAddress = > > (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateReservedMemoryBelow4G (PcdGet32 > > (PcdCapsulePeiLongModeStackSize)); > > - ASSERT (LongModeBuffer.StackBaseAddress != 0); > > + if (LongModeBuffer.StackBaseAddress == 0) { > > + DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be > > saved, ")); > > + DEBUG ((DEBUG_ERROR, "StackBaseAddress allocation failed. Capsule in > > PEI may fail!\n")); > > + gBS->FreePages (LongModeBuffer.PageTableAddress, TotalPagesNum); > > + return; > > + } > > > > Status = gRT->SetVariable ( > > EFI_CAPSULE_LONG_MODE_BUFFER_NAME, > > @@ -189,6 +207,7 @@ PrepareContextForCapsulePei ( > > ); > > } else { > > DEBUG ((DEBUG_ERROR, "FATAL ERROR: CapsuleLongModeBuffer cannot be > > saved: %r. Capsule in PEI may fail!\n", Status)); > > + gBS->FreePages (LongModeBuffer.PageTableAddress, TotalPagesNum); > > gBS->FreePages (LongModeBuffer.StackBaseAddress, EFI_SIZE_TO_PAGES > > (LongModeBuffer.StackSize)); > > } > > } > > -- > > 2.27.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#96160): https://edk2.groups.io/g/devel/message/96160 Mute This Topic: https://groups.io/mt/94569829/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
