From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::342; helo=mail-wm1-x342.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BB7972119707F for ; Mon, 10 Dec 2018 04:26:02 -0800 (PST) Received: by mail-wm1-x342.google.com with SMTP id a62so3298893wmh.4 for ; Mon, 10 Dec 2018 04:26:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=poucgJ/vRjFn3JhuF3abzGF5jCUomSYGvlWEKpwWEPc=; b=f7xRRG34MabVQSHjizpWkaquTsNz6R74p6DkcbqhXdrQ+sHCKaaQ1CRJytE1SK0JBe XG0S/lPXYHI2eX77ase8WmSfZddoGuZQ9U4HckRt8co7PEORjwU4Gn8ypM6Lpz3zHWja iMyl7gs+6+SGuj389K53RnsjTwo7GhqBjjsKA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=poucgJ/vRjFn3JhuF3abzGF5jCUomSYGvlWEKpwWEPc=; b=fT3SxXFW6wMxvkLz6cMOjNWd/R4YByWlNAQYhdPLY8QGOcuDC7qdNnmTV8KQZRa6nn AneLC6YSXiSq6COBrAKEjQzpAXr760vo6XARrpPl1g/5Ozte8HKj3M+NN1Cik0AFaU6Q YyTca8V/6zaDPgsPB1J72VTcw9IVK7LqU1MANcHbhzI9bloTItEWO3t8pfCVlclLBP3/ bjmfjuH+IHOPwSa4JaV9BIZfDoHzcHejgohmYZAODMfh6pk6cu2E/P2rU0Vv5Q8Op5MN Txy7DY4q9q75cmhMHAlaJ26BEEWOTD1C3/EgDMsMEgPoOhTPA8AVIqFx8s4uK1GNYxBG LtZA== X-Gm-Message-State: AA+aEWa0QRJ/H+hABX/Y5GcRauiasult/jOl62Kqo2jImSrXB+SGqBv4 c+ppXF/eN/35zB+1+YaDjKHdww== X-Google-Smtp-Source: AFSGD/XWFyMAwiVoEQZwNXc7VAp93kFgHJeRixEO+Jd9tZl6FVzM/MEyj7EIspElHZpN0TTE1fM8fQ== X-Received: by 2002:a1c:8d49:: with SMTP id p70mr11199152wmd.68.1544444761157; Mon, 10 Dec 2018 04:26:01 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id f15sm11917955wrt.10.2018.12.10.04.26.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 10 Dec 2018 04:26:00 -0800 (PST) Date: Mon, 10 Dec 2018 12:25:58 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, hao.a.wu@intel.com, ruiyu.ni@intel.com Message-ID: <20181210122558.k5aadqilfmxt2t7v@bivouac.eciton.net> References: <20181208102431.9185-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20181208102431.9185-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) 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 12:26:03 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 I agree with the architectural rationale. However, I also decided to have a look. $ git grep "} HII_VENDOR_DEVICE_PATH;" EdkCompatibilityPkg/Compatibility/FrameworkHiiOnUefiHiiThunk/HiiDatabase.h:} HII_VENDOR_DEVICE_PATH; EdkCompatibilityPkg/Foundation/Library/Dxe/UefiEfiIfrSupportLib/UefiIfrLibrary.h:} HII_VENDOR_DEVICE_PATH; EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.c:} HII_VENDOR_DEVICE_PATH; EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.c:} HII_VENDOR_DEVICE_PATH; 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_PATH; MdeModulePkg/Library/DeviceManagerUiLib/DeviceManager.h:} HII_VENDOR_DEVICE_PATH; MdeModulePkg/Library/FileExplorerLib/FileExplorer.h:} HII_VENDOR_DEVICE_PATH; 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_VENDOR_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_PATH; 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/PwdCredentialProvider.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/SecureBootConfigImpl.h:} HII_VENDOR_DEVICE_PATH; Vlv2TbltDevicePkg/Library/PlatformBdsLib/BdsPlatform.h:} HII_VENDOR_DEVICE_PATH; Vlv2TbltDevicePkg/PlatformSetupDxe/PlatformSetupDxe.c:} HII_VENDOR_DEVICE_PATH; 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). Does this definition belong in a central header somewhere rather than in 29 (and counting) different locations, implemented differently? / Leif > --- > MdeModulePkg/Library/FileExplorerLib/FileExplorer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.h b/MdeModulePkg/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_CALLBACK_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