From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9567022526490 for ; Wed, 4 Apr 2018 16:29:25 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Apr 2018 16:29:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,408,1517904000"; d="scan'208";a="31116648" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga008.jf.intel.com with ESMTP; 04 Apr 2018 16:29:24 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Apr 2018 16:29:24 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 4 Apr 2018 16:29:23 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.61]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.241]) with mapi id 14.03.0319.002; Thu, 5 Apr 2018 07:29:22 +0800 From: "Yao, Jiewen" To: "Kinney, Michael D" , "edk2-devel@lists.01.org" Thread-Topic: [RFC 0/4] Add FmpDevicePkg Thread-Index: AQHTzFvn1KOFiw3t2E68dmiyUTA0mKPxOTaQ Date: Wed, 4 Apr 2018 23:29:21 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AB37626@shsmsx102.ccr.corp.intel.com> References: <20180404212834.8644-1-michael.d.kinney@intel.com> In-Reply-To: <20180404212834.8644-1-michael.d.kinney@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNmE2ZGZmZDItN2NkZi00MzEzLWI4NDEtOTBkY2NmZmM4ZGNmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJnaDllTlB0SkwzWkdOTk1cL28wXC9CYkhlRWJYVksxRUxOaGwwR05LSkVJUDZuSEozNUx3WkFPTU5TbnBXeFJzN1cifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [RFC 0/4] Add FmpDevicePkg X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Apr 2018 23:29:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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)FmpPay= loadHeader || 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 SetPa= ckageInfo(). 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 th= e variable. VOID SetVersionInVariable ( UINT32 Version ) { EFI_STATUS Status; UINT32 Current; Status =3D EFI_SUCCESS; Current =3D GetVersionFromVariable(); if (Current !=3D Version) { Status =3D gRT->SetVariable ( VARNAME_VERSION, &gEfiCallerIdGuid, EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_AC= CESS, 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 p= rocessed. 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 !=3D GetBootModeHob ()) { LockAllVars (); } 8) I am surprised that we support "*" as variable name. Will it lock all va= riables with gEfiCallerIdGuid? Status =3D 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 ; Yao, Jiewen > > Subject: [RFC 0/4] Add FmpDevicePkg >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D922 >=20 > Based on content from the following branch: >=20 > https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport/MsCapsu > leUpdatePkg >=20 > Branch for review: >=20 > https://github.com/mdkinney/edk2/tree/Bug_922_FmpDevicePkg >=20 > 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. >=20 > Cc: Sean Brogan > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Michael D Kinney >=20 > 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 >=20 > 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/CapsuleUpdatePolicyLibNul= l > .c > create mode 100644 > FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNul= l > .inf > create mode 100644 > FmpDevicePkg/Library/CapsuleUpdatePolicyLibNull/CapsuleUpdatePolicyLibNul= l > .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 >=20 > -- > 2.14.2.windows.3