From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in7.apple.com (mail-out7.apple.com [17.151.62.29]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5A9E51A1E29 for ; Wed, 10 Aug 2016 10:35:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1470850511; x=2334764111; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=EHjOrGI9iclZjuvRj0IMLOURJ/BaN8SvJSyUJyDJ4P0=; b=iuHp98a+42g5qirs/T9lMQrj/j7ABrw1Kwjsff6D66INTtZjZPmxcHyHx7jHLbqk 69vqsgqw7qk0DsD0GTo+NPgOyVi8MGKEakUkATTbaI4Y3ZnvIpcs38c9qKjxwW+y uQrwF71UmXs7SIz5rliA+J6eH4ABS+yqVxoS46oZAk0zTkkyTZtSX8U3PVodgu0g 0UyTSAPWHQTH7ABlBrU42IyLYJRSgOd1TcYLKu8q4VXQ6X2w6EmlDYid6ZCPm3Uf rN83C9qFlHLSJyaOXwwEqDXLx142SWut34QbkuMewG0ZnVjtJw10wZt82JEI5DmJ 95UbYjEZ+2SmPu6jhHm5KQ==; Received: from relay2.apple.com (relay2.apple.com [17.128.113.67]) by mail-in7.apple.com (Apple Secure Mail Relay) with SMTP id E3.DE.17422.FC56BA75; Wed, 10 Aug 2016 10:35:11 -0700 (PDT) X-AuditID: 11973e16-f793f6d00000440e-b6-57ab65cf2ee8 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by relay2.apple.com (Apple SCV relay) with SMTP id CC.F0.01452.FC56BA75; Wed, 10 Aug 2016 10:35:11 -0700 (PDT) MIME-version: 1.0 Received: from [17.153.29.18] by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.1.1.0 64bit (built Jun 15 2016)) with ESMTPSA id <0OBP0091OFIM2Q10@nwk-mmpp-sz12.apple.com>; Wed, 10 Aug 2016 10:35:11 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish In-reply-to: Date: Wed, 10 Aug 2016 10:35:10 -0700 Cc: "Zeng, Star" , edk2-devel Message-id: <50FCE63D-C671-481E-B321-B0D06356A871@apple.com> References: <776695E1-E864-42F4-A633-B86FB913A510@apple.com> <0C09AFA07DD0434D9E2A0C6AEB048310036A1BCF@shsmsx102.ccr.corp.intel.com> <33692B19-74EE-4C13-8AE3-1C92B74836C3@apple.com> <9e979fb5-52ac-efe4-480e-8670d8233a7c@intel.com> To: Mike Kinney X-Mailer: Apple Mail (2.3112) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrDLMWRmVeSWpSXmKPExsUi2FDorHs+dXW4we1eNYs9h44yW3R0/GOy 2Ndr7cDssXjPSyaP7tn/WAKYorhsUlJzMstSi/TtErgybi7dylxwwa9i7/757A2Mqx27GDk5 JARMJLpnzWWHsMUkLtxbz9bFyMUhJLCXUWJd+yo2mKIZHyeyQiQOMUrc+j2FBSTBKyAo8WPy PSCbg4NZQF7i4HlZkDCzgJbE90etLBD1bxklZq7YyQqSEBYQl3h3ZhMzSEJYYBajxIsFM8AG sQkoS6yY/wHsDE6BMIkTbfOZQGwWAVWJP3v/MENM9ZG41rKTGWKxjcTF/Z+ZIDa8ZJLYOeks K8gVIgI6Et0royGulpXYt2EB2DsSAnvYJJ7/PcE+gVFkFpLDZyEcPgvJ4QsYmVcxCuUmZubo ZuaZ6yUWFOSk6iXn525iBAX/dDuxHYwPV1kdYhTgYFTi4d3AtCpciDWxrLgy9xCjNAeLkjjv NbfV4UIC6YklqdmpqQWpRfFFpTmpxYcYmTg4pRoYY2tXSX0WWfisiOXz9eWX/QWCnh7U3m95 8mhrqJ2FYuHkL2tjS92V41vjKnxXl+3X3WnBx+sz7YTbttP1qkdfv/paydVlXSewVE249YeJ 2zW9x9LNe2YfUfV25z/AUam5TXdaWPHJgv86C+dv+LybRaR0Irvnjo6pP04JeYdd+igm+4th QspGJZbijERDLeai4kQArZrhal8CAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpnkeLIzCtJLcpLzFFi42IRbCg+o3s+dXW4wednChZ7Dh1ltujo+Mdk sa/X2oHZY/Gel0we3bP/sQQwRXHZpKTmZJalFunbJXBl3Fy6lbnggl/F3v3z2RsYVzt2MXJy SAiYSMz4OJEVwhaTuHBvPVsXIxeHkMAhRolbv6ewgCR4BQQlfky+B2RzcDALyEscPC8LEmYW 0JL4/qiVBaL+LaPEzBU7wQYJC4hLvDuziRkkISwwi1HixYIZYIPYBJQlVsz/wA5icwqESZxo m88EYrMIqEr82fuHGWKqj8S1lp3MEIttJC7u/8wEseElk8TOSWdZQa4QEdCR6F4ZDXG1rMS+ DQvYJjAKzkJy6yyEW2chuXUBI/MqRoGi1JzESiO9xIKCnFS95PzcTYzgYC103sF4bJnVIUYB DkYlHt4NTKvChVgTy4orc4GBwcGsJMIrnLQ6XIg3JbGyKrUoP76oNCe1+BBjMtD9E5mlRJPz gZGUVxJvaGJiYGJsbGZsbG5iTpqwkjjvI/ml4UIC6YklqdmpqQWpRTBbmDg4pRoYd6Wv7J06 S+NnZ+3Dj8t4BdO2nTII+RItKz+/dGaBbMnzcztWqf/+eURNdwFTaesG46TH1Q5/1J8V/BfQ clz2QDiz4uicyB2sT2U2Xzd7UjH1a0md88MSo8epfxhvL3mpULrJJS1Az2unEqfYsanyp7QM OH982C4Y+VZd1XELM+P+fbm6PC3ySizFGYmGWsxFxYkA1zzUi5oCAAA= Subject: Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Aug 2016 17:35:12 -0000 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII > On Aug 10, 2016, at 10:22 AM, Kinney, Michael D 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-bounces@lists.01.org] On Behalf Of Andrew Fish >> Sent: Tuesday, August 9, 2016 7:50 AM >> To: Zeng, Star >> Cc: edk2-devel >> 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 wrote: >>> >>> On 2016/8/9 10:07, Andrew Fish wrote: >>>> >>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star 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-bounces@lists.01.org] On Behalf Of Andrew >> Fish >>>>> Sent: Tuesday, August 9, 2016 7:08 AM >>>>> To: edk2-devel >>>>> 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