On 05/06/19 23:02, Kinney, Michael D wrote: > Laszlo, > > I agree with both your points. The file name should not imply > that it can only be used for capsule resets. > > I also agree that the structure name and GUIDs should use > EDKII_ and gEdkii prefixes. > > I also suggest, since the 2 new GUIDs are not associated with > a structure (they are used to fill in a GUID value field in a > structure), that they can be declared in the DEC file alone and > do not require the MACRO name or extern declaration in a new .h > file. The .h file should only contain the new data structure > EDKII_RESET_DATA_WITH_NULL_STRING. > > Depending on what modules/libs use EDKII_RESET_DATA_WITH_NULL_STRING, > it may make more sense to add this structure to an existing > library class .h file.
The lib class header "MdeModulePkg/Include/Library/ResetUtilityLib.h" definitely crossed my mind, but I didn't dare suggest it. :) Thanks Laszlo > Best regards, > > Mike > >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] >> On Behalf Of Laszlo Ersek >> Sent: Monday, May 6, 2019 8:44 AM >> To: [email protected]; Gao, Zhichao >> <[email protected]> >> Cc: Bret Barkelew <[email protected]>; Wang, >> Jian J <[email protected]>; Wu, Hao A >> <[email protected]>; Ni, Ray <[email protected]>; Zeng, >> Star <[email protected]>; Gao, Liming >> <[email protected]>; Sean Brogan >> <[email protected]>; Michael Turner >> <[email protected]> >> Subject: Re: [edk2-devel] [PATCH 1/4] MdeModulePkg: Add >> reset data difinition and guid >> >> On 05/05/19 09:50, Gao, Zhichao wrote: >>> From: Bret Barkelew <[email protected]> >>> >>> REF: >> https://bugzilla.tianocore.org/show_bug.cgi?id=1772 >>> >>> Add a useful definition of reset data for capsule >> function. And add >>> two guids gCapsuleArmedResetGuid and >> gCapsuleUpdateCompleteResetGuid. >>> >>> Cc: Jian J Wang <[email protected]> >>> Cc: Hao Wu <[email protected]> >>> Cc: Ray Ni <[email protected]> >>> Cc: Star Zeng <[email protected]> >>> Cc: Liming Gao <[email protected]> >>> Cc: Sean Brogan <[email protected]> >>> Cc: Michael Turner <[email protected]> >>> Cc: Bret Barkelew <[email protected]> >>> Signed-off-by: Zhichao Gao <[email protected]> >>> --- >>> MdeModulePkg/Include/Guid/CapsuleResetData.h | 40 >> ++++++++++++++++++++ >>> MdeModulePkg/MdeModulePkg.dec | 5 +++ >>> 2 files changed, 45 insertions(+) >>> create mode 100644 >> MdeModulePkg/Include/Guid/CapsuleResetData.h >>> >>> diff --git >> a/MdeModulePkg/Include/Guid/CapsuleResetData.h >> b/MdeModulePkg/Include/Guid/CapsuleResetData.h >>> new file mode 100644 >>> index 0000000000..b0666a0da2 >>> --- /dev/null >>> +++ b/MdeModulePkg/Include/Guid/CapsuleResetData.h >>> @@ -0,0 +1,40 @@ >>> +/** @file >>> + The capsule reset data difinition and guids. >>> + >>> + Copyright (c) 2019, Intel Corporation. All rights >> reserved.<BR> >>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> +#ifndef __CAPSULE_RESET_DATA_H__ >>> +#define __CAPSULE_RESET_DATA_H__ >>> + >>> +#include <Uefi/UefiBaseType.h> >>> + >>> +// >>> +// reset data started with a null string and followed >> by a guid data >>> +// >>> +#pragma pack(1) >>> +typedef struct { >>> + CHAR16 NullString; >>> + EFI_GUID ResetGuid; >>> +} RESET_DATA_WITH_NULL_STRING; >>> +#pragma pack() >>> + >>> +VERIFY_SIZE_OF (RESET_DATA_WITH_NULL_STRING, 18); >> >> I think that exposing RESET_DATA_WITH_NULL_STRING as a >> public structure >> is a good idea -- it indeed looks like something useful >> for many >> platforms and for different purposes. >> >> - However, I'd argue that it should not go into an >> include file called >> "CapsuleResetData.h". The type looks generic and not >> tied to capsules. >> >> - Additionally, I believe (but I'm not fully sure!) that >> new structures >> under MdeModulePkg/Include/Guid should be named EDKII_*. >> >> If the MdeModulePkg maintainers are OK with these >> patches in the current >> form, I'm fine too; I just thought we might want to >> consider the above >> points. (I certainly suggest waiting for their feedback >> before starting >> work on v2, just to address the above.) >> >> Thanks! >> Laszlo >> >>> + >>> +#define CAPSULE_ARMED_RESET_GUID \ >>> + { \ >>> + 0xc6b4eea7, 0xfce2, 0x4625, { 0x9c, 0x4f, 0xc4, >> 0xb0, 0x82, 0x37, 0xae, 0x23 } \ >>> + } >>> + >>> +#define CAPSULE_UPDATE_COMPLETE_RESET_GUID \ >>> + { \ >>> + 0x5d512714, 0xa4df, 0x4e46, { 0xb6, 0xc7, 0xbc, >> 0x9f, 0x97, 0x9d, 0x59, 0xa0 } \ >>> + } >>> + >>> +extern EFI_GUID gCapsuleArmedResetGuid; >>> + >>> +extern EFI_GUID gCapsuleUpdateCompleteResetGuid; >>> + >>> +#endif >>> + >>> diff --git a/MdeModulePkg/MdeModulePkg.dec >> b/MdeModulePkg/MdeModulePkg.dec >>> index 2ef48f2c37..c12b836c24 100644 >>> --- a/MdeModulePkg/MdeModulePkg.dec >>> +++ b/MdeModulePkg/MdeModulePkg.dec >>> @@ -423,6 +423,11 @@ >>> ## Include/Guid/S3StorageDeviceInitList.h >>> gS3StorageDeviceInitListGuid = { 0x310e9b8c, >> 0xcf90, 0x421e, { 0x8e, 0x9b, 0x9e, 0xef, 0xb6, 0x17, >> 0xc8, 0xef } } >>> >>> + ## Guid to use for gRT->ResetSystem() to indicate >> the type of reset that is being performed. >>> + # Include/Guid/CapsuleResetData.h >>> + gCapsuleArmedResetGuid = {0xc6b4eea7, >> 0xfce2, 0x4625, {0x9c, 0x4f, 0xc4, 0xb0, 0x82, 0x37, >> 0xae, 0x23}} >>> + gCapsuleUpdateCompleteResetGuid = {0x5d512714, >> 0xa4df, 0x4e46, {0xb6, 0xc7, 0xbc, 0x9f, 0x97, 0x9d, >> 0x59, 0xa0}} >>> + >>> [Ppis] >>> ## Include/Ppi/AtaController.h >>> gPeiAtaControllerPpiGuid = { 0xa45e60d1, >> 0xc719, 0x44aa, { 0xb0, 0x7a, 0xaa, 0x77, 0x7f, 0x85, >> 0x90, 0x6d }} >>> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40103): https://edk2.groups.io/g/devel/message/40103 Mute This Topic: https://groups.io/mt/31507358/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
