Re: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of EFI_FIRMWARE_MANAGEMENT_PROTOCOL

["Xu, GuoX" <guox.xu@intel.com>]

Hi Michael,

Thanks for your suggestions.

1. In edk2 repo there are two implementations for Firmware Management Protocol.
  1.1 In edk2\FmpDevicePkg\FmpDxe\FmpDxe.c, function GetTheImageInfo():
    //
    // Check the buffer size
    // NOTE: Check this first so caller can get the necessary memory size it must allocate.
    //
    if (*ImageInfoSize < (sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR))) {
      *ImageInfoSize = sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR);
      DEBUG ((DEBUG_VERBOSE, "FmpDxe(%s): GetImageInfo() - ImageInfoSize is to small.\n", mImageIdName));
      Status = EFI_BUFFER_TOO_SMALL;
      goto cleanup;
    }

    //
    // Confirm that buffer isn't null
    //
    if (  (ImageInfo == NULL) || (DescriptorVersion == NULL) || (DescriptorCount == NULL) || (DescriptorSize == NULL)
       || (PackageVersion == NULL))
    {
      DEBUG ((DEBUG_ERROR, "FmpDxe(%s): GetImageInfo() - Pointer Parameter is NULL.\n", mImageIdName));
      Status = EFI_INVALID_PARAMETER;
      goto cleanup;
    }
    In the line 497 to 498, it is missed the judgment of PackageVersionName but there is a comment in line 529 " // Do not update PackageVersionName since it is not supported in this instance."
    So, do we need to add the judgment of PackageVersionName in this function?

  1.2 In edk2\SignedCapsulePkg\Universal\SystemFirmwareUpdate\SystemFirmwareCommonDxe.c, function FmpGetImageInfo():
         if (ImageInfoSize == NULL) {
           return EFI_INVALID_PARAMETER;
         }

         if (*ImageInfoSize < sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR) * SystemFmpPrivate->DescriptorCount) {
           *ImageInfoSize = sizeof (EFI_FIRMWARE_IMAGE_DESCRIPTOR) * SystemFmpPrivate->DescriptorCount;
           return EFI_BUFFER_TOO_SMALL;
        }

        if ((ImageInfo == NULL) ||
          (DescriptorVersion == NULL) ||
          (DescriptorCount == NULL) ||
          (DescriptorSize == NULL) ||
          (PackageVersion == NULL) ||
          (PackageVersionName == NULL))
       {
         return EFI_INVALID_PARAMETER;
       }
    The implementation already included the required code changes.

 2. In edk2-platforms repo there is only one implementation for Firmware Management Protocol.
     In the file MicrocodeFmp.c, function FmpGetImageInfo():
     If (ImageInfoSize == NULL) {
       return EFI_INVALID_PARAMETER;
     }
     if (*ImageInfoSize < sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR) * MicrocodeFmpPrivate->DescriptorCount) {
       *ImageInfoSize = sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR) * MicrocodeFmpPrivate->DescriptorCount;
       return EFI_BUFFER_TOO_SMALL;
     }
     if (ImageInfo == NULL ||
        DescriptorVersion == NULL ||
        DescriptorCount == NULL ||
        DescriptorSize == NULL ||
        PackageVersion == NULL ||
        PackageVersionName == NULL) {
      return EFI_INVALID_PARAMETER;
    }
   The implementation already included the required code changes.    

Thanks
Xu Guo

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Tuesday, January 23, 2024 7:08 AM
To: Xu, GuoX <guox.xu@intel.com>; gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of EFI_FIRMWARE_MANAGEMENT_PROTOCOL

Hi Guo,

Thank you for starting this task to update Firmware Management Protocol.

There are additional tasks to make sure all required code changes are also implemented for a specification update like this.

* Create a TianoCore Bugzilla that describes the specification update
  needed with links to the public documents with the required
  specification update. 

* Review/update edk2 repo include files.

* Review/update function headers of the implementations of the
  Firmware Management Protocol in the edk2 repo.

* Review/update logic in implementations of the Firmware
  Management Protocol in the edk2 repo to make sure all error
  return conditions are checked.

* Review/update function headers of the implementations of the
  Firmware Management Protocol in the edk2-platforms repo.

* Review/update logic in implementations of the Firmware
  Management Protocol in the edk2-platforms repo to make sure
  all error return conditions are checked.

* Review/update test cases edk2-test repo for the UEFI SCTs for
  these error return conditions.


The "REF" in the commit message should be a link to the TianoCore Bugzilla.  The reference to the specification is also required, but should be in the text of the commit message.

Thanks,

Mike


> -----Original Message-----
> From: Xu, GuoX <guox.xu@intel.com>
> Sent: Sunday, January 21, 2024 9:37 PM
> To: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang 
> <zhiguang.liu@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of 
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> 
> Hi Liming,
>   I created a PR: https://github.com/tianocore/edk2/pull/5182, could 
> you help push it ?
> 
> Thanks
> Xu Guo
> 
> -----Original Message-----
> From: gaoliming <gaoliming@byosoft.com.cn>
> Sent: Tuesday, January 16, 2024 10:20 PM
> To: devel@edk2.groups.io; Xu, GuoX <guox.xu@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang 
> <zhiguang.liu@intel.com>
> Subject: 回复: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of 
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> 
> I am OK for this change. Reviewed-by: Liming Gao 
> <gaoliming@byosoft.com.cn>
> 
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Xu, GuoX
> > 发送时间: 2024年1月9日 17:24
> > 收件人: devel@edk2.groups.io
> > 抄送: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
> > 主题: Re: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of 
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > Hi all, any comments about this patch?
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of GuoX 
> > Xu
> > Sent: Monday, December 25, 2023 9:21 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> > <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; 
> > Li, Yi1 <yi1.li@intel.com>
> > Subject: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of 
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > 1.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImage():
> > Add the following sentence at the end of the Image parameter
> description.
> > "May be NULL with a zero ImageSize in order to determine the size of 
> > the buffer needed".
> >
> > Modify the description of "EFI_INVALID_PARAMETER" return code as 
> > "The ImageSize is not too small and Image is NULL."
> >
> > 2.For EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo():
> > Add the following sentence at the end of the ImageInfo parameter 
> > description."May be NULL with a zero ImageInfoSize in order to 
> > determine the size of the buffer needed".
> >
> > Modify the description of "EFI_INVALID_PARAMETER" return code as 
> > "The ImageInfoSize is not too small and Image is NULL." and add new 
> > descriptions for "EFI_INVALID_PARAMETER" return code.
> >
> > REF: UEFI Spec 2.7B Chapter 23.1.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > Cc: Yi Li <yi1.li@intel.com>
> > Signed-off-by: GuoX Xu <guox.xu@intel.com>
> > ---
> >  MdePkg/Include/Protocol/FirmwareManagement.h | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdePkg/Include/Protocol/FirmwareManagement.h
> > b/MdePkg/Include/Protocol/FirmwareManagement.h
> > index f37067df3455..93c8b7658e1a 100644
> > --- a/MdePkg/Include/Protocol/FirmwareManagement.h
> > +++ b/MdePkg/Include/Protocol/FirmwareManagement.h
> > @@ -293,6 +293,8 @@ EFI_STATUS
> >                                       to contain the image(s) 
> > information if the buffer was too small.
> >    @param[in, out] ImageInfo          A pointer to the buffer in which
> > firmware places the current image(s)
> >                                       information. The information 
> > is an array of EFI_FIRMWARE_IMAGE_DESCRIPTORs.
> > +                                     May be NULL with a zero
> > ImageInfoSize in order to determine the size of the
> > +                                     buffer needed.
> >    @param[out]     DescriptorVersion  A pointer to the location in
> which
> > firmware returns the version number
> >                                       associated with the 
> > EFI_FIRMWARE_IMAGE_DESCRIPTOR.
> >    @param[out]     DescriptorCount    A pointer to the location in
> > which firmware returns the number of @@ -313,7 +315,12 @@ EFI_STATUS
> >    @retval EFI_SUCCESS                The device was successfully
> > updated with the new image.
> >    @retval EFI_BUFFER_TOO_SMALL       The ImageInfo buffer was too
> > small. The current buffer size
> >                                       needed to hold the image(s) 
> > information is returned in ImageInfoSize.
> > -  @retval EFI_INVALID_PARAMETER      ImageInfoSize is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is not too small
> > and ImageInfo is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > DescriptorVersion is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > DescriptorCount is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > DescriptorSize is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > PackageVersion is NULL.
> > +  @retval EFI_INVALID_PARAMETER      ImageInfoSize is non-zero and
> > PackageVersionName is NULL.
> >    @retval EFI_DEVICE_ERROR           Valid information could not be
> > returned. Possible corrupted image.
> >
> >  **/
> > @@ -340,6 +347,8 @@ EFI_STATUS
> >    @param[in]      ImageIndex     A unique number identifying the
> > firmware image(s) within the device.
> >                                   The number is between 1 and 
> > DescriptorCount.
> >    @param[out]     Image          Points to the buffer where the
> > current image is copied to.
> > +                                 May be NULL with a zero
> > ImageSize in order to determine the size of the
> > +                                 buffer needed.
> >    @param[in, out] ImageSize      On entry, points to the size of the
> > buffer pointed to by Image, in bytes.
> >                                   On return, points to the length of 
> > the image, in bytes.
> >
> > @@ -347,7 +356,7 @@ EFI_STATUS
> >    @retval EFI_BUFFER_TOO_SMALL   The buffer specified by ImageSize is
> > too small to hold the
> >                                   image. The current buffer size 
> > needed to hold the image is returned
> >                                   in ImageSize.
> > -  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> > +  @retval EFI_INVALID_PARAMETER  The ImageSize is not too small and
> > Image is NULL.
> >    @retval EFI_NOT_FOUND          The current image is not copied to
> > the buffer.
> >    @retval EFI_UNSUPPORTED        The operation is not supported.
> >    @retval EFI_SECURITY_VIOLATION The operation could not be 
> > performed due to an authentication failure.
> > --
> > 2.29.2.windows.3
> >
> >
> >
> >
> >
> >
> >
> >
> > 
> >
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114798): https://edk2.groups.io/g/devel/message/114798
Mute This Topic: https://groups.io/mt/103898765/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-