Re: [RFC 0/4] Add FmpDevicePkg

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

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