Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg: Add Definition of EDKII_PEI_VARIABLE_PPI

["Michael Kubacki" <michael.kubacki@outlook.com>]

I just saw the comment in the cover letter about the additional 
implementation.

Thanks,
Michael

On 6/9/2022 9:59 PM, Michael Kubacki wrote:
> 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.wang@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>> Cc: Chasel Chiu <chasel.chiu@intel.com>
>> Cc: Gao Cheng <gao.cheng@intel.com>
>> Cc: Di Zhang <di.zhang@intel.com>
>> Cc: Daocheng Bu <daocheng.bu@intel.com>
>> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
>> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@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