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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Michael,

Thanks for the great feedback! I believe I have addressed all of it in the V2 patch. Please take a look when you get a chance.

Per your comment on the EFI_AUTHENTICATED_VARIABLE_HOB... this is one of those old data structures migrated from the edk1 that has the EFI_ prefix even though it was never added to the PI spec proper. The name wasn't changed for backwards compatibility reasons (there is a huge amount of code dependent on those old names.) There are a fair number of instances of this in the variable services, EFI_SMM_VARIABLE_PROTOCOL perhaps being the most visible.

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Thursday, June 9, 2022 7:00 PM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D <michael.d.kinney@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Cheng, Gao <gao.cheng@intel.com>; Zhang, Di <di.zhang@intel.com>; Bu, Daocheng <daocheng.bu@intel.com>; Kubacki, Michael <michael.kubacki@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.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