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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Guo,

I also see Madhan has been working on this same issue.

https://edk2.groups.io/g/devel/topic/103620853#114085

Can you please coordinate your patches with Madhan so it is a single patch series.

Also, my feedback to enter a BZ and update REF: with the link to the BZ has 
not been done yet.  Please do that for the next update.

Thanks,

Mike


> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, January 30, 2024 10:18 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,
> 
> One comment below.
> 
> Mike
> 
> 
> > -----Original Message-----
> > From: Xu, GuoX <guox.xu@intel.com>
> > Sent: Monday, January 29, 2024 11:18 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; gaoliming
> > <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> > Cc: Liu, Zhiguang <zhiguang.liu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] MdePkg:Updated the comments of
> > EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > 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?
> 
> Yes.  The check needs to be added for PackageVersionName being NULL for
> this protocol service to be conformant with the UEFI Specification.
> 
> The fact that this specific implementation does not update
> PackageVersionName
> contents does not change what a conformance test that checks the
> behavior
> of a NULL parameter value would conclude that the service does not
> follow
> the spec.
> 
> >
> >   1.2 In
> >
> edk2\SignedCapsulePkg\Universal\SystemFirmwareUpdate\SystemFirmwareCommo
> > nDxe.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 (#114825): https://edk2.groups.io/g/devel/message/114825
Mute This Topic: https://groups.io/mt/103898765/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-