From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 00D6B21196815 for ; Mon, 10 Dec 2018 06:39:05 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Dec 2018 06:39:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,338,1539673200"; d="scan'208";a="128670797" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 10 Dec 2018 06:39:05 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 10 Dec 2018 06:39:05 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 10 Dec 2018 06:39:04 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.203]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.46]) with mapi id 14.03.0415.000; Mon, 10 Dec 2018 22:39:02 +0800 From: "Gao, Liming" To: Leif Lindholm , Ard Biesheuvel CC: "Wu, Hao A" , "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [PATCH] MdeModulePkg/FileExplorerLib: avoid packed struct for program data Thread-Index: AQHUjuA+mHWXsX77i0eljVUOMusAiKV3YyUAgACMs8A= Date: Mon, 10 Dec 2018 14:39:01 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E3898D4@SHSMSX104.ccr.corp.intel.com> References: <20181208102431.9185-1-ard.biesheuvel@linaro.org> <20181210122558.k5aadqilfmxt2t7v@bivouac.eciton.net> In-Reply-To: <20181210122558.k5aadqilfmxt2t7v@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYmE1OTk4MWMtZDI0Mi00NGUxLTlhNDItYWFmZmE3MzRhM2QwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiblJDYjh0OStSMzEwOHdZTVJlV0xuNk5BR3ZmNUhCOXFuc09mS0UwUW5zRXg3ZE9SNmh2ZzVhNEw0WHcwc1NlSyJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdeModulePkg/FileExplorerLib: avoid packed struct for program data X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2018 14:39:06 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ard: I agree this change. Leif: On the duplicated definition of HII_VENDOR_DEVICE_PATH, MdeModulePkg Driv= erSampleDxe first defines it and use it as the driver level definition. Oth= er UEFI HII driver may refer to the driver sample driver and define the sam= e structure. One centralized header file can be defined for the default HII= vendor device path. If the driver wants to use the different one, it can d= efine its own structure.=20 Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Le= if Lindholm > Sent: Monday, December 10, 2018 8:26 PM > To: Ard Biesheuvel > Cc: Wu, Hao A ; Ni, Ruiyu ; edk2-= devel@lists.01.org > Subject: Re: [edk2] [PATCH] MdeModulePkg/FileExplorerLib: avoid packed st= ruct for program data >=20 > On Sat, Dec 08, 2018 at 11:24:31AM +0100, Ard Biesheuvel wrote: > > Struct packing is only necessary for data structures whose in-memory > > representation is covered by the PI or UEFI specs, and may deviate > > from the ordinary C rules for alignment. > > > > So in case of FileExplorerLib, this applies to the device path struct > > only, and other structures used to carry program data should not be > > packed, or we may end up with alignment faults on architectures such > > as ARM, which don't permit load/store double or multiple instructions > > to access memory locations that are not 32-bit aligned. > > > > E.g., the following call in FileExplorerLibConstructor() > > > > InitializeListHead (&gFileExplorerPrivate.FsOptionMenu->Head); > > > > which is emitted as follows for 32-bit ARM/Thumb2 by Clang-5.0 > > > > 3de0: b510 push {r4, lr} > > 3de2: 4604 mov r4, r0 > > ... > > 3de8: e9c4 4400 strd r4, r4, [r4] > > 3dec: bd10 pop {r4, pc} > > > > will perform a double-word store on the first argument, passed in > > register r0, assuming that the pointer type of the argument is > > enough to guarantee that the value is suitably aligned. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel >=20 > I agree with the architectural rationale. >=20 > However, I also decided to have a look. >=20 > $ git grep "} HII_VENDOR_DEVICE_PATH;" > EdkCompatibilityPkg/Compatibility/FrameworkHiiOnUefiHiiThunk/HiiDatabase.= h:} HII_VENDOR_DEVICE_PATH; > EdkCompatibilityPkg/Foundation/Library/Dxe/UefiEfiIfrSupportLib/UefiIfrLi= brary.h:} HII_VENDOR_DEVICE_PATH; > EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c:} HII_VENDOR_DEVICE_P= ATH; > EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c:} HII_VENDOR_DEVICE_PAT= H; > IntelFrameworkModulePkg/Library/LegacyBootMaintUiLib/LegacyBootMaintUi.h:= } HII_VENDOR_DEVICE_PATH; > IntelFrameworkModulePkg/Universal/BdsDxe/Bds.h:} HII_VENDOR_DEVICE_PATH; > MdeModulePkg/Application/UiApp/Ui.h:} HII_VENDOR_DEVICE_PATH; > MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanceManager.h= :} HII_VENDOR_DEVICE_PATH; > MdeModulePkg/Library/BootManagerUiLib/BootManager.h:} HII_VENDOR_DEVICE_P= ATH; > MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h:} HII_VENDOR_DEVI= CE_PATH; > MdeModulePkg/Library/FileExplorerLib/FileExplorer.h:} HII_VENDOR_DEVICE_P= ATH; > MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanup.h:} HII_VENDOR_= DEVICE_PATH; > MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h:} HII_VENDOR_DEVICE_= PATH; > MdeModulePkg/Universal/DriverSampleDxe/DriverSample.h:} HII_VENDOR_DEVICE= _PATH; > MdeModulePkg/Universal/HiiResourcesSampleDxe/HiiResourcesSample.c:} HII_V= ENDOR_DEVICE_PATH; > MdeModulePkg/Universal/Network/IScsiDxe/IScsiConfig.h:} HII_VENDOR_DEVICE= _PATH; > MdeModulePkg/Universal/PlatformDriOverrideDxe/PlatDriOverrideDxe.c:} HII_= VENDOR_DEVICE_PATH; > NetworkPkg/IScsiDxe/IScsiConfig.h:} HII_VENDOR_DEVICE_PATH; > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h:} HII_VENDOR_DEVICE_PATH; > QuarkPlatformPkg/Platform/Dxe/Setup/SetupPlatform.h:} HII_VENDOR_DEVICE_P= ATH; > SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.h:} HII_VENDOR_DEVICE_PATH; > SecurityPkg/Tcg/Tcg2Config/Tcg2ConfigImpl.h:} HII_VENDOR_DEVICE_PATH; > SecurityPkg/Tcg/TcgConfigDxe/TcgConfigImpl.h:} HII_VENDOR_DEVICE_PATH; > SecurityPkg/UserIdentification/PwdCredentialProviderDxe/PwdCredentialProv= ider.h:} HII_VENDOR_DEVICE_PATH; > SecurityPkg/UserIdentification/UserIdentifyManagerDxe/UserIdentifyManager= .h:} HII_VENDOR_DEVICE_PATH; > SecurityPkg/UserIdentification/UserProfileManagerDxe/UserProfileManager.h= :} HII_VENDOR_DEVICE_PATH; > SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImp= l.h:} HII_VENDOR_DEVICE_PATH; > Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h:} HII_VENDOR_DEVIC= E_PATH; > Vlv2TbltDevicePkg/PlatformSetupDxe/PlatformSetupDxe.c:} HII_VENDOR_DEVICE= _PATH; >=20 > Now, I will confess to being lazy and not manually checking all of > them, but 100% of the ones I did check were identical (except in > packedness). >=20 > Does this definition belong in a central header somewhere rather than > in 29 (and counting) different locations, implemented differently? >=20 > / > Leif >=20 > > --- > > MdeModulePkg/Library/FileExplorerLib/FileExplorer.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h b/MdeM= odulePkg/Library/FileExplorerLib/FileExplorer.h > > index bf1450dbd581..603185abe4b1 100644 > > --- a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h > > +++ b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h > > @@ -51,6 +51,8 @@ typedef struct { > > EFI_DEVICE_PATH_PROTOCOL End; > > } HII_VENDOR_DEVICE_PATH; > > > > +#pragma pack() > > + > > typedef struct { > > EFI_HANDLE DeviceHandle; > > EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > @@ -100,8 +102,6 @@ typedef struct { > > > > #define FILE_EXPLORER_PRIVATE_FROM_THIS(a) CR (a, FILE_EXPLORER_CALLB= ACK_DATA, FeConfigAccess, > FILE_EXPLORER_CALLBACK_DATA_SIGNATURE) > > > > -#pragma pack() > > - > > extern UINT8 FileExplorerVfrBin[]; > > > > #define MENU_OPTION_SIGNATURE SIGNATURE_32 ('m', 'e', 'n', 'u') > > -- > > 2.19.2 > > > > _______________________________________________ > > 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