From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c00::234; helo=mail-pf0-x234.google.com; envelope-from=haojian.zhuang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf0-x234.google.com (mail-pf0-x234.google.com [IPv6:2607:f8b0:400e:c00::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 061A42034D8C5 for ; Thu, 22 Feb 2018 03:43:52 -0800 (PST) Received: by mail-pf0-x234.google.com with SMTP id 17so1968355pfw.11 for ; Thu, 22 Feb 2018 03:49:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:thread-topic:thread-index:date:message-id :references:in-reply-to:reply-to:accept-language:content-language :content-id:content-transfer-encoding:mime-version; bh=NF6mkAtB43tcY91EvMfGPaXLRr9eCULMT+OI/afmmcc=; b=HgzuQ6rBpljycnLBJv8B5HdTh4mJIZVF+JylpmJIBs6C92BE5WRXGBuLWljUHd5Btk SWk+U3H3zejhJkrO3Zuuu+0JmsIE93WFRbSMu09QEZZ8ckT+yhHjsYhKZoL0i3xbkHBa ESwbXMKbz7fZ31a+qbiYkvOb8arPdk3uR/P8c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:thread-topic:thread-index :date:message-id:references:in-reply-to:reply-to:accept-language :content-language:content-id:content-transfer-encoding:mime-version; bh=NF6mkAtB43tcY91EvMfGPaXLRr9eCULMT+OI/afmmcc=; b=O+MoUHvy7l7v7hyQrZj0w+xEuMQJXdWhFCoD1YOPOWLareuJFKaStRhxoE6X4ygTMA K5xc3aIw9Fed15RmBASxrwAOT3bPnFdYymJICbomhnn2YW+RA1sQZIMZthLstHuWlkbD 67O2UhbBlUmjHFSIqn/pe4Dudsp707hNsWZ0IBb0oSAsEGn9UtOBsepDTXyXKjW6Etcq MkySxvAEv9BlsXVJ+CXxwP3aJWmvPOXZze2GItSKo5jd/BN10BnLx4h5iY6DosQ8gaFp 9OJWVQ9wEHOHN84F2b0VgWuq5fROUI6CszkveQL1OeeCjB1oYIXjitXYcbiXny50hmwH YEKg== X-Gm-Message-State: APf1xPD755ePPgZqJcYYbE8ieXYfai0KXcPViDCSm/OxWAsbp3oaSc5g +9I6evzl1q/ISDY9JRF2XvhKRQ== X-Google-Smtp-Source: AH8x225aJsU9e4MseRjD6GBPotJ8dpigt+zNTnTtKiGpYpXRaegAxMIEz/pZr7LWKVQGPkp/cTXcrA== X-Received: by 10.101.81.12 with SMTP id f12mr5374567pgq.81.1519300192642; Thu, 22 Feb 2018 03:49:52 -0800 (PST) Received: from CY1PR15MB0730.namprd15.prod.outlook.com ([132.245.253.237]) by smtp.gmail.com with ESMTPSA id f79sm76243743pfd.103.2018.02.22.03.49.50 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 22 Feb 2018 03:49:51 -0800 (PST) From: Haojian Zhuang To: "lersek@redhat.com" 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+sElJEhS3h6kiVoV9u2zFzi6OteVGAgAKvwgCAACGaAIAADrUA X-MS-Exchange-MessageSentRepresentingType: 2 Date: Thu, 22 Feb 2018 11:49:49 +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> <61c94cc9-1b5c-95a7-fdd9-812609c68561@redhat.com> In-Reply-To: <61c94cc9-1b5c-95a7-fdd9-812609c68561@redhat.com> Reply-To: "haojian.zhuang@linaro.org" Accept-Language: en-US X-MS-Has-Attach: X-MS-Exchange-Organization-SCL: -1 X-MS-TNEF-Correlator: X-MS-Exchange-Organization-RecordReviewCfmType: 0 x-ms-exchange-imapappendstamp: CY1PR15MB0730.namprd15.prod.outlook.com (15.20.0527.000) MIME-Version: 1.0 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 11:43:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-ID: <1B315674FDD23B4CA7FAA71148DADF02@sct-15-20-156-4-msonline-outlook-b071b.templateTenant> Content-Transfer-Encoding: quoted-printable On 02/22/2018 06:56 PM, Laszlo Ersek wrote: > On 02/22/18 09:57, Haojian Zhuang wrote: >> On 02/20/2018 11:54 PM, Laszlo Ersek wrote: >>> Hi, >>> >>> On 02/15/18 16:14, Haojian Zhuang wrote: >>>> Add four boot options in emu variable region. They are >>>> "Boot on SD", "Grub", "Android Boot" and "Android Fastboot". >>>> >>> >>> While in both ArmVirtPkg and OvmfPkg we use a similar DATA definition >>> for *formatting* the pristine variable store (just setting the >>> absolutely necessary headers via a hexdump), I think that adding >>> "business content" like this is a bad idea. For one, if the defaults >>> have to be updated ever again, the patches for the DATA definitions wil= l >>> look terrible. It's hard to review and maintain hexdump like these; we >>> should keep such DATA definitions to the absolute minimum. >>> >>> Such "default" boot options should be set by the platform's >>> PlatformBootManagerLib instance. PlatformBootManagerLib is responsible >>> for generating boot options. In doing that, PlatformBootManagerLib can >>> call helper functions from UefiBootManagerLib, so everything need not b= e >>> done manually. >>> >>> For example, EfiBootManagerGetLoadOptions() can be used to retrieve the >>> current boot options. Then, you can iterate over the four boot options >>> that you want to ensure exist -- for each: >>> >>> - create a EFI_BOOT_MANAGER_LOAD_OPTION object with >>> EfiBootManagerInitializeLoadOption(), >>> - check if the option already exists, with EfiBootManagerFindLoadOption= (), >>> - if the option doesn't exist yet, add it with >>> EfiBootManagerAddLoadOptionVariable(), >>> - free the EFI_BOOT_MANAGER_LOAD_OPTION object with >>> EfiBootManagerFreeLoadOption() >>> >>> Finally, free the originally retrieved set of boot options with >>> EfiBootManagerFreeLoadOptions(). >>> >>> You can find example code in >>> "ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c". >>> >>> Thanks, >>> Laszlo >>> >> >> Hi Laszlo, >> >> Yes, it's a bit hard to review and maintain. Actually, I implemented a >> PlatformBootManagerLib in this link >> (https://github.com/96boards-hikey/OpenPlatformPkg/blob/testing/hikey960= _v1.3.4/Platforms/Hisilicon/Library/PlatformBootManagerLib/PlatformBm.c). >> >> I format it with hard code since I want to re-use >> PlatformBootManagerLib in ArmPkg. Because these boot options only exist >> on HiKey/HiKey960 platforms, and most of them are same of the one in >> ArmPkg. If I append them into ArmPkg, it's not common enough. At last, I >> hope to implement a non-volatile region on flash devices in the future. >> It means that implementing a HiKey specific PlatformBootManagerLib is >> only a temporary solution. >=20 > Sorry, I don't understand how this follows. Again: why is it only a > temporary solution to implement a PlatformBootManagerLib instance for HiK= ey? >=20 > The library class says "Platform" in the name. Platforms are supposed to > provide it, one way or another. >=20 > If you want to minimize code duplication between ArmPkg and HiKey, it > should be possible to factor out another library class from ArmPkg's > PlatformBootManagerLib instance. It could be a function that returns the > list of platform-specific boot options -- as an array of > EFI_BOOT_MANAGER_LOAD_OPTION elements -- that should always exist. Then > ArmPkg's PlatformBootManagerLib would perform the iteration that I > described above. >=20 > Platforms different from HiKey would return an empty array, while HiKey > could return the four options it needs. >=20 > Another benefit is that these four boot options would be restored on > every boot, even if the user deleted them. This seems independent of > whether the HiKey platform has functional non-volatile storage or not. >=20 >> It seems that I have two options to go ahead. >> 1. Move those hard code boot options into a FV file, and put the FV file >> in edk2-non-osi repository. >> >> 2. Implement a HiKey related PlatformBootManagerLib. >=20 > I think option #2 is far superior. You don't have to duplicate all code > from ArmPkg's PlatformBootManagerLib for that, you can extract another > utility library class / callback function just for providing the options > you always want. >=20 > Or perhaps you *don't* want them always, just until NVRAM support > arrives to HiKey? And, in that case, it will be alright for the user to > delete these options permanently? Yes, I hope the default boot options could be dropped when NVRAM support=20 arrives on HiKey. User could decide which boot option is really good for=20 him. >=20 > ... I still think extracting a new lib class for these default boot > options would be better. If at some point you'd like to drop the default > boot options, it's easy to switch the new callback to returning zero > elements. The lib class can remain useful to other development platforms. > OK. I'll try to add the new callback in it. Best Regards Haojian