[v1] Add FmpDevicePkg

[edk2] [RFC 0/4] Add FmpDevicePkg

Michael D Kinney posted 4 patches 6 years, 7 months ago

Diff against v2 v3 v5 v6
Download series mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20180404212834.8644-1-michael.d.kinney@intel.com

There is a newer version of this series

FmpDevicePkg/FmpDevicePkg.dec                      |  126 ++
FmpDevicePkg/FmpDevicePkg.dsc                      |  119 ++
FmpDevicePkg/FmpDevicePkg.uni                      |   75 +
FmpDevicePkg/FmpDevicePkgExtra.uni                 |   18 +
FmpDevicePkg/FmpDxe/FmpDxe.c                       | 1533 ++++++++++++++++++++
FmpDevicePkg/FmpDxe/FmpDxe.inf                     |   96 ++
FmpDevicePkg/FmpDxe/FmpDxe.uni                     |   20 +
FmpDevicePkg/FmpDxe/FmpDxeExtra.uni                |   18 +
FmpDevicePkg/FmpDxe/FmpDxeLib.inf                  |   93 ++
FmpDevicePkg/FmpDxe/VariableSupport.c              |  431 ++++++
FmpDevicePkg/FmpDxe/VariableSupport.h              |  180 +++
.../Include/Library/CapsuleUpdatePolicyLib.h       |  120 ++
FmpDevicePkg/Include/Library/FmpDeviceLib.h        |  385 +++++
FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h |  100 ++
.../CapsuleUpdatePolicyLibNull.c                   |  136 ++
.../CapsuleUpdatePolicyLibNull.inf                 |   45 +
.../CapsuleUpdatePolicyLibNull.uni                 |   17 +
.../Library/FmpDeviceLibNull/FmpDeviceLib.c        |  395 +++++
.../Library/FmpDeviceLibNull/FmpDeviceLibNull.inf  |   48 +
.../Library/FmpDeviceLibNull/FmpDeviceLibNull.uni  |   18 +
.../FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c    |  188 +++
.../FmpPayloadHeaderLibV1.inf                      |   48 +
.../FmpPayloadHeaderLibV1.uni                      |   21 +
23 files changed, 4230 insertions(+)
create mode 100644 FmpDevicePkg/FmpDevicePkg.dec
create mode 100644 FmpDevicePkg/FmpDevicePkg.dsc
create mode 100644 FmpDevicePkg/FmpDevicePkg.uni
create mode 100644 FmpDevicePkg/FmpDevicePkgExtra.uni
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.c
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.inf
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.uni
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeExtra.uni
create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeLib.inf
create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.c
create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.h
create mode 100644 FmpDevicePkg/Include/Library/CapsuleUpdatePolicyLib.h
create mode 100644 FmpDevicePkg/Include/Library/FmpDeviceLib.h
create mode 100644 FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h
create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.c
create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.inf
create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.uni
create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.inf
create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.uni
create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c
create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.inf
create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.uni

Expand all Fold all

[edk2] [RFC 0/4] Add FmpDevicePkg

Posted by Michael D Kinney 6 years, 7 months ago

https://bugzilla.tianocore.org/show_bug.cgi?id=922

Based on content from the following branch:

https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport/MsCapsuleUpdatePkg

Branch for review:

https://github.com/mdkinney/edk2/tree/Bug_922_FmpDevicePkg

This package provides an implementation of a Firmware Management Protocol
instance that supports the update of firmware storage devices using UEFI
Capsules.  The behavior of the Firmware Management Protocol instance is
customized using libraries and PCDs.

Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>

Kinney, Michael D (4):
  FmpDevicePkg: Add package, library classes, and PCDs
  FmpDevicePkg: Add library instances
  FmpDevicePkg: Add FmpDxe module
  FmpDevicePkg: Add DSC file to build all package components

 FmpDevicePkg/FmpDevicePkg.dec                      |  126 ++
 FmpDevicePkg/FmpDevicePkg.dsc                      |  119 ++
 FmpDevicePkg/FmpDevicePkg.uni                      |   75 +
 FmpDevicePkg/FmpDevicePkgExtra.uni                 |   18 +
 FmpDevicePkg/FmpDxe/FmpDxe.c                       | 1533 ++++++++++++++++++++
 FmpDevicePkg/FmpDxe/FmpDxe.inf                     |   96 ++
 FmpDevicePkg/FmpDxe/FmpDxe.uni                     |   20 +
 FmpDevicePkg/FmpDxe/FmpDxeExtra.uni                |   18 +
 FmpDevicePkg/FmpDxe/FmpDxeLib.inf                  |   93 ++
 FmpDevicePkg/FmpDxe/VariableSupport.c              |  431 ++++++
 FmpDevicePkg/FmpDxe/VariableSupport.h              |  180 +++
 .../Include/Library/CapsuleUpdatePolicyLib.h       |  120 ++
 FmpDevicePkg/Include/Library/FmpDeviceLib.h        |  385 +++++
 FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h |  100 ++
 .../CapsuleUpdatePolicyLibNull.c                   |  136 ++
 .../CapsuleUpdatePolicyLibNull.inf                 |   45 +
 .../CapsuleUpdatePolicyLibNull.uni                 |   17 +
 .../Library/FmpDeviceLibNull/FmpDeviceLib.c        |  395 +++++
 .../Library/FmpDeviceLibNull/FmpDeviceLibNull.inf  |   48 +
 .../Library/FmpDeviceLibNull/FmpDeviceLibNull.uni  |   18 +
 .../FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c    |  188 +++
 .../FmpPayloadHeaderLibV1.inf                      |   48 +
 .../FmpPayloadHeaderLibV1.uni                      |   21 +
 23 files changed, 4230 insertions(+)
 create mode 100644 FmpDevicePkg/FmpDevicePkg.dec
 create mode 100644 FmpDevicePkg/FmpDevicePkg.dsc
 create mode 100644 FmpDevicePkg/FmpDevicePkg.uni
 create mode 100644 FmpDevicePkg/FmpDevicePkgExtra.uni
 create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.c
 create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.inf
 create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.uni
 create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeExtra.uni
 create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeLib.inf
 create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.c
 create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.h
 create mode 100644 FmpDevicePkg/Include/Library/CapsuleUpdatePolicyLib.h
 create mode 100644 FmpDevicePkg/Include/Library/FmpDeviceLib.h
 create mode 100644 FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h
 create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.c
 create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.inf
 create mode 100644 FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull.uni
 create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
 create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.inf
 create mode 100644 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.uni
 create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c
 create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.inf
 create mode 100644 FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.uni

-- 
2.14.2.windows.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC 0/4] Add FmpDevicePkg

Posted by Yao, Jiewen 6 years, 6 months ago

Hi Mike
It is great to see this feature. Thanks for doing this.

Some thought below:
1) Do we must add V1 as suffix for the FmpPayloadHeaderLib?
I think we can just use that FmpPayloadHeaderLib.
If we have second version, we can use FmpPayloadHeaderLibV2.

2) The check in GetFmpPayloadHeaderSize() seems weird.

  if ((UINTN)FmpPayloadHeader + sizeof (FMP_PAYLOAD_HEADER) < (UINTN)FmpPayloadHeader ||

If we just want to add overflow check, I suggest we use below.
  if ((UINTN)FmpPayloadHeader > (MAX_ADDRESS - sizeof (FMP_PAYLOAD_HEADER)) ||

I think it is more readable.

3) For FmpDeviceLib, the interface definition seems not consistent.

Most of them return EFI_STATUS, such as:
EFI_STATUS
EFIAPI
FmpDeviceGetLowestSupportedVersion (
  OUT UINT32  *LowestSupportedVersion
  )

But some return data directly, such as:
CHAR16 *
EFIAPI
FmpDeviceGetVersionString (
  VOID
  )

Can we make them consistent to return EFI_STATUS for all? Such as
EFI_STATUS
EFIAPI
FmpDeviceGetVersionString (
  OUT CHAR16 **VersionString
  )

4) Do we need GetPackageVersion() and GetPackageVersionName() ?

5) I found FmpDxe driver returns unsupported for GetPackageInfo() and SetPackageInfo().
It means that an implementation cannot use FmpDxe driver as template, if it wants to support those 2.

Is there any design limitation ?

6) for variable set, I do not think we need get and compare.
I think variable driver already did that for us. The caller can just set the variable.

VOID
SetVersionInVariable (
   UINT32  Version
  )
{
  EFI_STATUS  Status;
  UINT32      Current;

  Status = EFI_SUCCESS;

  Current = GetVersionFromVariable();
  if (Current != Version) {
    Status = gRT->SetVariable (
                    VARNAME_VERSION,
                    &gEfiCallerIdGuid,
                    EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS,
                    sizeof (Version),
                    &Version
                    );

7) for LockAllVars(), I found it only happens in non flash_update boot mode.
I think we should also do in flash_update boot mode, after the capsule is processed.

Or there will be a hole, for the non-reset capsule.

  //
  // If the boot mode isn't flash update then we must lock all vars.
  // If it is flash update leave them unlocked as the system will not
  // boot all the way in flash update mode
  //
  if (BOOT_ON_FLASH_UPDATE != GetBootModeHob ()) {
    LockAllVars ();
  }

8) I am surprised that we support "*" as variable name. Will it lock all variables with gEfiCallerIdGuid?

Status = Protocol->RequestToLock (Protocol, L"*", &gEfiCallerIdGuid);


Thank you
Yao Jiewen


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, April 5, 2018 5:29 AM
> To: edk2-devel@lists.01.org
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [RFC 0/4] Add FmpDevicePkg
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=922
> 
> Based on content from the following branch:
> 
> https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport/MsCapsu
> leUpdatePkg
> 
> Branch for review:
> 
> https://github.com/mdkinney/edk2/tree/Bug_922_FmpDevicePkg
> 
> This package provides an implementation of a Firmware Management Protocol
> instance that supports the update of firmware storage devices using UEFI
> Capsules.  The behavior of the Firmware Management Protocol instance is
> customized using libraries and PCDs.
> 
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Kinney, Michael D (4):
>   FmpDevicePkg: Add package, library classes, and PCDs
>   FmpDevicePkg: Add library instances
>   FmpDevicePkg: Add FmpDxe module
>   FmpDevicePkg: Add DSC file to build all package components
> 
>  FmpDevicePkg/FmpDevicePkg.dec                      |  126 ++
>  FmpDevicePkg/FmpDevicePkg.dsc                      |  119 ++
>  FmpDevicePkg/FmpDevicePkg.uni                      |   75 +
>  FmpDevicePkg/FmpDevicePkgExtra.uni                 |   18 +
>  FmpDevicePkg/FmpDxe/FmpDxe.c                       | 1533
> ++++++++++++++++++++
>  FmpDevicePkg/FmpDxe/FmpDxe.inf                     |   96 ++
>  FmpDevicePkg/FmpDxe/FmpDxe.uni                     |   20 +
>  FmpDevicePkg/FmpDxe/FmpDxeExtra.uni                |   18 +
>  FmpDevicePkg/FmpDxe/FmpDxeLib.inf                  |   93 ++
>  FmpDevicePkg/FmpDxe/VariableSupport.c              |  431 ++++++
>  FmpDevicePkg/FmpDxe/VariableSupport.h              |  180 +++
>  .../Include/Library/CapsuleUpdatePolicyLib.h       |  120 ++
>  FmpDevicePkg/Include/Library/FmpDeviceLib.h        |  385 +++++
>  FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h |  100 ++
>  .../CapsuleUpdatePolicyLibNull.c                   |  136 ++
>  .../CapsuleUpdatePolicyLibNull.inf                 |   45 +
>  .../CapsuleUpdatePolicyLibNull.uni                 |   17 +
>  .../Library/FmpDeviceLibNull/FmpDeviceLib.c        |  395 +++++
>  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.inf  |   48 +
>  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.uni  |   18 +
>  .../FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c    |  188 +++
>  .../FmpPayloadHeaderLibV1.inf                      |   48 +
>  .../FmpPayloadHeaderLibV1.uni                      |   21 +
>  23 files changed, 4230 insertions(+)
>  create mode 100644 FmpDevicePkg/FmpDevicePkg.dec
>  create mode 100644 FmpDevicePkg/FmpDevicePkg.dsc
>  create mode 100644 FmpDevicePkg/FmpDevicePkg.uni
>  create mode 100644 FmpDevicePkg/FmpDevicePkgExtra.uni
>  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.c
>  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.inf
>  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.uni
>  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeExtra.uni
>  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeLib.inf
>  create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.c
>  create mode 100644 FmpDevicePkg/FmpDxe/VariableSupport.h
>  create mode 100644 FmpDevicePkg/Include/Library/CapsuleUpdatePolicyLib.h
>  create mode 100644 FmpDevicePkg/Include/Library/FmpDeviceLib.h
>  create mode 100644 FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h
>  create mode 100644
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull
> .c
>  create mode 100644
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull
> .inf
>  create mode 100644
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNull
> .uni
>  create mode 100644
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
>  create mode 100644
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.inf
>  create mode 100644
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.uni
>  create mode 100644
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c
>  create mode 100644
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.inf
>  create mode 100644
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHeaderLibV1.uni
> 
> --
> 2.14.2.windows.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC 0/4] Add FmpDevicePkg

Posted by Kinney, Michael D 6 years, 6 months ago

Hi Jiewen,

Responses below.  

A couple require some comments from Sean or Bret.

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, April 4, 2018 4:29 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-devel@lists.01.org
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Subject: RE: [RFC 0/4] Add FmpDevicePkg
> 
> Hi Mike
> It is great to see this feature. Thanks for doing this.
> 
> Some thought below:
> 1) Do we must add V1 as suffix for the
> FmpPayloadHeaderLib?
> I think we can just use that FmpPayloadHeaderLib.
> If we have second version, we can use
> FmpPayloadHeaderLibV2.
> 

It is not a must, but since this library class is designed
to manage a versioned header structure with extensibility
in mind, I do not see anything wrong with V1 in the name.

In many cases the exiting use of a 2 suffix in the EDK II
is because the original one was not intending multiple
versions but an issue was discovered later that required
a new version. 

> 2) The check in GetFmpPayloadHeaderSize() seems weird.
> 
>   if ((UINTN)FmpPayloadHeader + sizeof
> (FMP_PAYLOAD_HEADER) < (UINTN)FmpPayloadHeader ||
> 
> If we just want to add overflow check, I suggest we use
> below.
>   if ((UINTN)FmpPayloadHeader > (MAX_ADDRESS - sizeof
> (FMP_PAYLOAD_HEADER)) ||
> 
> I think it is more readable.
> 

Maybe we should use SafeIntLib service instead?


> 3) For FmpDeviceLib, the interface definition seems not
> consistent.
> 
> Most of them return EFI_STATUS, such as:
> EFI_STATUS
> EFIAPI
> FmpDeviceGetLowestSupportedVersion (
>   OUT UINT32  *LowestSupportedVersion
>   )
> 
> But some return data directly, such as:
> CHAR16 *
> EFIAPI
> FmpDeviceGetVersionString (
>   VOID
>   )
> 
> Can we make them consistent to return EFI_STATUS for
> all? Such as
> EFI_STATUS
> EFIAPI
> FmpDeviceGetVersionString (
>   OUT CHAR16 **VersionString
>   )
> 

I agree they are inconsistent.  These APIs are the same
as the ones from MS_UEFI GitHub branch and I did not change
the API design.  It appears that some of the APIs in 
the FmpDeviceLib are optimized for how they are used by the
FmpDxe component.

> 4) Do we need GetPackageVersion() and
> GetPackageVersionName() ?

No.

> 
> 5) I found FmpDxe driver returns unsupported for
> GetPackageInfo() and SetPackageInfo().
> It means that an implementation cannot use FmpDxe
> driver as template, if it wants to support those 2.
> 
> Is there any design limitation ?

Yes.  This simplified Firmware Management Protocol
Implementation does not support Get/Set PackageInfo().
The UEFI Specification allows these APIs to return
EFI_UNSUPPORTED, and for this firmware update use
case they are not required.  If a Firmware Management
Protocol implementation requires Get/Set PackageInfo()
then FmpDxe should not be used.

> 
> 6) for variable set, I do not think we need get and
> compare.
> I think variable driver already did that for us. The
> caller can just set the variable.
> 
> VOID
> SetVersionInVariable (
>    UINT32  Version
>   )
> {
>   EFI_STATUS  Status;
>   UINT32      Current;
> 
>   Status = EFI_SUCCESS;
> 
>   Current = GetVersionFromVariable();
>   if (Current != Version) {
>     Status = gRT->SetVariable (
>                     VARNAME_VERSION,
>                     &gEfiCallerIdGuid,
>                     EFI_VARIABLE_NON_VOLATILE |
> EFI_VARIABLE_BOOTSERVICE_ACCESS,
>                     sizeof (Version),
>                     &Version
>                     );
> 

Does the UEFI Specification require the compare on set?
If not, then is there any harm in doing this check? 


> 7) for LockAllVars(), I found it only happens in non
> flash_update boot mode.
> I think we should also do in flash_update boot mode,
> after the capsule is processed.
> 
> Or there will be a hole, for the non-reset capsule.
> 
>   //
>   // If the boot mode isn't flash update then we must
> lock all vars.
>   // If it is flash update leave them unlocked as the
> system will not
>   // boot all the way in flash update mode
>   //
>   if (BOOT_ON_FLASH_UPDATE != GetBootModeHob ()) {
>     LockAllVars ();
>   }
> 

That is a good point.  Maybe Sean had another use case in mind?

> 8) I am surprised that we support "*" as variable name.
> Will it lock all variables with gEfiCallerIdGuid?
> 
> Status = Protocol->RequestToLock (Protocol, L"*",
> &gEfiCallerIdGuid);
> 

I was equally surprised.  I thought it may be a special argument
to the EDK II specific lock protocol.  Maybe it is a feature
that was not included in the MS_UEFI branch.

> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Thursday, April 5, 2018 5:29 AM
> > To: edk2-devel@lists.01.org
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Yao,
> Jiewen
> > <jiewen.yao@intel.com>
> > Subject: [RFC 0/4] Add FmpDevicePkg
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=922
> >
> > Based on content from the following branch:
> >
> >
> https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsu
> leSupport/MsCapsu
> > leUpdatePkg
> >
> > Branch for review:
> >
> >
> https://github.com/mdkinney/edk2/tree/Bug_922_FmpDevice
> Pkg
> >
> > This package provides an implementation of a Firmware
> Management Protocol
> > instance that supports the update of firmware storage
> devices using UEFI
> > Capsules.  The behavior of the Firmware Management
> Protocol instance is
> > customized using libraries and PCDs.
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> >
> > Kinney, Michael D (4):
> >   FmpDevicePkg: Add package, library classes, and
> PCDs
> >   FmpDevicePkg: Add library instances
> >   FmpDevicePkg: Add FmpDxe module
> >   FmpDevicePkg: Add DSC file to build all package
> components
> >
> >  FmpDevicePkg/FmpDevicePkg.dec                      |
> 126 ++
> >  FmpDevicePkg/FmpDevicePkg.dsc                      |
> 119 ++
> >  FmpDevicePkg/FmpDevicePkg.uni                      |
> 75 +
> >  FmpDevicePkg/FmpDevicePkgExtra.uni                 |
> 18 +
> >  FmpDevicePkg/FmpDxe/FmpDxe.c                       |
> 1533
> > ++++++++++++++++++++
> >  FmpDevicePkg/FmpDxe/FmpDxe.inf                     |
> 96 ++
> >  FmpDevicePkg/FmpDxe/FmpDxe.uni                     |
> 20 +
> >  FmpDevicePkg/FmpDxe/FmpDxeExtra.uni                |
> 18 +
> >  FmpDevicePkg/FmpDxe/FmpDxeLib.inf                  |
> 93 ++
> >  FmpDevicePkg/FmpDxe/VariableSupport.c              |
> 431 ++++++
> >  FmpDevicePkg/FmpDxe/VariableSupport.h              |
> 180 +++
> >  .../Include/Library/CapsuleUpdatePolicyLib.h       |
> 120 ++
> >  FmpDevicePkg/Include/Library/FmpDeviceLib.h        |
> 385 +++++
> >  FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h |
> 100 ++
> >  .../CapsuleUpdatePolicyLibNull.c                   |
> 136 ++
> >  .../CapsuleUpdatePolicyLibNull.inf                 |
> 45 +
> >  .../CapsuleUpdatePolicyLibNull.uni                 |
> 17 +
> >  .../Library/FmpDeviceLibNull/FmpDeviceLib.c        |
> 395 +++++
> >  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.inf  |
> 48 +
> >  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.uni  |
> 18 +
> >  .../FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c    |
> 188 +++
> >  .../FmpPayloadHeaderLibV1.inf                      |
> 48 +
> >  .../FmpPayloadHeaderLibV1.uni                      |
> 21 +
> >  23 files changed, 4230 insertions(+)
> >  create mode 100644 FmpDevicePkg/FmpDevicePkg.dec
> >  create mode 100644 FmpDevicePkg/FmpDevicePkg.dsc
> >  create mode 100644 FmpDevicePkg/FmpDevicePkg.uni
> >  create mode 100644
> FmpDevicePkg/FmpDevicePkgExtra.uni
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.c
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.inf
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.uni
> >  create mode 100644
> FmpDevicePkg/FmpDxe/FmpDxeExtra.uni
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeLib.inf
> >  create mode 100644
> FmpDevicePkg/FmpDxe/VariableSupport.c
> >  create mode 100644
> FmpDevicePkg/FmpDxe/VariableSupport.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/CapsuleUpdatePolicyLib.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .c
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .uni
> >  create mode 100644
> > FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.
> inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.
> uni
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLib.c
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLibV1.inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLibV1.uni
> >
> > --
> > 2.14.2.windows.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC 0/4] Add FmpDevicePkg

Posted by Zeng, Star 6 years, 6 months ago

Curious about which tool to generate the FMP_PAYLOAD_HEADER?
BaseTools needs to be updated?


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
Sent: Thursday, April 5, 2018 8:29 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [RFC 0/4] Add FmpDevicePkg

Hi Jiewen,

Responses below.  

A couple require some comments from Sean or Bret.

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, April 4, 2018 4:29 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; 
> edk2-devel@lists.01.org
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Subject: RE: [RFC 0/4] Add FmpDevicePkg
> 
> Hi Mike
> It is great to see this feature. Thanks for doing this.
> 
> Some thought below:
> 1) Do we must add V1 as suffix for the FmpPayloadHeaderLib?
> I think we can just use that FmpPayloadHeaderLib.
> If we have second version, we can use
> FmpPayloadHeaderLibV2.
> 

It is not a must, but since this library class is designed to manage a versioned header structure with extensibility in mind, I do not see anything wrong with V1 in the name.

In many cases the exiting use of a 2 suffix in the EDK II is because the original one was not intending multiple versions but an issue was discovered later that required a new version. 

> 2) The check in GetFmpPayloadHeaderSize() seems weird.
> 
>   if ((UINTN)FmpPayloadHeader + sizeof
> (FMP_PAYLOAD_HEADER) < (UINTN)FmpPayloadHeader ||
> 
> If we just want to add overflow check, I suggest we use below.
>   if ((UINTN)FmpPayloadHeader > (MAX_ADDRESS - sizeof
> (FMP_PAYLOAD_HEADER)) ||
> 
> I think it is more readable.
> 

Maybe we should use SafeIntLib service instead?


> 3) For FmpDeviceLib, the interface definition seems not consistent.
> 
> Most of them return EFI_STATUS, such as:
> EFI_STATUS
> EFIAPI
> FmpDeviceGetLowestSupportedVersion (
>   OUT UINT32  *LowestSupportedVersion
>   )
> 
> But some return data directly, such as:
> CHAR16 *
> EFIAPI
> FmpDeviceGetVersionString (
>   VOID
>   )
> 
> Can we make them consistent to return EFI_STATUS for all? Such as 
> EFI_STATUS EFIAPI FmpDeviceGetVersionString (
>   OUT CHAR16 **VersionString
>   )
> 

I agree they are inconsistent.  These APIs are the same as the ones from MS_UEFI GitHub branch and I did not change the API design.  It appears that some of the APIs in the FmpDeviceLib are optimized for how they are used by the FmpDxe component.

> 4) Do we need GetPackageVersion() and
> GetPackageVersionName() ?

No.

> 
> 5) I found FmpDxe driver returns unsupported for
> GetPackageInfo() and SetPackageInfo().
> It means that an implementation cannot use FmpDxe driver as template, 
> if it wants to support those 2.
> 
> Is there any design limitation ?

Yes.  This simplified Firmware Management Protocol Implementation does not support Get/Set PackageInfo().
The UEFI Specification allows these APIs to return EFI_UNSUPPORTED, and for this firmware update use case they are not required.  If a Firmware Management Protocol implementation requires Get/Set PackageInfo() then FmpDxe should not be used.

> 
> 6) for variable set, I do not think we need get and compare.
> I think variable driver already did that for us. The caller can just 
> set the variable.
> 
> VOID
> SetVersionInVariable (
>    UINT32  Version
>   )
> {
>   EFI_STATUS  Status;
>   UINT32      Current;
> 
>   Status = EFI_SUCCESS;
> 
>   Current = GetVersionFromVariable();
>   if (Current != Version) {
>     Status = gRT->SetVariable (
>                     VARNAME_VERSION,
>                     &gEfiCallerIdGuid,
>                     EFI_VARIABLE_NON_VOLATILE | 
> EFI_VARIABLE_BOOTSERVICE_ACCESS,
>                     sizeof (Version),
>                     &Version
>                     );
> 

Does the UEFI Specification require the compare on set?
If not, then is there any harm in doing this check? 


> 7) for LockAllVars(), I found it only happens in non flash_update boot 
> mode.
> I think we should also do in flash_update boot mode, after the capsule 
> is processed.
> 
> Or there will be a hole, for the non-reset capsule.
> 
>   //
>   // If the boot mode isn't flash update then we must lock all vars.
>   // If it is flash update leave them unlocked as the system will not
>   // boot all the way in flash update mode
>   //
>   if (BOOT_ON_FLASH_UPDATE != GetBootModeHob ()) {
>     LockAllVars ();
>   }
> 

That is a good point.  Maybe Sean had another use case in mind?

> 8) I am surprised that we support "*" as variable name.
> Will it lock all variables with gEfiCallerIdGuid?
> 
> Status = Protocol->RequestToLock (Protocol, L"*", &gEfiCallerIdGuid);
> 

I was equally surprised.  I thought it may be a special argument to the EDK II specific lock protocol.  Maybe it is a feature that was not included in the MS_UEFI branch.

> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Thursday, April 5, 2018 5:29 AM
> > To: edk2-devel@lists.01.org
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Yao,
> Jiewen
> > <jiewen.yao@intel.com>
> > Subject: [RFC 0/4] Add FmpDevicePkg
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=922
> >
> > Based on content from the following branch:
> >
> >
> https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsu
> leSupport/MsCapsu
> > leUpdatePkg
> >
> > Branch for review:
> >
> >
> https://github.com/mdkinney/edk2/tree/Bug_922_FmpDevice
> Pkg
> >
> > This package provides an implementation of a Firmware
> Management Protocol
> > instance that supports the update of firmware storage
> devices using UEFI
> > Capsules.  The behavior of the Firmware Management
> Protocol instance is
> > customized using libraries and PCDs.
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> >
> > Kinney, Michael D (4):
> >   FmpDevicePkg: Add package, library classes, and
> PCDs
> >   FmpDevicePkg: Add library instances
> >   FmpDevicePkg: Add FmpDxe module
> >   FmpDevicePkg: Add DSC file to build all package
> components
> >
> >  FmpDevicePkg/FmpDevicePkg.dec                      |
> 126 ++
> >  FmpDevicePkg/FmpDevicePkg.dsc                      |
> 119 ++
> >  FmpDevicePkg/FmpDevicePkg.uni                      |
> 75 +
> >  FmpDevicePkg/FmpDevicePkgExtra.uni                 |
> 18 +
> >  FmpDevicePkg/FmpDxe/FmpDxe.c                       |
> 1533
> > ++++++++++++++++++++
> >  FmpDevicePkg/FmpDxe/FmpDxe.inf                     |
> 96 ++
> >  FmpDevicePkg/FmpDxe/FmpDxe.uni                     |
> 20 +
> >  FmpDevicePkg/FmpDxe/FmpDxeExtra.uni                |
> 18 +
> >  FmpDevicePkg/FmpDxe/FmpDxeLib.inf                  |
> 93 ++
> >  FmpDevicePkg/FmpDxe/VariableSupport.c              |
> 431 ++++++
> >  FmpDevicePkg/FmpDxe/VariableSupport.h              |
> 180 +++
> >  .../Include/Library/CapsuleUpdatePolicyLib.h       |
> 120 ++
> >  FmpDevicePkg/Include/Library/FmpDeviceLib.h        |
> 385 +++++
> >  FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h |
> 100 ++
> >  .../CapsuleUpdatePolicyLibNull.c                   |
> 136 ++
> >  .../CapsuleUpdatePolicyLibNull.inf                 |
> 45 +
> >  .../CapsuleUpdatePolicyLibNull.uni                 |
> 17 +
> >  .../Library/FmpDeviceLibNull/FmpDeviceLib.c        |
> 395 +++++
> >  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.inf  |
> 48 +
> >  .../Library/FmpDeviceLibNull/FmpDeviceLibNull.uni  |
> 18 +
> >  .../FmpPayloadHeaderLibV1/FmpPayloadHeaderLib.c    |
> 188 +++
> >  .../FmpPayloadHeaderLibV1.inf                      |
> 48 +
> >  .../FmpPayloadHeaderLibV1.uni                      |
> 21 +
> >  23 files changed, 4230 insertions(+)  create mode 100644 
> > FmpDevicePkg/FmpDevicePkg.dec  create mode 100644 
> > FmpDevicePkg/FmpDevicePkg.dsc  create mode 100644 
> > FmpDevicePkg/FmpDevicePkg.uni  create mode 100644
> FmpDevicePkg/FmpDevicePkgExtra.uni
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxe.c  create mode 100644 
> > FmpDevicePkg/FmpDxe/FmpDxe.inf  create mode 100644 
> > FmpDevicePkg/FmpDxe/FmpDxe.uni  create mode 100644
> FmpDevicePkg/FmpDxe/FmpDxeExtra.uni
> >  create mode 100644 FmpDevicePkg/FmpDxe/FmpDxeLib.inf  create mode 
> > 100644
> FmpDevicePkg/FmpDxe/VariableSupport.c
> >  create mode 100644
> FmpDevicePkg/FmpDxe/VariableSupport.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/CapsuleUpdatePolicyLib.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >  create mode 100644
> FmpDevicePkg/Include/Library/FmpPayloadHeaderLib.h
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .c
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/Capsule
> UpdatePolicyLibNull
> > .uni
> >  create mode 100644
> > FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.
> inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLibNull.
> uni
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLib.c
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLibV1.inf
> >  create mode 100644
> >
> FmpDevicePkg/Library/FmpPayloadHeaderLibV1/FmpPayloadHe
> aderLibV1.uni
> >
> > --
> > 2.14.2.windows.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel