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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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
> >
> >
> > 
> >