Re: [PATCH] MdeModulePkg/FileExplorerLib: avoid packed struct for program data

[Leif Lindholm <leif.lindholm@linaro.org>]

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 <ard.biesheuvel@linaro.org>

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