[edk2-devel] [PATCH 1/4] MdeModulePkg: Add reset data difinition and guid

Mon May 6 21:02:45 UTC 2019

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: devel at edk2.groups.io [mailto:devel at edk2.groups.io]
> On Behalf Of Laszlo Ersek
> Sent: Monday, May 6, 2019 8:44 AM
> To: devel at edk2.groups.io; Gao, Zhichao
> <zhichao.gao at intel.com>
> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>; Wang,
> Jian J <jian.j.wang at intel.com>; Wu, Hao A
> <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Zeng,
> Star <star.zeng at intel.com>; Gao, Liming
> <liming.gao at intel.com>; Sean Brogan
> <sean.brogan at microsoft.com>; Michael Turner
> <Michael.Turner at microsoft.com>
> 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 <Bret.Barkelew at microsoft.com>
> >
> > 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 <jian.j.wang at intel.com>
> > Cc: Hao Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Star Zeng <star.zeng at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Sean Brogan <sean.brogan at microsoft.com>
> > Cc: Michael Turner <Michael.Turner at microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao at intel.com>
> > ---
> >  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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-