From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 07 May 2019 03:14:38 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4547959473; Tue, 7 May 2019 10:14:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-152.rdu2.redhat.com [10.10.120.152]) by smtp.corp.redhat.com (Postfix) with ESMTP id 45575611D9; Tue, 7 May 2019 10:14:34 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/4] MdeModulePkg: Add reset data difinition and guid To: "Kinney, Michael D" , "devel@edk2.groups.io" , "Gao, Zhichao" Cc: Bret Barkelew , "Wang, Jian J" , "Wu, Hao A" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , Michael Turner References: <20190505075046.14464-1-zhichao.gao@intel.com> <20190505075046.14464-2-zhichao.gao@intel.com> <4dd2122c-1d05-c8a3-4adc-949e87b8dc20@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 7 May 2019 12:14:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 07 May 2019 10:14:37 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> >> Cc: Bret Barkelew ; Wang, >> Jian J ; Wu, Hao A >> ; Ni, Ray ; Zeng, >> Star ; Gao, Liming >> ; Sean Brogan >> ; Michael Turner >> >> 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 >>> >>> 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 >>> Cc: Hao Wu >>> Cc: Ray Ni >>> Cc: Star Zeng >>> Cc: Liming Gao >>> Cc: Sean Brogan >>> Cc: Michael Turner >>> Cc: Bret Barkelew >>> Signed-off-by: Zhichao Gao >>> --- >>> 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.
>>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>> + >>> +**/ >>> +#ifndef __CAPSULE_RESET_DATA_H__ >>> +#define __CAPSULE_RESET_DATA_H__ >>> + >>> +#include >>> + >>> +// >>> +// 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 }} >>> >> >> >> >