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

["Gao, Zhichao" <zhichao.gao@intel.com>]

I think ResetUtilityLib.h is a good place to add the new structure definition.
But I think this lib cannot be consumed in some conditions. The new ResetUtilityLib add a new API which would consume a new API ResetSystem in ResetSystemLib.
As we know different platforms have different instances of ResetSystemLib. And I think all of them do not implement the new API yet. So if they consume the ResetUtilityLib, a link error would happen. As an alternative, I define the structure in the ResetSystemLib.h. And do not use the new API ResetSystemWithSubtype in ResetUtilityLib to transfer the ResetData.
Here are some detail conditions:
The platform normally use its own ResetSystemLib. That would lack the new API implement. If it works with the edk2 master and some driver consume the new API. Then a link error would occur.
If the platform add the new API and it works both with edk2 master and some stable tag before, then a name conflict would occur with the stable tag. Because the new API name is conflict with ResetSystemRuntimeDxe and we already change it to RuntimeServiceResetSystem while we add the new API.

All the above is why I want to define the structure in ResetSystemLib.h and not to use the ResetSystemWithSubtype to transfer the ResetData.

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, May 7, 2019 6:15 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
> Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] MdeModulePkg: Add reset data
> difinition and guid
> 
> 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: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Monday, May 6, 2019 8:44 AM
> >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> >> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> >> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> >> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> >> Michael Turner <Michael.Turner@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@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@intel.com>
> >>> Cc: Hao Wu <hao.a.wu@intel.com>
> >>> Cc: Ray Ni <ray.ni@intel.com>
> >>> Cc: Star Zeng <star.zeng@intel.com>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Cc: Sean Brogan <sean.brogan@microsoft.com>
> >>> Cc: Michael Turner <Michael.Turner@microsoft.com>
> >>> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> >>> Signed-off-by: Zhichao Gao <zhichao.gao@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 }}
> >>>
> >>
> >>
> >>
> >
> 
> 
>