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 <[email protected]>
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
> [email protected]
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
