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

["Michael Kubacki" <mikuback@linux.microsoft.com>]

The file looks okay to me as an interface proposal but I agree with 
Jiewen about deferring the edk2 submission until an implementation can 
be reviewed with it as well.

Regards,
Michael


On 6/10/2022 9:09 PM, Yao, Jiewen wrote:
> 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.desimone@intel.com>
>> Sent: Saturday, June 11, 2022 5:49 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
>> michael.kubacki@outlook.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
>>
>> 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.yao@intel.com>
>> Sent: Friday, June 10, 2022 9:56 AM
>> To: devel@edk2.groups.io; michael.kubacki@outlook.com; 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
>>
>> 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.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
>>>
>>>
>>>
>>>
> 
> 
> 
> 
> 
>