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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.