diff mbox

[edk2] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct

Message ID 1488543408-27921-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel March 3, 2017, 12:16 p.m. UTC 
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Yao, Jiewen March 3, 2017, 1:24 p.m. UTC | #1 
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


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 3, 2017, 1:45 p.m. UTC | #2 
On 3 March 2017 at 13:24, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 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.

>


Yes, please.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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