> On Aug 10, 2016, at 10:22 AM, Kinney, Michael D <michael.d.kin...@intel.com> > wrote: > > Hi Andrew, > > I am staring at the code. Something is not right here, but I am > concerned that there are use cases I am not considering yet. Here > is my analysis so far. > > Here are the lib instances I see: > (ignoring IntelFramework ones for the purposes of this discussion). > > MdeModulePkg\Library\DxeReportStatusCodeLib\DxeReportStatusCodeLib.inf(25): > LIBRARY_CLASS = ReportStatusCodeLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER > DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE > > MdeModulePkg\Library\PeiReportStatusCodeLib\PeiReportStatusCodeLib.inf(27): > LIBRARY_CLASS = ReportStatusCodeLib|SEC PEIM PEI_CORE > > MdeModulePkg\Library\RuntimeDxeReportStatusCodeLib\RuntimeDxeReportStatusCodeLib.inf(23): > LIBRARY_CLASS = ReportStatusCodeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER > > MdeModulePkg\Library\SmmReportStatusCodeLib\SmmReportStatusCodeLib.inf(26): > LIBRARY_CLASS = ReportStatusCodeLib|DXE_SMM_DRIVER SMM_CORE > > MdePkg\Library\BaseReportStatusCodeLibNull\BaseReportStatusCodeLibNull.inf(23): > LIBRARY_CLASS = ReportStatusCodeLib > > * The BASE one that is a Null instance makes sense when disabling Report > Status Code. > * The PEI one makes sense for its module compatibility. > * The SMM one also makes sense its module compatibility. > * And the RuntimeDxe one also makes sense its module compatibility. > * The DXE one seems to be over specified and if we reduced its module > compatibility, the SMM and RuntimeDxe lib instances can be used to cover > those module types. > > When I look at the source code in DxeReportStatusCodeLib, I see comments > that describe the use case where a module is dispatched before the Report > Status Code Protocol is available. > > The use case you are describing is when the first call to Report Status > Code by a module occurs after ExitBootServices(). In that case, we > depend on the Boot Services table being zeroed to exit early. > > I agree that no component should depend on Boot Services table being > cleared to zeros at ExitBootServices(). It is legal from UEFI/PI spec > to not zero at all, or in your example, fill with a pattern other than > zeros. > > It appears that the use case of first call to Report Status Code after > ExitBootServices() is not covered by the DXE lib instance, and we have > just been getting lucky due to zeroing behavior of boot services table. > > I can think of a couple options: > > * Update DXE INF to remove the DXE Runtime, DXE SAL, SMM, SMM Core module > types. This may break platform builds if a platform is using this lib > mapping for modules of the types being removed. However, those could > be considered platform DSC bugs. > > * Update DXE lib instance to add ExitBootServices() and SetVirtualAddressMap() > events. However, this would basically make the DXE lib instance the > same as the DXE Runtime instance and would increase size of non-RT > components. > > I would prefer the first option, but we would need to do some testing to > see how many platform DSC files break. >
Mike, That seems to make the most sense, but I too was worried about compatibility. I tried adding the UefiRuntimeLib to just fail calls at Runtime, but that breaks the build as DXE_CORE does not support UefiRuntimeLib. I guess one option would be to only add the ExitBootServices event and fail calls at runtime? Thanks, Andrew Fish > Mike > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Andrew Fish >> Sent: Tuesday, August 9, 2016 7:50 AM >> To: Zeng, Star <star.z...@intel.com> >> Cc: edk2-devel <edk2-devel@lists.01.org> >> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to >> DxeReportStatusCodeLib assuming the state of the BootService Memory at >> runtime. >> >> >>> On Aug 8, 2016, at 7:26 PM, Zeng, Star <star.z...@intel.com> wrote: >>> >>> On 2016/8/9 10:07, Andrew Fish wrote: >>>> >>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star <star.z...@intel.com> wrote: >>>>> >>>>> Andrew, >>>>> >>>>> Should MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib be used for >>>>> your case >> if there are really runtime status code reporting needed? >>>>> >>>> >>>> Star, >>>> >>>> If the Library instance does not fully support DXE_RUNTIME_DRIVER, why is >>>> it >> listed in the LIBRARY_CLASS as supported? >>> >>> This kind of library can support DXE_RUNTIME_DRIVER at boot time, for >>> example >> UefiHiiLib, UefiHiiServicesLib, UefiBootServicesTableLib, DxePcdLib and etc. >>> >> >> Star, >> >> I understand giving access to Boot Services resources to a Runtime Driver >> for its >> constructor. I think the difference here is the DxeReportStatusCodeLib is >> abstracting >> a runtime service, but not doing it in a way to really works properly at >> runtime in >> all cases. I would expect that a library abstraction a runtime feature would >> work at >> runtime. Which is why I ended up with a "bad" mapping in the first place. >> >> Thanks, >> >> Andrew Fish >> >>>> >>>> >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeReportStatusCod >> eLib/DxeReportStatusCodeLib.inf#L25 >>>> >>>> LIBRARY_CLASS = ReportStatusCodeLib|DXE_CORE DXE_DRIVER >> DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER >> SMM_CORE >>>> >>>> Actually I tried to add the UefiRuntimeDriverLib and the build failed as >> UefiRuntimeDriverLib was not supported for the DXE_CORE type. Maybe the bug >> is this >> library instance lists DXE_RUNTIME_DRIVER, DXE_SAL_DRIVER and >> DXE_SMM_DRIVER when it >> has special case code to support DXE_CORE? Maybe this library is trying to >> do too >> many things? >>>> >>>> What ReportStatusCodeLib would you recommend to link with RuntimeDxe >>>> driver: >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/RuntimeDxe/RuntimeDxe >> .inf >>> >>> [LibraryClasses.common.DXE_CORE] >>> >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi >> b.inf >>> >>> [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>> >> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRepo >> rtStatusCodeLib.inf >>> >>> [LibraryClasses.common.UEFI_DRIVER] >>> >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi >> b.inf >>> >>> [LibraryClasses.common.DXE_DRIVER] >>> >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi >> b.inf >>> >>> [LibraryClasses.common.UEFI_APPLICATION] >>> >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi >> b.inf >>> >>> Thanks, >>> Star >>> >>>> >>>> Thanks, >>>> >>>> Andrew Fish >>>> >>>>> Thanks, >>>>> Star >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>>>> Andrew >> Fish >>>>> Sent: Tuesday, August 9, 2016 7:08 AM >>>>> To: edk2-devel <edk2-devel@lists.01.org> >>>>> Subject: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to >> DxeReportStatusCodeLib assuming the state of the BootService Memory at >> runtime. >>>>> >>>>> I was messing about with an ExitBootServices test that fills boot >>>>> services memory >> with 0xAFAFAFAFAFAFAFAF (It was Vincent's idea to use my Initials but it has >> the >> handy property of being a non-cononical address and causes on GP fault on >> X64) and >> SetVirtualAddressMap() started crashing. >>>>> >>>>> It looks like this code is assuming the 1st call to ReportStatus code >>>>> will not >> happen at runtime. This is not the case for the RuntimeDxe driver. >>>>> >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeReportStatusCod >> eLib/ReportStatusCodeLib.c#L43 >>>>> VOID >>>>> InternalGetReportStatusCode ( >>>>> VOID >>>>> ) >>>>> { >>>>> EFI_STATUS Status; >>>>> >>>>> if (mReportStatusCodeLibStatusCodeProtocol != NULL) { >>>>> return; >>>>> } >>>>> >>>>> // >>>>> // Check gBS just in case ReportStatusCode is called before gBS is >>>>> initialized. >>>>> // >>>>> if (gBS != NULL && gBS->LocateProtocol != NULL) { >>>>> Status = gBS->LocateProtocol (&gEfiStatusCodeRuntimeProtocolGuid, NULL, >>>>> (VOID**) >> &mReportStatusCodeLibStatusCodeProtocol); >>>>> if (EFI_ERROR (Status)) { >>>>> mReportStatusCodeLibStatusCodeProtocol = NULL; >>>>> } >>>>> } >>>>> } >>>>> >>>>> I'm guessing this seems to work due >> to:https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/DxeMain/DxeMai >> n.c#L803 >>>>> >>>>> // >>>>> // Zero out the Boot Service Table >>>>> // >>>>> ZeroMem (gBS, sizeof (EFI_BOOT_SERVICES)); >>>>> >>>>> >>>>> Thus if I'm looking at this code correctly it only looks like it works at >>>>> Runtime >> since it is depending on the value of a boot services memory buffer not >> changing. >> This is not a valid assumption as that code is owned by the caller of >> ExitBootServices, so it should be legal for my test to change the value. >>>>> >>>>> I wanted to get a few more eyes on this prior to filling a bug? >>>>> >>>>> Thanks, >>>>> >>>>> Andrew Fish >>>>> >>>>> >>>>> _______________________________________________ >>>>> 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 >>> >>> _______________________________________________ >>> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
