From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web12.1374.1654910078715827289 for ; Fri, 10 Jun 2022 18:14:38 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=ql4itUiE; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.195.228.134]) by linux.microsoft.com (Postfix) with ESMTPSA id D97AF20BE6B4; Fri, 10 Jun 2022 18:14:36 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D97AF20BE6B4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1654910078; bh=tA2oXXsot/a+77J0mLsDhJ16WrdVwXHUx0NIgk9S8J0=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ql4itUiEWvPMpB+H3HO2F+U5dTAL3LX3m6rjEXzTVJ5W3AAeufoNparzM2mI3bVLx 6tpbO2KyIpNFSkLtAbbF7SPcjVXDXNWHak0+2kyfEAViwiLUilcPjOJYaN/M4Hftt/ qHkM28LomL6ny8Y6y+uvCdRrNvimFnLJEWMC8CeM= Message-ID: <182e73f1-c5d4-c35a-7d03-a02e33cdd8ef@linux.microsoft.com> Date: Fri, 10 Jun 2022 21:14:35 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg: Add Definition of EDKII_PEI_VARIABLE_PPI To: devel@edk2.groups.io, jiewen.yao@intel.com, "Desimone, Nathaniel L" Cc: "Wang, Jian J" , "Gao, Liming" , "Kinney, Michael D" , "Oram, Isaac W" , "Chiu, Chasel" , "Cheng, Gao" , "Zhang, Di" , "Bu, Daocheng" , "mikuback@linux.microsoft.com" References: <20220610011705.5148-1-nathaniel.l.desimone@intel.com> <20220610011705.5148-2-nathaniel.l.desimone@intel.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable The file looks okay to me as an interface proposal but I agree with=20 Jiewen about deferring the edk2 submission until an implementation can=20 be reviewed with it as well. Regards, Michael On 6/10/2022 9:09 PM, Yao, Jiewen wrote: > Thanks for the response. >=20 > 1) Why we need "enable UEFI variable write before permanent memory is ava= ilable"? >=20 > 2) If the implementation is not ready, I do have concern to add it so ear= ly in EDKII. > If I don=E2=80=99t have a big picture, I am not sure how to review the co= mpleteness. >=20 > Can we put it to EDKII-staging (https://github.com/tianocore/edk2-staging= ) for a moment? > I don=E2=80=99t see the need to add the interface now for work-in-progres= s feature, since there is no consumer and no producer. >=20 > Another reason is that I happen to know other feature (in EDKII stage) is= impacting variable driver. > https://github.com/tianocore/edk2-staging/tree/ProtectedVariable/libs >=20 > Please do consider that as well - how to write a protected variable in PE= I phase. >=20 > Thank you > Yao Jiewen >=20 >> -----Original Message----- >> From: Desimone, Nathaniel L >> Sent: Saturday, June 11, 2022 5:49 AM >> To: Yao, Jiewen ; devel@edk2.groups.io; >> michael.kubacki@outlook.com >> Cc: Wang, Jian J ; Gao, Liming >> ; Kinney, Michael D >> ; Oram, Isaac W ; >> Chiu, Chasel ; Cheng, Gao ; >> Zhang, Di ; Bu, Daocheng ; >> Kubacki, Michael >> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg: Add Definition of >> EDKII_PEI_VARIABLE_PPI >> >> Hi Jiewen, >> >> Thanks for the feedback, per your questions: >> >> 1. The primary use case for this is to enable UEFI variable writes befor= e >> permanent memory is available. >> 2. The implementation is a work in progress. We will provide it shortly.= As this >> will be a rather large patch set, I would like to get this piece in plac= e beforehand >> so that the reviewers can focus on the implementation separate from the = API >> definition. >> 3. No impact to secure boot. We are not going to support writing to >> authenticated variables in PEI. As mentioned in the comments, if a PEIM = wishes >> to update any of the authenticated variables it must use the existing HO= B >> mechanism to have a later DXE phase perform the update. >> 4. With regard to atomicity, we have a complete implementation of the fa= ult >> tolerant write services operational in Pre-Memory PEI. >> 5. Good point on the S3 resume, we will need to add an SMI to have the v= ariable >> services re-initialize the mNvVariableCache. >> >> Hope that helps, >> Nate >> >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Friday, June 10, 2022 9:56 AM >> To: devel@edk2.groups.io; michael.kubacki@outlook.com; Desimone, >> Nathaniel L >> Cc: Wang, Jian J ; Gao, Liming >> ; Kinney, Michael D >> ; Oram, Isaac W ; >> Chiu, Chasel ; Cheng, Gao ; >> Zhang, Di ; Bu, Daocheng ; >> Kubacki, Michael >> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg: Add Definition of >> EDKII_PEI_VARIABLE_PPI >> >> Hi >> >> I am curious why we need this interface. Why we need write variable capa= bility >> in PEI phase? >> >> Where is the implementation of this? I prefer to see an implementation >> submitted together with header file. >> For example, what is the impact to secure boot related feature, how to w= rite >> auth variable in PEI, how PEI write variable cowork with SMM version in = S3 >> resume phase, how to support variable atomicity, etc. >> >> Thank you >> Yao Jiewen >> >> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of Michael >>> Kubacki >>> Sent: Friday, June 10, 2022 10:00 AM >>> To: devel@edk2.groups.io; Desimone, Nathaniel L >>> >>> Cc: Wang, Jian J ; Gao, Liming >>> ; Kinney, Michael D >>> ; Oram, Isaac W ; >>> Chiu, Chasel ; Cheng, Gao >>> ; Zhang, Di ; Bu, Daocheng >>> ; Kubacki, Michael >>> >>> Subject: Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg: Add Definition >>> of EDKII_PEI_VARIABLE_PPI >>> >>> Is this change just adding the interface to Tianocore or is there >>> additional implementation planned as well? >>> >>> --- >>> >>> I thought we were following this convention now: >>> >>> "#ifndef __PEI_VARIABLE_PPI_H_" -> "#ifndef PEI_VARIABLE_PPI_H_" >>> >>> Some other comments are inline. >>> >>> Regards, >>> Michael >>> >>> On 6/9/2022 9:17 PM, Nate DeSimone wrote: >>>> Adds definition of EDKII_PEI_VARIABLE_PPI, a pre-cursor to enabling >>>> variable writes in the PEI environment. >>>> >>>> Cc: Jian J Wang >>>> Cc: Liming Gao >>>> Cc: Michael D Kinney >>>> Cc: Isaac Oram >>>> Cc: Chasel Chiu >>>> Cc: Gao Cheng >>>> Cc: Di Zhang >>>> Cc: Daocheng Bu >>>> Cc: Michael Kubacki >>>> Signed-off-by: Nate DeSimone >>>> --- >>>> MdeModulePkg/Include/Ppi/Variable.h | 189 >>> ++++++++++++++++++++++++++++ >>>> MdeModulePkg/MdeModulePkg.dec | 3 + >>>> 2 files changed, 192 insertions(+) >>>> create mode 100644 MdeModulePkg/Include/Ppi/Variable.h >>>> >>>> diff --git a/MdeModulePkg/Include/Ppi/Variable.h >>> b/MdeModulePkg/Include/Ppi/Variable.h >>>> new file mode 100644 >>>> index 0000000000..97dc7ceefa >>>> --- /dev/null >>>> +++ b/MdeModulePkg/Include/Ppi/Variable.h >>>> @@ -0,0 +1,189 @@ >>>> +/** @file >>>> + EDKII PEI Variable Protocol provides an implementation of >>>> +variables >>> >>> [MK] Was "EDKII PEI Variable PPI" intended? >>> >>>> + intended for use as a means to store data in the PEI environment. >>>> + >>>> + Copyright (c) 2022, Intel Corporation. All rights reserved.
>>>> + SPDX-License-Identifier: BSD-2-Clause-Patent >>>> + >>>> +**/ >>>> + >>>> +#ifndef __PEI_VARIABLE_PPI_H_ >>>> +#define __PEI_VARIABLE_PPI_H_ >>>> + >>>> +#define EDKII_PEI_VARIABLE_PPI_GUID \ >>>> + { \ >>>> + 0xe7b2cd04, 0x4b14, 0x44c2, { 0xb7, 0x48, 0xce, 0xaf, 0x2b, >>>> +0x66, 0x4a, >>> 0xb0 } \ >>>> + } >>>> + >>>> +typedef struct _EDKII_PEI_VARIABLE_PPI EDKII_PEI_VARIABLE_PPI; >>>> + >>>> +/** >>>> + This service retrieves a variable's value using its name and GUID. >>>> + >>>> + Read the specified variable from the UEFI variable store. If the >>>> + Data buffer is too small to hold the contents of the variable, >>>> + the error EFI_BUFFER_TOO_SMALL is returned and DataSize is set to >>>> + the required buffer size to obtain the data. >>>> + >>>> + @param[in] This A pointer to this instance of t= he >>> EDKII_PEI_VARIABLE_PPI. >>>> + @param[in] VariableName A pointer to a null-terminated = string that >>> is the variable's name. >>>> + @param[in] VariableGuid A pointer to an EFI_GUID that i= s the >>> variable's GUID. The combination of >>>> + VariableGuid and VariableName m= ust be unique. >>>> + @param[out] Attributes If non-NULL, on return, points = to the >>> variable's attributes. >>>> + @param[in, out] DataSize On entry, points to the size in= bytes of the >>> Data buffer. >>>> + On return, points to the size o= f the data returned in >> Data. >>>> + @param[out] Data Points to the buffer which will= hold the >>> returned variable value. >>>> + May be NULL with a zero >>>> + DataSize in order to determine >>> the size of the >>>> + buffer needed. >>>> + >>>> + @retval EFI_SUCCESS The variable was read successfu= lly. >>>> + @retval EFI_NOT_FOUND The variable was not found. >>>> + @retval EFI_BUFFER_TOO_SMALL The DataSize is too small for t= he >>> resulting data. >>>> + DataSize is updated with the si= ze required for >>>> + the specified variable. >>>> + @retval EFI_INVALID_PARAMETER VariableName, VariableGuid, >>> DataSize or Data is NULL. >>>> + @retval EFI_DEVICE_ERROR The variable could not be retri= eved >>> because of a device error. >>>> + >>>> +**/ >>>> +typedef >>>> +EFI_STATUS >>>> +(EFIAPI *EDKII_PEI_GET_VARIABLE)( >>>> + IN CONST EDKII_PEI_VARIABLE_PPI *This, >>>> + IN CONST CHAR16 *VariableName, >>>> + IN CONST EFI_GUID *VariableGuid, >>>> + OUT UINT32 *Attributes, >>> >>> [MK] Based on the description, Attributes should be marked "OPTIONAL". >>> >>>> + IN OUT UINTN *DataSize, >>>> + OUT VOID *Data OPTIONAL >>>> + ); >>>> + >>>> +/** >>>> + Return the next variable name and GUID. >>>> + >>>> + This function is called multiple times to retrieve the >>>> + VariableName and VariableGuid of all variables currently available = in the >> system. >>>> + On each call, the previous results are passed into the interface, >>>> + and, on return, the interface returns the data for the next >>>> + interface. When the entire variable list has been returned, >>>> + EFI_NOT_FOUND is returned. >>>> + >>> >>> [MK] I know other descriptions don't usually have it but it would be >>> nice to describe the initial calling values expected. >>> >>>> + @param[in] This A pointer to this instance of t= he >>> EDKII_PEI_VARIABLE_PPI. >>>> + @param[in, out] VariableNameSize On entry, points to the size of= the >>> buffer pointed to by VariableName. >>>> + On return, the size of the vari= able name buffer. >>>> + @param[in, out] VariableName On entry, a pointer to a null- >> terminated >>> string that is the variable's name. >>>> + On return, points to the next >>>> + variable's null-terminated >>> name string. >>>> + @param[in, out] VariableGuid On entry, a pointer to an EFI_G= UID that >> is >>> the variable's GUID. >>>> + On return, a pointer to the nex= t variable's GUID. >>>> + >>>> + @retval EFI_SUCCESS The variable was read successfu= lly. >>>> + @retval EFI_NOT_FOUND The variable could not be found= . >>>> + @retval EFI_BUFFER_TOO_SMALL The VariableNameSize is too sma= ll >> for >>> the resulting >>>> + data. VariableNameSize is updat= ed with the size >>>> + required for the specified vari= able. >>>> + @retval EFI_INVALID_PARAMETER VariableName, VariableGuid or >>>> + VariableNameSize is NULL. >>>> + @retval EFI_DEVICE_ERROR The variable could not be retri= eved >>> because of a device error. >>>> + >>>> +**/ >>>> +typedef >>>> +EFI_STATUS >>>> +(EFIAPI *EDKII_PEI_GET_NEXT_VARIABLE_NAME)( >>>> + IN CONST EDKII_PEI_VARIABLE_PPI *This, >>>> + IN OUT UINTN *VariableNameSize, >>>> + IN OUT CHAR16 *VariableName, >>>> + IN OUT EFI_GUID *VariableGuid >>>> + ); >>>> + >>>> +/** >>>> + Sets the value of a variable. >>>> + >>>> + @param[in] This A pointer to this instance of t= he >>> EDKII_PEI_VARIABLE_PPI. >>>> + @param[in] VariableName A Null-terminated string that i= s the name >>> of the vendor's variable. >>>> + Each VariableName is unique for= each VendorGuid. >>> VariableName must >>>> + contain 1 or more characters. >>>> + If VariableName is an >>> empty string, >>>> + then EFI_INVALID_PARAMETER is r= eturned. >>>> + @param[in] VendorGuid A unique identifier for the ven= dor. >>>> + @param[in] Attributes Attributes bitmask to set for t= he variable. >>>> + @param[in] DataSize The size in bytes of the Data b= uffer. Unless >>> the EFI_VARIABLE_APPEND_WRITE >>>> + attribute is set, a size of >>>> + zero causes the variable to be >>> deleted. When the >>>> + EFI_VARIABLE_APPEND_WRITE >>>> + attribute is set, then a >>> SetVariable() call with a >>>> + DataSize of zero will not >>>> + cause any change to the >>> variable value. >>>> + @param[in] Data The contents for the variable. >>>> + >>>> + @retval EFI_SUCCESS The firmware has successfully s= tored the >>> variable and its data as >>>> + defined by the Attributes. >>>> + @retval EFI_INVALID_PARAMETER An invalid combination of attri= bute >>> bits, name, and GUID was supplied, or the >>>> + DataSize exceeds the maximum al= lowed. >>>> + @retval EFI_INVALID_PARAMETER VariableName is an empty string= . >>>> + @retval EFI_OUT_OF_RESOURCES Not enough storage is available= to >>> hold the variable and its data. >>>> + @retval EFI_DEVICE_ERROR The variable could not be retri= eved due >>> to a hardware error. >>>> + @retval EFI_WRITE_PROTECTED The variable in question is rea= d-only. >>>> + @retval EFI_WRITE_PROTECTED The variable in question cannot= be >>> deleted. >>>> + @retval EFI_SECURITY_VIOLATION The variable could not be writt= en >> due >>> to EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS, >>>> + or >>> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, or >>>> + >>>> + EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS >>> being set. Writing to authenticated >>>> + variables is not supported in t= he PEI environment. >>> Updates to authenticated >>>> + variables can be requested >>>> + during PEI via the >>> EFI_AUTHENTICATED_VARIABLE_HOB, but >>>> + these updates won't be >>>> + written to non-volatile storage >>> until later in DXE. See >>>> + >>>> + MdeModulePkg/Include/Guid/VariableFormat.h for >>> more details on >>>> + EFI_AUTHENTICATED_VARIABLE_HOB. >>> >>> [MK] I didn't see "EFI_AUTHENTICATED_VARIABLE_HOB" mentioned in >>> VariableFormat.h. >>> >>> [MK] It seems that if a contract for producing and then consuming this >>> HOB is going to be defined between the HOB producer and consumer >>> phase, it should be described in something like the PI Spec. >>> >>>> + @retval EFI_NOT_FOUND The variable trying to be updat= ed or >>> deleted was not found. >>>> + >>>> +**/ >>>> +typedef >>>> +EFI_STATUS >>>> +(EFIAPI *EDKII_PEI_SET_VARIABLE)( >>>> + IN CONST EDKII_PEI_VARIABLE_PPI *This, >>>> + IN CHAR16 *VariableName, >>>> + IN EFI_GUID *VendorGuid, >>>> + IN UINT32 Attributes, >>>> + IN UINTN DataSize, >>>> + IN VOID *Data >>>> + ); >>>> + >>>> +/** >>>> + Returns information about the UEFI variables. >>>> + >>>> + @param[in] This A pointer to this i= nstance of the >>> EDKII_PEI_VARIABLE_PPI. >>>> + @param[in] Attributes Attributes bitmask = to specify the type >>> of variables on >>>> + which to return inf= ormation. >>>> + @param[out] MaximumVariableStorageSize On output the maxim= um >>> size of the storage space >>>> + available for the E= FI variables associated with >> the >>>> + attributes specifie= d. >>>> + @param[out] RemainingVariableStorageSize Returns the remaini= ng >> size >>> of the storage space >>>> + available for the E= FI variables associated with >> the >>>> + attributes specifie= d. >>>> + @param[out] MaximumVariableSize Returns the maximum= size of >>> the individual EFI >>>> + variables associate= d with the attributes >> specified. >>>> + >>>> + @retval EFI_SUCCESS Valid answer return= ed. >>>> + @retval EFI_INVALID_PARAMETER An invalid combinat= ion of >>> attribute bits was supplied >>>> + @retval EFI_UNSUPPORTED The attribute is no= t supported >> on >>> this platform, and the >>>> + MaximumVariableStor= ageSize, >>>> + >>>> + RemainingVariableStorageSize, >>> MaximumVariableSize >>>> + are undefined. >>>> + >>>> +**/ >>>> +typedef >>>> +EFI_STATUS >>>> +(EFIAPI *EDKII_PEI_QUERY_VARIABLE_INFO)( >>>> + IN CONST EDKII_PEI_VARIABLE_PPI *This, >>>> + IN UINT32 Attributes, >>>> + OUT UINT64 *MaximumVariableStorageSize, >>>> + OUT UINT64 *RemainingVariableStorageSize, >>>> + OUT UINT64 *MaximumVariableSize >>>> + ); >>>> + >>>> +/// >>>> +/// PEI Variable Protocol is intended for use as a means >>> >>> [MK] Was "PEI Variable PPI" intended? >>> >>>> +/// to store data in the PEI environment. >>>> +/// >>>> +struct _EDKII_PEI_VARIABLE_PPI { >>>> + EDKII_PEI_GET_VARIABLE GetVariable; >>>> + EDKII_PEI_GET_NEXT_VARIABLE_NAME GetNextVariableName; >>>> + EDKII_PEI_SET_VARIABLE SetVariable; >>>> + EDKII_PEI_QUERY_VARIABLE_INFO QueryVariableInfo; >>>> +}; >>>> + >>>> +extern EFI_GUID gEdkiiPeiVariablePpiGuid; >>>> + >>>> +#endif >>>> diff --git a/MdeModulePkg/MdeModulePkg.dec >>> b/MdeModulePkg/MdeModulePkg.dec >>>> index 2bcb9f9453..4f4c48b81f 100644 >>>> --- a/MdeModulePkg/MdeModulePkg.dec >>>> +++ b/MdeModulePkg/MdeModulePkg.dec >>>> @@ -513,6 +513,9 @@ >>>> gEdkiiPeiCapsuleOnDiskPpiGuid =3D { 0x71a9ea61, 0x5a3= 5, 0x4a5d, >>> { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } } >>>> gEdkiiPeiBootInCapsuleOnDiskModePpiGuid =3D { 0xb08a11e4, 0xe2b= 7, >>> 0x4b75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } } >>>> >>>> + ## Include/Ppi/Variable.h >>>> + gEdkiiPeiVariablePpiGuid =3D { 0xe7b2cd04, 0x4b14,= 0x44c2, { 0xb7, >>> 0x48, 0xce, 0xaf, 0x2b, 0x66, 0x4a, 0xb0 } } >>>> + >>>> [Protocols] >>>> ## Load File protocol provides capability to load and unload EFI >>>> image into >>> memory and execute it. >>>> # Include/Protocol/LoadPe32Image.h >>> >>> >>> >>> >=20 >=20 >=20 >=20 >=20 >=20