From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DEAF782214 for ; Fri, 3 Mar 2017 05:24:17 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Mar 2017 05:24:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,237,1484035200"; d="scan'208";a="55482580" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga002.jf.intel.com with ESMTP; 03 Mar 2017 05:24:17 -0800 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 3 Mar 2017 05:24:17 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 3 Mar 2017 05:24:16 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Fri, 3 Mar 2017 21:24:14 +0800 From: "Yao, Jiewen" To: Ard Biesheuvel , "edk2-devel@lists.01.org" CC: "Gao, Liming" , "leif.lindholm@linaro.org" , "Kinney, Michael D" Thread-Topic: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct Thread-Index: AQHSlBgd0lRVeJqyM0iRwRRBCpSv+qGDGDHw Date: Fri, 3 Mar 2017 13:24:14 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503A8F901F@shsmsx102.ccr.corp.intel.com> References: <1488543408-27921-1-git-send-email-ard.biesheuvel@linaro.org> In-Reply-To: <1488543408-27921-1-git-send-email-ard.biesheuvel@linaro.org> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Mar 2017 13:24:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 head= ers" 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 > Cc: Gao, Liming ; leif.lindholm@linaro.org; Kinney, > Michael D ; Ard Biesheuvel > > Subject: [PATCH] MdePkg/Uefi: pack EFI_CAPSULE_TABLE struct >=20 > The UEFI specification describes the per-capsule type configuration > table entry as follows: >=20 > 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 arra= y > of capsules. >=20 > In the current EDK2 implementation, this is translated into the following >=20 > typedef struct { > /// > /// the size of the array of capsules. > /// > UINT32 CapsuleArrayNumber; > /// > /// Point to an array of capsules that contain the same CapsuleGuid v= alue. > /// > VOID* CapsulePtr[1]; > } EFI_CAPSULE_TABLE; >=20 > 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. >=20 > 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 numbe= r > of entries, > - pack the struct to remove any padding on 64-bit architectures > - replace the array of pointers with an array of capsule headers. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > 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(-) >=20 > 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; >=20 > +#pragma pack(1) > typedef struct { > - UINT32 CapsuleArrayNumber; > - VOID* CapsulePtr[1]; > + UINT32 CapsuleArraySize; > + EFI_CAPSULE_HEADER Capsule[1]; > } EFI_CAPSULE_TABLE; > +#pragma pack() >=20 > // > // This struct is deprecated because VendorTable entries physical addres= s 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 !=3D 0) { > - Size =3D sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) * > sizeof(VOID*); > + Size =3D sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) * > sizeof(EFI_CAPSULE_HEADER); > CapsuleTable =3D AllocateRuntimePool (Size); > ASSERT (CapsuleTable !=3D NULL); > - CapsuleTable->CapsuleArrayNumber =3D CapsuleNumber; > - CopyMem(&CapsuleTable->CapsulePtr[0], CapsulePtrCache, > CapsuleNumber * sizeof(VOID*)); > + CapsuleTable->CapsuleArraySize =3D Size - sizeof(EFI_CAPSULE_TABLE= ); > + for (Index =3D 0; Index < CapsuleNumber; Index++) { > + CopyMem(&CapsuleTable->Capsule[Index], CapsulePtrCache[Index], > sizeof(EFI_CAPSULE_TABLE)); > + } > Status =3D 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 !=3D 0) { > - Size =3D sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) * > sizeof(VOID*); > + Size =3D sizeof(EFI_CAPSULE_TABLE) + (CapsuleNumber - 1) * > sizeof(EFI_CAPSULE_HEADER); > CapsuleTable =3D AllocateRuntimePool (Size); > if (CapsuleTable =3D=3D NULL) { > DEBUG ((DEBUG_ERROR, "Allocate CapsuleTable (%g) fail!\n", > &CapsuleGuidCache[CacheIndex])); > continue; > } > - CapsuleTable->CapsuleArrayNumber =3D CapsuleNumber; > - CopyMem(&CapsuleTable->CapsulePtr[0], CapsulePtrCache, > CapsuleNumber * sizeof(VOID*)); > + CapsuleTable->CapsuleArraySize =3D Size - sizeof(EFI_CAPSULE_TABLE= ); > + for (Index =3D 0; Index < CapsuleNumber; Index++) { > + CopyMem(&CapsuleTable->Capsule[Index], CapsulePtrCache[Index], > sizeof(EFI_CAPSULE_TABLE)); > + } > Status =3D 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 capsul= es. > /// > +#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 va= lue. > + /// Array of capsules that contain the same CapsuleGuid value. > /// > - VOID* CapsulePtr[1]; > + EFI_CAPSULE_HEADER Capsule[1]; > } EFI_CAPSULE_TABLE; > +#pragma pack() >=20 > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > #define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE 0x00020000 > -- > 2.7.4