Thanks for the response. 1) Why we need "enable UEFI variable write before permanent memory is available"?
2) If the implementation is not ready, I do have concern to add it so early in EDKII. If I don’t have a big picture, I am not sure how to review the completeness. Can we put it to EDKII-staging (https://github.com/tianocore/edk2-staging) for a moment? I don’t see the need to add the interface now for work-in-progress feature, since there is no consumer and no producer. 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 Please do consider that as well - how to write a protected variable in PEI phase. Thank you Yao Jiewen > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com> > Sent: Saturday, June 11, 2022 5:49 AM > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; > michael.kuba...@outlook.com > Cc: Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Kinney, Michael D > <michael.d.kin...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; > Chiu, Chasel <chasel.c...@intel.com>; Cheng, Gao <gao.ch...@intel.com>; > Zhang, Di <di.zh...@intel.com>; Bu, Daocheng <daocheng...@intel.com>; > Kubacki, Michael <michael.kuba...@microsoft.com> > 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 before > 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 place > 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 HOB > mechanism to have a later DXE phase perform the update. > 4. With regard to atomicity, we have a complete implementation of the fault > 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 > variable > services re-initialize the mNvVariableCache. > > Hope that helps, > Nate > > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Friday, June 10, 2022 9:56 AM > To: devel@edk2.groups.io; michael.kuba...@outlook.com; Desimone, > Nathaniel L <nathaniel.l.desim...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Kinney, Michael D > <michael.d.kin...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; > Chiu, Chasel <chasel.c...@intel.com>; Cheng, Gao <gao.ch...@intel.com>; > Zhang, Di <di.zh...@intel.com>; Bu, Daocheng <daocheng...@intel.com>; > Kubacki, Michael <michael.kuba...@microsoft.com> > 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 capability > 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 write > 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 <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 > > <nathaniel.l.desim...@intel.com> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Gao, Liming > > <gaolim...@byosoft.com.cn>; Kinney, Michael D > > <michael.d.kin...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; > > Chiu, Chasel <chasel.c...@intel.com>; Cheng, Gao > > <gao.ch...@intel.com>; Zhang, Di <di.zh...@intel.com>; Bu, Daocheng > > <daocheng...@intel.com>; Kubacki, Michael > > <michael.kuba...@microsoft.com> > > 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 <jian.j.w...@intel.com> > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > Cc: Isaac Oram <isaac.w.o...@intel.com> > > > Cc: Chasel Chiu <chasel.c...@intel.com> > > > Cc: Gao Cheng <gao.ch...@intel.com> > > > Cc: Di Zhang <di.zh...@intel.com> > > > Cc: Daocheng Bu <daocheng...@intel.com> > > > Cc: Michael Kubacki <michael.kuba...@microsoft.com> > > > Signed-off-by: Nate DeSimone <nathaniel.l.desim...@intel.com> > > > --- > > > 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.<BR> > > > + 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 the > > 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 is > > > the > > variable's GUID. The combination of > > > + VariableGuid and VariableName must > > > 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 of > > > 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 successfully. > > > + @retval EFI_NOT_FOUND The variable was not found. > > > + @retval EFI_BUFFER_TOO_SMALL The DataSize is too small for the > > resulting data. > > > + DataSize is updated with the size > > > 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 retrieved > > 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 the > > 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 > > > variable 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_GUID > > > that > is > > the variable's GUID. > > > + On return, a pointer to the next > > > variable's GUID. > > > + > > > + @retval EFI_SUCCESS The variable was read successfully. > > > + @retval EFI_NOT_FOUND The variable could not be found. > > > + @retval EFI_BUFFER_TOO_SMALL The VariableNameSize is too small > for > > the resulting > > > + data. VariableNameSize is updated > > > with the size > > > + required for the specified > > > variable. > > > + @retval EFI_INVALID_PARAMETER VariableName, VariableGuid or > > > + VariableNameSize is NULL. > > > + @retval EFI_DEVICE_ERROR The variable could not be retrieved > > 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 the > > EDKII_PEI_VARIABLE_PPI. > > > + @param[in] VariableName A Null-terminated string that is > > > 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 > > > returned. > > > + @param[in] VendorGuid A unique identifier for the vendor. > > > + @param[in] Attributes Attributes bitmask to set for the > > > variable. > > > + @param[in] DataSize The size in bytes of the Data > > > buffer. 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 > > > stored the > > variable and its data as > > > + defined by the Attributes. > > > + @retval EFI_INVALID_PARAMETER An invalid combination of attribute > > bits, name, and GUID was supplied, or the > > > + DataSize exceeds the maximum > > > allowed. > > > + @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 > > > retrieved due > > to a hardware error. > > > + @retval EFI_WRITE_PROTECTED The variable in question is > > > read-only. > > > + @retval EFI_WRITE_PROTECTED The variable in question cannot be > > deleted. > > > + @retval EFI_SECURITY_VIOLATION The variable could not be written > 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 the > > > 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 updated > > > 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 > > > instance of the > > EDKII_PEI_VARIABLE_PPI. > > > + @param[in] Attributes Attributes bitmask to > > > specify the type > > of variables on > > > + which to return > > > information. > > > + @param[out] MaximumVariableStorageSize On output the maximum > > size of the storage space > > > + available for the EFI > > > variables associated with > the > > > + attributes specified. > > > + @param[out] RemainingVariableStorageSize Returns the remaining > size > > of the storage space > > > + available for the EFI > > > variables associated with > the > > > + attributes specified. > > > + @param[out] MaximumVariableSize Returns the maximum > > > size of > > the individual EFI > > > + variables associated > > > with the attributes > specified. > > > + > > > + @retval EFI_SUCCESS Valid answer returned. > > > + @retval EFI_INVALID_PARAMETER An invalid combination > > > of > > attribute bits was supplied > > > + @retval EFI_UNSUPPORTED The attribute is not > > > supported > on > > this platform, and the > > > + > > > MaximumVariableStorageSize, > > > + > > > + 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 = { 0x71a9ea61, 0x5a35, > > > 0x4a5d, > > { 0xac, 0xef, 0x9c, 0xf8, 0x6d, 0x6d, 0x67, 0xe0 } } > > > gEdkiiPeiBootInCapsuleOnDiskModePpiGuid = { 0xb08a11e4, 0xe2b7, > > 0x4b75, { 0xb5, 0x15, 0xaf, 0x61, 0x6, 0x68, 0xbf, 0xd1 } } > > > > > > + ## Include/Ppi/Variable.h > > > + gEdkiiPeiVariablePpiGuid = { 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 > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90454): https://edk2.groups.io/g/devel/message/90454 Mute This Topic: https://groups.io/mt/91659908/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
