Re: [RFC 0/4] Add FmpDevicePkg

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

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