1. Patchew
  2. EDK2
  3. View series

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

Ard Biesheuvel posted 1 patch 7 years, 1 month ago
Failed in applying to current master (apply log)
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(-)
[edk2] [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
Posted by Ard Biesheuvel 7 years, 1 month ago 
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
Re: [edk2] [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
Posted by Yao, Jiewen 7 years, 1 month ago 
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
Re: [edk2] [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
Posted by Ard Biesheuvel 7 years, 1 month ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.