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

["Laszlo Ersek" <lersek@redhat.com>]

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 }}
>