* [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
@ 2017-03-03 12:16 Ard Biesheuvel
2017-03-03 13:24 ` Yao, Jiewen
0 siblings, 1 reply; 3+ messages in thread
From: Ard Biesheuvel @ 2017-03-03 12:16 UTC (permalink / raw)
To: edk2-devel, jiewen.yao
Cc: liming.gao, leif.lindholm, michael.d.kinney, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
2017-03-03 12:16 [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct Ard Biesheuvel
@ 2017-03-03 13:24 ` Yao, Jiewen
2017-03-03 13:45 ` Ard Biesheuvel
0 siblings, 1 reply; 3+ messages in thread
From: Yao, Jiewen @ 2017-03-03 13:24 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel@lists.01.org
Cc: Gao, Liming, leif.lindholm@linaro.org, Kinney, Michael D
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct
2017-03-03 13:24 ` Yao, Jiewen
@ 2017-03-03 13:45 ` Ard Biesheuvel
0 siblings, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2017-03-03 13:45 UTC (permalink / raw)
To: Yao, Jiewen
Cc: edk2-devel@lists.01.org, Gao, Liming, leif.lindholm@linaro.org,
Kinney, Michael D
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-03 13:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-03 12:16 [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct Ard Biesheuvel
2017-03-03 13:24 ` Yao, Jiewen
2017-03-03 13:45 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox