Re: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct

["Yao, Jiewen" <jiewen.yao@intel.com>]

HI Ard
Thanks to raise this issue.

I agree with you on pack.

I think there is still some confusing in " The EFI System Table entry must point to an array of capsules that contain the same CapsuleGuid value."

I am still not clear if it is a proper way to use "an array of capsule headers" to represent the "an array of capsules".

I recommend we ask USWG to clarify the meaning to avoid misunderstanding.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, March 3, 2017 8:17 PM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; leif.lindholm@linaro.org; Kinney,
> Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
> 
> The UEFI specification describes the per-capsule type configuration
> table entry as follows:
> 
>   The EFI System Table entry must use the GUID from the CapsuleGuid
>   field of the EFI_CAPSULE_HEADER. The EFI System Table entry must point
>   to an array of capsules that contain the same CapsuleGuid value. The
>   array must be prefixed by a UINT32 that represents the size of the array
>   of capsules.
> 
> In the current EDK2 implementation, this is translated into the following
> 
>   typedef struct {
>     ///
>     /// the size of the array of capsules.
>     ///
>     UINT32   CapsuleArrayNumber;
>     ///
>     /// Point to an array of capsules that contain the same CapsuleGuid value.
>     ///
>     VOID*    CapsulePtr[1];
>   } EFI_CAPSULE_TABLE;
> 
> Note that this implements an array of capsule *pointers* rather than an
> array of capsules. Also, it lacks the #pragma pack(1), resulting in
> padding to be added after the CapsuleArrayNumber.
> 
> So let's bring this code in line with the UEFI spec and
> - put the *size* of the array in the leading UINT32 rather than the number
>   of entries,
> - pack the struct to remove any padding on 64-bit architectures
> - replace the array of pointers with an array of capsule headers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EdkCompatibilityPkg/Foundation/Framework/Include/EfiCapsule.h | 6 ++++--
>  IntelFrameworkModulePkg/Universal/BdsDxe/Capsules.c           | 8
> +++++---
>  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c  | 8
> +++++---
>  MdePkg/Include/Uefi/UefiSpec.h                                | 8
> +++++---
>  4 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/EdkCompatibilityPkg/Foundation/Framework/Include/EfiCapsule.h
> b/EdkCompatibilityPkg/Foundation/Framework/Include/EfiCapsule.h
> index cae8aec1619e..7026a1876961 100644
> --- a/EdkCompatibilityPkg/Foundation/Framework/Include/EfiCapsule.h
> +++ b/EdkCompatibilityPkg/Foundation/Framework/Include/EfiCapsule.h
> @@ -54,10 +54,12 @@ typedef struct {
>    UINT32    CapsuleImageSize;
>  } EFI_CAPSULE_HEADER;
> 
> +#pragma pack(1)
>  typedef struct {
> -  UINT32   CapsuleArrayNumber;
> -  VOID*    CapsulePtr[1];
> +  UINT32              CapsuleArraySize;
> +  EFI_CAPSULE_HEADER  Capsule[1];
>  } EFI_CAPSULE_TABLE;
> +#pragma pack()
> 
>  //
>  // This struct is deprecated because VendorTable entries physical address will
> not be fixed up when
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/Capsules.c
> b/IntelFrameworkModulePkg/Universal/BdsDxe/Capsules.c
> index 6c7fc7ced4c9..91d21f1d7218 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/Capsules.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/Capsules.c
> @@ -172,11 +172,13 @@ BdsProcessCapsules (
>        }
>      }
>      if (CapsuleNumber != 0) {
> -      Size = sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) *
> sizeof(VOID*);
> +      Size = sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) *
> sizeof(EFI_CAPSULE_HEADER);
>        CapsuleTable = AllocateRuntimePool (Size);
>        ASSERT (CapsuleTable != NULL);
> -      CapsuleTable->CapsuleArrayNumber =  CapsuleNumber;
> -      CopyMem(&CapsuleTable->CapsulePtr[0], CapsulePtrCache,
> CapsuleNumber * sizeof(VOID*));
> +      CapsuleTable->CapsuleArraySize = Size - sizeof(EFI_CAPSULE_TABLE);
> +      for (Index = 0; Index < CapsuleNumber; Index++) {
> +        CopyMem(&CapsuleTable->Capsule[Index], CapsulePtrCache[Index],
> sizeof(EFI_CAPSULE_TABLE));
> +      }
>        Status = gBS->InstallConfigurationTable
> (&CapsuleGuidCache[CacheIndex], (VOID*)CapsuleTable);
>        ASSERT_EFI_ERROR (Status);
>      }
> diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> index ba3ff90b9f73..45a0026acacc 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> @@ -277,14 +277,16 @@ PopulateCapsuleInConfigurationTable (
>        }
>      }
>      if (CapsuleNumber != 0) {
> -      Size = sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) *
> sizeof(VOID*);
> +      Size = sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) *
> sizeof(EFI_CAPSULE_HEADER);
>        CapsuleTable = AllocateRuntimePool (Size);
>        if (CapsuleTable == NULL) {
>          DEBUG ((DEBUG_ERROR, "Allocate CapsuleTable (%g) fail!\n",
> &CapsuleGuidCache[CacheIndex]));
>          continue;
>        }
> -      CapsuleTable->CapsuleArrayNumber =  CapsuleNumber;
> -      CopyMem(&CapsuleTable->CapsulePtr[0], CapsulePtrCache,
> CapsuleNumber * sizeof(VOID*));
> +      CapsuleTable->CapsuleArraySize = Size - sizeof(EFI_CAPSULE_TABLE);
> +      for (Index = 0; Index < CapsuleNumber; Index++) {
> +        CopyMem(&CapsuleTable->Capsule[Index], CapsulePtrCache[Index],
> sizeof(EFI_CAPSULE_TABLE));
> +      }
>        Status = gBS->InstallConfigurationTable
> (&CapsuleGuidCache[CacheIndex], (VOID*)CapsuleTable);
>        if (EFI_ERROR (Status)) {
>          DEBUG ((DEBUG_ERROR, "InstallConfigurationTable (%g) fail!\n",
> &CapsuleGuidCache[CacheIndex]));
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h
> b/MdePkg/Include/Uefi/UefiSpec.h
> index 57cb4e804f70..ad9dfefbccf8 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -1630,16 +1630,18 @@ typedef struct {
>  /// that contain the same CapsuleGuid value. The array must be
>  /// prefixed by a UINT32 that represents the size of the array of capsules.
>  ///
> +#pragma pack(1)
>  typedef struct {
>    ///
>    /// the size of the array of capsules.
>    ///
> -  UINT32   CapsuleArrayNumber;
> +  UINT32   CapsuleArraySize;
>    ///
> -  /// Point to an array of capsules that contain the same CapsuleGuid value.
> +  /// Array of capsules that contain the same CapsuleGuid value.
>    ///
> -  VOID*    CapsulePtr[1];
> +  EFI_CAPSULE_HEADER    Capsule[1];
>  } EFI_CAPSULE_TABLE;
> +#pragma pack()
> 
>  #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET          0x00010000
>  #define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE         0x00020000
> --
> 2.7.4