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.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 (#90448): https://edk2.groups.io/g/devel/message/90448
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]
-=-=-=-=-=-=-=-=-=-=-=-
