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. 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 (#40054): https://edk2.groups.io/g/devel/message/40054 Mute This Topic: https://groups.io/mt/31507358/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
