From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=40.92.4.22; helo=nam02-cy1-obe.outbound.protection.outlook.com; envelope-from=haojian.zhuang@outlook.com; receiver=edk2-devel@lists.01.org Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-oln040092004022.outbound.protection.outlook.com [40.92.4.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E2CCA2034D8C1 for ; Thu, 22 Feb 2018 00:51:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=UPw4WBMxtjv8oPZZlAa/B/Xn1MfUi57QsVqVgThAujo=; b=dTYiiPaDqahsfU4BtsVCp7yHUL2JowN0bkzCgGoanW3z/7SiPmS7SwM19zbT1vzGzzPODdaX6oNwYs3u4rTAP4A5fuJbycBu8gowhu0Lv57bL5D9b36mRIysE+R6AJTdmMr9NsReo9LuSnr4LBD+EXSXKe/U80iE9GqVRr2K0vMe0i5iqTcTh+09Ms5VBc9PPdRz3krrGLtDItj4RXxBfwOHorNJufLhs42g3lYoBVLnqMQcwLj4zHtfEJwv9hBmcrGZxy1r3gcRTkm22wfbaPzk8hYVNTJdf8I454ZmrEYgzpNubLdCrvNqjCwUkYUQAaa0dzoIierlA6jXxVux+g== Received: from CY1NAM02FT013.eop-nam02.prod.protection.outlook.com (10.152.74.56) by CY1NAM02HT075.eop-nam02.prod.protection.outlook.com (10.152.74.129) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.464.11; Thu, 22 Feb 2018 08:57:04 +0000 Received: from CY1PR15MB0730.namprd15.prod.outlook.com (10.152.74.57) by CY1NAM02FT013.mail.protection.outlook.com (10.152.75.162) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.506.19 via Frontend Transport; Thu, 22 Feb 2018 08:57:04 +0000 Received: from CY1PR15MB0730.namprd15.prod.outlook.com ([10.169.21.148]) by CY1PR15MB0730.namprd15.prod.outlook.com ([10.169.21.148]) with mapi id 15.20.0527.017; Thu, 22 Feb 2018 08:57:04 +0000 From: Haojian Zhuang To: "lersek@redhat.com" , Haojian Zhuang CC: "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "ard.biesheuvel@linaro.org" , "linaro-uefi@lists.linaro.org" Thread-Topic: [edk2] [PATCH 1/2] Platform/Hisilicon/HiKey960: add boot options Thread-Index: AQHTpm+sElJEhS3h6kiVoV9u2zFzi6OteVGAgAKvwgA= Date: Thu, 22 Feb 2018 08:57:04 +0000 Message-ID: References: <1518707649-16912-1-git-send-email-haojian.zhuang@linaro.org> <1518707649-16912-2-git-send-email-haojian.zhuang@linaro.org> <51cc9548-3c02-09c6-30a4-5846b03a1374@redhat.com> In-Reply-To: <51cc9548-3c02-09c6-30a4-5846b03a1374@redhat.com> Reply-To: "haojian.zhuang@linaro.org" Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-imapappendstamp: CY1PR15MB0730.namprd15.prod.outlook.com (15.20.0527.000) x-incomingtopheadermarker: OriginalChecksum:57657AB16683554DC5D22AEECC7917988FB336CFD7054E87F4CF91024223D94E; UpperCasedChecksum:C11E090461E8980BD9449C4FB17A1F5AE61BEB11432DAC65784D0200FDC3F90E; SizeAsReceived:7549; Count:49 x-ms-exchange-messagesentrepresentingtype: 1 x-tmn: [Amvj4cSWX9c0KDiONKOhUhUr1GFZLFWC] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; CY1NAM02HT075; 6:IV751+fn5EuATTNvM/P99KrtrWaEjN+o9N4wzf7n2k+EC9Z+ZOd82RAX1ERuUfkz1VbIp797Jv0WxQ1Yfu3G4DVGzJ5V4N6b9Jf+LTqn+2Mr7WbAZyCL8ZMmSJ2DRh0lRFuL3WvRKQrY9NKW1fQ3vukc8YijaUnPlQVVbPrxvH/uc5AqFRB1OhvMKJP+iqIxrw+2MHbyJCbveB7xAXqxVVWZRJqD8i9Ervj1/+mo3cS+Dajj5K4RfJg8G/kIIlPbSrH6AoZVE+9FLz0ewTjm60xdNm8KUs8JtuWfYtzb5PX+yODcVel1h1LlvH3YlpnC+AjLN2MWdaT6YMhINEhDm5WGJcSLT23A8DlWbwzwBgk=; 5:X9mQqq5JrWQfIyYQx+AEk76koEfKfeMJKJ3ICIENgka3zSDF3dEnngsaJDgmWMAeeVld7AlcYJn4UhEnSdlyd0O9fh9oFWak7MKelH3BkOlIfILi44v6Xld1J8Y8dmB53mA7RB5DWPpFUrL7EncuWGVe0/vkDvLVvOVYfef/MPo=; 24:CUzS8Qq/SSI0Od/Jnj/JjA7P2bOftCXXbb4uJrrsSFVpaQr4b9Px87Rbs88W+RL2iThavP/nOUWN2aWneAsaxOBssJv/PftIevfzNgABfvw=; 7:grmRTL5GF00pHYBLbzQU5Z3ldoz2bz4W0AlAwGxEnhQFEh2PLDEZNryxgPSP8ROHEyCOb1wsoysoJOsduHLOQyvddjlfwQj4M3W5M5sFUhvjldTT1nsVn5i+E9ehzsAxSKPj9DWB2ImabsPo7vJQsfNPp2u10HyRj4k26fsqreE63rU4KvJFNoaOxmMbJ/kFZh5fy0IFdxC1D9hAg26YzPc4EIBIE/MD2EAcQW7GmMvEOWSRBmsnsLtPSgbVXjDs x-incomingheadercount: 49 x-eopattributedmessage: 0 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(201702061078)(5061506573)(5061507331)(1603103135)(2017031320274)(2017031324274)(2017031323274)(2017031322404)(1601125374)(1603101448)(1701031045); SRVR:CY1NAM02HT075; x-ms-traffictypediagnostic: CY1NAM02HT075: x-ms-office365-filtering-correlation-id: ac70f4c5-30db-4d36-12e5-08d579d23e92 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(444000031); SRVR:CY1NAM02HT075; BCL:0; PCL:0; RULEID:; SRVR:CY1NAM02HT075; x-forefront-prvs: 059185FE08 x-forefront-antispam-report: SFV:NSPM; SFS:(7070007)(98901004); DIR:OUT; SFP:1901; SCL:1; SRVR:CY1NAM02HT075; H:CY1PR15MB0730.namprd15.prod.outlook.com; FPR:; SPF:None; LANG:; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ac70f4c5-30db-4d36-12e5-08d579d23e92 X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Feb 2018 08:57:04.2334 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1NAM02HT075 Subject: Re: [PATCH 1/2] Platform/Hisilicon/HiKey960: add boot options X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Feb 2018 08:51:05 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-ID: <068E9A519708C047820A65574B1A30C9@sct-15-20-156-4-msonline-outlook-b071b.templateTenant> Content-Transfer-Encoding: quoted-printable On 02/20/2018 11:54 PM, Laszlo Ersek wrote:=0A= > Hi,=0A= > =0A= > On 02/15/18 16:14, Haojian Zhuang wrote:=0A= >> Add four boot options in emu variable region. They are=0A= >> "Boot on SD", "Grub", "Android Boot" and "Android Fastboot".=0A= >>=0A= >> Contributed-under: TianoCore Contribution Agreement 1.1=0A= >> Signed-off-by: Haojian Zhuang =0A= >> ---=0A= >> Platform/Hisilicon/HiKey960/HiKey960.dsc | 3 +=0A= >> Platform/Hisilicon/HiKey960/HiKey960.fdf | 241 +++++++++++++++++++++++= +++++++-=0A= >> 2 files changed, 242 insertions(+), 2 deletions(-)=0A= >>=0A= >> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilic= on/HiKey960/HiKey960.dsc=0A= >> index 98289c0..a6864c1 100644=0A= >> --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc=0A= >> +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc=0A= >> @@ -78,6 +78,9 @@=0A= >> gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|FALSE=0A= >> =0A= >> [PcdsFixedAtBuild.common]=0A= >> + gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0x1AD880= 48=0A= >> + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x7FB8=0A= >> +=0A= >> gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4=0A= >> =0A= >> gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"Alpha"=0A= >> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.fdf b/Platform/Hisilic= on/HiKey960/HiKey960.fdf=0A= >> index 655032a..af10430 100644=0A= >> --- a/Platform/Hisilicon/HiKey960/HiKey960.fdf=0A= >> +++ b/Platform/Hisilicon/HiKey960/HiKey960.fdf=0A= >> @@ -26,12 +26,12 @@=0A= >> =0A= >> [FD.BL33_AP_UEFI]=0A= >> BaseAddress =3D 0x1AC98000|gArmTokenSpaceGuid.PcdFdBaseAddress # Th= e base address of the Firmware in NOR Flash.=0A= >> -Size =3D 0x000F0000|gArmTokenSpaceGuid.PcdFdSize # The= size in bytes of the FLASH Device=0A= >> +Size =3D 0x00100000|gArmTokenSpaceGuid.PcdFdSize # The= size in bytes of the FLASH Device=0A= >> ErasePolarity =3D 1=0A= >> =0A= >> # This one is tricky, it must be: BlockSize * NumBlocks =3D Size=0A= >> BlockSize =3D 0x00001000=0A= >> -NumBlocks =3D 0xF0=0A= >> +NumBlocks =3D 0x100=0A= >> =0A= >> ######################################################################= ##########=0A= >> #=0A= >> @@ -53,6 +53,243 @@ NumBlocks =3D 0xF0=0A= >> gArmTokenSpaceGuid.PcdFvBaseAddress|gArmTokenSpaceGuid.PcdFvSize=0A= >> FV =3D FVMAIN_COMPACT=0A= >> =0A= >> +0x000F0000|0x00008000=0A= >> +gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|gEfiMdeMod= ulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize=0A= >> +DATA =3D {=0A= >> + ## This is the EFI_FIRMWARE_VOLUME_HEADER=0A= >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,=0A= >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,=0A= >> + # FileSystemGuid: gEfiSystemNvDataFvGuid =3D=0A= >> + 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,=0A= >> + 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,=0A= >> + # FvLength: 0x8000=0A= >> + 0x00, 0x80, 0x0, 0x00, 0x00, 0x00, 0x00, 0x00,=0A= >> + #Signature "_FVH" #Attributes=0A= >> + 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,=0A= >> + #HeaderLength #CheckSum #ExtHeaderOffset #Reserved #Revision=0A= >> + 0x48, 0x00, 0x36, 0x09, 0x00, 0x00, 0x00, 0x02,=0A= >> + #Blockmap[0]: 8 Blocks * 0x1000 Bytes / Block=0A= >> + 0x08, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,=0A= >> + #Blockmap[1]: End=0A= >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,=0A= >> + ## end of EFI_FIRMWARE_VOLUME_HEADER.=0A= >> +=0A= >> + ### Offset (0x48) ###=0A= >> + ## This is the VARIABLE_STORE_HEADER gEfiVariableGuid=0A= >> + 0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,=0A= >> + 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,=0A= >> + #Size: 0x8000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariab= leSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) =3D 0x7FB8=0A= >> + 0xB8, 0x7F, 0x00, 0x00,=0A= >> + #FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32= =0A= >> + 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,=0A= >> +=0A= >> + ### Offset (0x64) ###=0A= >> + ## This is the VARIABLE_HEADER=0A= >> + #StartId: VARIABLE_DATA #State: VAR_ADDED #Reserved=0A= >> + 0xAA, 0x55, 0x3F, 0x00,=0A= >> + #Attributes: EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACC= ESS=0A= >> + 0x03, 0x00, 0x00, 0x00,=0A= >> + #NameSize:=0A= >> + 0x14, 0x00, 0x00, 0x00,=0A= >> + #DataSize:=0A= >> + 0x18, 0x00, 0x00, 0x00,=0A= >> + #VariableGuid: gEfiGlobalVariableGuid=0A= >> + 0x61, 0xDF, 0xE4, 0x8B, 0xCA, 0x93, 0xD2, 0x11,=0A= >> + 0xAA, 0x0D, 0x00, 0xE0, 0x98, 0x03, 0x2B, 0x8C,=0A= >> + ## End of VARIABLE_HEADER. Offset (0x84)=0A= >> + #VariableName: BootOrder=0A= >> + 0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00,=0A= >> + 0x4F, 0x00, 0x72, 0x00, 0x64, 0x00, 0x65, 0x00,=0A= >> + 0x72, 0x00, 0x00, 0x00,=0A= >> + #Data=0A= >> + 0x00, 0x00, 0x01, 0x00, 0x02, 0x00, 0x03, 0x00,=0A= >> + 0x04, 0x00, 0x05, 0x00, 0x06, 0x00, 0x07, 0x00,=0A= >> + 0x08, 0x00, 0x09, 0x00, 0x0a, 0x00, 0x0b, 0x00,=0A= >> + ### End of BootOrder.=0A= >> +=0A= >> + ### Offset (0xB0) ###=0A= >> + ## This is the VARIABLE_HEADER=0A= >> + #StartId: VARIABLE_DATA #State: VAR_ADDED #Reserved=0A= >> + 0xAA, 0x55, 0x3F, 0x00,=0A= >> + #Attributes: EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACC= ESS=0A= >> + 0x03, 0x00, 0x00, 0x00,=0A= >> + #NameSize:=0A= >> + 0x12, 0x00, 0x00, 0x00,=0A= >> + #DataSize:=0A= >> + 0x42, 0x00, 0x00, 0x00,=0A= >> + #VariableGuid: gEfiGlobalVariableGuid=0A= >> + 0x61, 0xDF, 0xE4, 0x8B, 0xCA, 0x93, 0xD2, 0x11,=0A= >> + 0xAA, 0x0D, 0x00, 0xE0, 0x98, 0x03, 0x2B, 0x8C,=0A= >> + #VariableName: Boot0000=0A= >> + 0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00,=0A= >> + 0x30, 0x00, 0x30, 0x00, 0x30, 0x00, 0x30, 0x00,=0A= >> + 0x00, 0x00,=0A= >> + ### Offset (0xE2), Size(0x32) ###=0A= >> + #Data:=0A= >> + #Attributes: LOAD_OPTION_ACTIVE=0A= >> + 0x01, 0x00, 0x00, 0x00,=0A= >> + #FilePathListLength=0A= >> + 0x26, 0x00,=0A= >> + #Description: "Boot on SD"=0A= >> + 0x42, 0x00, 0x6F, 0x00, 0x6F, 0x00, 0x74, 0x00, #Boot=0A= >> + 0x20, 0x00, 0x6F, 0x00, 0x6E, 0x00, 0x20, 0x00, # on=0A= >> + 0x53, 0x00, 0x44, 0x00, 0x00, 0x00, #SD=0A= >> + #FilePathList (0x26)=0A= >> + 0x01, 0x04, 0x1D, 0x00, 0x5B, 0x90, 0x51, 0x0D,=0A= >> + 0x7E, 0xB7, 0x2A, 0x45, 0xA2, 0xC0, 0xEC, 0xA0,=0A= >> + 0xCC, 0x8D, 0x51, 0x4A, 0x00, 0xF0, 0x37, 0xFF,=0A= >> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x1A, 0x05,=0A= >> + 0x00, 0x00, 0x7F, 0xFF, 0x04, 0x00,=0A= >> + ### Offset (0x124), Size (0x42) ###=0A= >> + ### End of Boot0000=0A= >> +=0A= >>=0A= > =0A= > While in both ArmVirtPkg and OvmfPkg we use a similar DATA definition=0A= > for *formatting* the pristine variable store (just setting the=0A= > absolutely necessary headers via a hexdump), I think that adding=0A= > "business content" like this is a bad idea. For one, if the defaults=0A= > have to be updated ever again, the patches for the DATA definitions will= =0A= > look terrible. It's hard to review and maintain hexdump like these; we=0A= > should keep such DATA definitions to the absolute minimum.=0A= > =0A= > Such "default" boot options should be set by the platform's=0A= > PlatformBootManagerLib instance. PlatformBootManagerLib is responsible=0A= > for generating boot options. In doing that, PlatformBootManagerLib can=0A= > call helper functions from UefiBootManagerLib, so everything need not be= =0A= > done manually.=0A= > =0A= > For example, EfiBootManagerGetLoadOptions() can be used to retrieve the= =0A= > current boot options. Then, you can iterate over the four boot options=0A= > that you want to ensure exist -- for each:=0A= > =0A= > - create a EFI_BOOT_MANAGER_LOAD_OPTION object with=0A= > EfiBootManagerInitializeLoadOption(),=0A= > - check if the option already exists, with EfiBootManagerFindLoadOption()= ,=0A= > - if the option doesn't exist yet, add it with=0A= > EfiBootManagerAddLoadOptionVariable(),=0A= > - free the EFI_BOOT_MANAGER_LOAD_OPTION object with=0A= > EfiBootManagerFreeLoadOption()=0A= > =0A= > Finally, free the originally retrieved set of boot options with=0A= > EfiBootManagerFreeLoadOptions().=0A= > =0A= > You can find example code in=0A= > "ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c".=0A= > =0A= > Thanks,=0A= > Laszlo=0A= > =0A= =0A= Hi Laszlo,=0A= =0A= Yes, it's a bit hard to review and maintain. Actually, I implemented a =0A= PlatformBootManagerLib in this link =0A= (https://github.com/96boards-hikey/OpenPlatformPkg/blob/testing/hikey960_v1= .3.4/Platforms/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c).=0A= =0A= I format it with hard code since I want to re-use =0A= PlatformBootManagerLib in ArmPkg. Because these boot options only exist =0A= on HiKey/HiKey960 platforms, and most of them are same of the one in =0A= ArmPkg. If I append them into ArmPkg, it's not common enough. At last, I = =0A= hope to implement a non-volatile region on flash devices in the future. =0A= It means that implementing a HiKey specific PlatformBootManagerLib is =0A= only a temporary solution.=0A= =0A= It seems that I have two options to go ahead.=0A= 1. Move those hard code boot options into a FV file, and put the FV file=0A= in edk2-non-osi repository.=0A= =0A= 2. Implement a HiKey related PlatformBootManagerLib.=0A= =0A= Which one do you prefer?=0A= =0A= Best Regards=0A= Haojian=0A=