Hi Leif,
Thank you for rewriting this in a much less invasive way. I have a few comments, but I hope they are simple enough that they can be addressed quickly. On Tue, May 16, 2017 at 03:31:17PM +0800, Chenhui Sun wrote:From: huangming <huangming23@huawei.com> Support the feature that BIOS get boot option from BMC and put it in the first boot order.Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: huangming <huangming23@huawei.com> Signed-off-by: Heyi Guo <guoheyi@huawei.com> Signed-off-by: Yi Li <Phoenix.liyi@huawei.com> Signed-off-by: Chenhui Sun <chenhui.sun@linaro.org> --- Chips/Hisilicon/Include/Library/IpmiCmdLib.h | 94 +++++ .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 393 +++++++++++++++++++++ .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf | 4 + 3 files changed, 491 insertions(+) create mode 100644 Chips/Hisilicon/Include/Library/IpmiCmdLib.h diff --git a/Chips/Hisilicon/Include/Library/IpmiCmdLib.h b/Chips/Hisilicon/Include/Library/IpmiCmdLib.h new file mode 100644 index 0000000..d0602a4 --- /dev/null +++ b/Chips/Hisilicon/Include/Library/IpmiCmdLib.h @@ -0,0 +1,94 @@ +/** @file +* +* Copyright (c) 2016, Hisilicon Limited. All rights reserved. +* Copyright (c) 2016, Linaro Limited. All rights reserved.Since this file is added in 2017, the copyright statements should probably either say "2017" or "2016-2017".+* +* This program and the accompanying materials +* are licensed and made available under the terms and conditions of the BSD License +* which accompanies this distribution. The full text of the license may be found at +* http://opensource.org/licenses/bsd-license.php +* +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +* +**/ + +#ifndef _IPMI_CMD_LIB_H_ +#define _IPMI_CMD_LIB_H_ + +#define BOOT_OPTION_BOOT_FLAG_VALID 1 +#define BOOT_OPTION_BOOT_FLAG_INVALID 0 + +typedef enum { + NoOverride = 0x0, + ForcePxe, + ForceDefaultHardDisk, + ForceDefaultHardDiskSafeMode, + ForceDefaultDiagnosticPartition, + ForceDefaultCD, + ForceSetupUtility, + ForceRemoteRemovableMedia, + ForceRemoteCD, + ForcePrimaryRemoteMedia, + ForceRemoteHardDisk = 0xB, + ForcePrimaryRemovableMedia = 0xF +} BOOT_DEVICE_SELECTOR; + +// +// Get System Boot Option data structure +// +typedef struct { + UINT8 ParameterVersion :4; + UINT8 Reserved1 :4; + UINT8 ParameterSelector :7; + UINT8 ParameterValid :1; + // + // Boot Flags Data 1 + // + UINT8 Reserved2 :5; + UINT8 BiosBootType :1; + UINT8 Persistent :1; + UINT8 BootFlagsValid :1; + // + // Boot Flags Data 2 + // + UINT8 LockResetBtn :1; + UINT8 ScreenBlank :1; + UINT8 BootDeviceSelector :4; + UINT8 LockKeyboard :1; + UINT8 ClearCmos :1; + // + // Boot Flags Data 3 + // + UINT8 ConsoleRedirectionControl :2; + UINT8 LockSleepBtn :1; + UINT8 UserPasswordByPass :1; + UINT8 Reserved3 :1; + UINT8 FirmwareVerbosity :2; + UINT8 LockPowerBtn :1; + // + // Boot Flags Data 4 + // + UINT8 MuxControlOverride :3; + UINT8 ShareModeOverride :1; + UINT8 Reserved4 :4; + // + // Boot Flags Data 5 + // + UINT8 DeviceInstanceSelector :5; + UINT8 Reserved5 :3; +} IPMI_GET_BOOT_OPTION; + +EFI_STATUS +EFIAPI +IpmiCmdSetSysBootOptions ( + OUT IPMI_GET_BOOT_OPTION *BootOption + ); + +EFI_STATUS +EFIAPI +IpmiCmdGetSysBootOptions ( + IN IPMI_GET_BOOT_OPTION *BootOption + ); + +#endif diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c index efefeb6..56360f2 100644 --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c @@ -30,7 +30,14 @@ #include <Protocol/PciRootBridgeIo.h> #include "IntelBdsPlatform.h" +#include <Library/PrintLib.h> +#include <Library/IpmiCmdLib.h> +#include <Library/GenericBdsLib.h> +#include <Protocol/DevicePathToText.h> +#include <Guid/GlobalVariable.h>Please insert these new includes alphabetically sorted with the already existing includes.+GUID gOemBootVaraibleGuid = {0xb7784577, 0x5aaf, 0x4557, {0xa1, 0x99, + 0xd4, 0xa4, 0x2f, 0x45, 0x06, 0xf8} }; //3CEF354A-3B7A-4519-AD70-72A134698311 GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70, @@ -142,6 +149,388 @@ STATIC PLATFORM_USB_KEYBOARD mUsbKeyboard = { } }; +STATIC +UINT16 +GetBBSTypeFromUsbPath ( + IN CONST EFI_DEVICE_PATH_PROTOCOL *UsbPath + ) +{ + EFI_STATUS Status; + EFI_HANDLE *FileSystemHandles; + UINTN NumberFileSystemHandles; + UINTN Index; + EFI_DEVICE_PATH_PROTOCOL *FileSysPath; + EFI_DEVICE_PATH_PROTOCOL *Node; + EFI_DEVICE_PATH_TO_TEXT_PROTOCOL *DevPathToText; + CHAR16 *UsbPathTxt; + CHAR16 *FileSysPathTxt; + UINT16 Result; + + Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL, (VOID **)&DevPathToText); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Locate DevicePathToTextPro %r\n", Status)); + return BBS_TYPE_UNKNOWN; + } + + Result = BBS_TYPE_UNKNOWN; + UsbPathTxt = DevPathToText->ConvertDevicePathToText (UsbPath, TRUE, TRUE); + if (UsbPathTxt == NULL) { + return Result; + } + + Status = gBS->LocateHandleBuffer ( + ByProtocol, + &gEfiSimpleFileSystemProtocolGuid, + NULL, + &NumberFileSystemHandles, + &FileSystemHandles + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Locate SimpleFileSystemProtocol error(%r)\n", Status)); + FreePool (UsbPathTxt); + return BBS_TYPE_UNKNOWN; + } + + for (Index = 0; Index < NumberFileSystemHandles; Index++) { + FileSysPath = DevicePathFromHandle (FileSystemHandles[Index]); + FileSysPathTxt = DevPathToText->ConvertDevicePathToText (FileSysPath, TRUE, TRUE); + + if (FileSysPathTxt == NULL) { + continue; + } + + if (StrnCmp (UsbPathTxt, FileSysPathTxt, StrLen (UsbPathTxt)) == 0) { + Node = FileSysPath; + while (!IsDevicePathEnd (Node)) { + if ((DevicePathType (Node) == MEDIA_DEVICE_PATH) && + (DevicePathSubType (Node) == MEDIA_CDROM_DP)) { + Result = BBS_TYPE_CDROM; + break; + } + Node = NextDevicePathNode (Node); + } + } + + FreePool (FileSysPathTxt); + + if (Result != BBS_TYPE_UNKNOWN) { + break; + } + }This is a comment that does not require addressing, but it is feedback for future submissions: This for-loop is a very good example of an opportunity to make the source code more readable by breaking it out into a separate function. This would let the entire body of this function fit on one screen page, as well as giving the ability to provide a descriptive function name that explains what the effect of the loop does without having to read through it all. (It would also mean that temporary variables used only in the for-loop would not take up space at the start of this function.) This is nearly exactly the cut-off point for me - if there was only a couple of more lines of code in the loop, I would always ask for it to be broken out.+ + if (NumberFileSystemHandles != 0) { + FreePool (FileSystemHandles); + } + + FreePool (UsbPathTxt); + + return Result; +} + +STATIC +UINT16 +GetBBSTypeByDevicePath ( + IN EFI_DEVICE_PATH_PROTOCOL *DevicePath + ) +{ + VENDOR_DEVICE_PATH *Vendor; + EFI_DEVICE_PATH_PROTOCOL *Node; + UINT16 Result; + + Result = BBS_TYPE_UNKNOWN; + if (DevicePath == NULL) { + return Result; + } + + Node = DevicePath; + while (!IsDevicePathEnd (Node)) { + switch (DevicePathType (Node)) { + case MEDIA_DEVICE_PATH: + if (DevicePathSubType (Node) == MEDIA_CDROM_DP) { + Result = BBS_TYPE_CDROM; + } + break; + + case MESSAGING_DEVICE_PATH: + switch (DevicePathSubType (Node)) {But nested switch statements are always too hard to read in one place, so please turn this inner one into a helper function.+ case MSG_MAC_ADDR_DP: + Result = BBS_TYPE_EMBEDDED_NETWORK; + break; + + case MSG_USB_DP: + Result = GetBBSTypeFromUsbPath (DevicePath); + if (Result == BBS_TYPE_UNKNOWN) { + Result = BBS_TYPE_USB; + } + break; + + case MSG_SATA_DP: + Result = BBS_TYPE_HARDDRIVE; + break; + + case MSG_VENDOR_DP: + Vendor = (VENDOR_DEVICE_PATH*)(Node); + if ((&Vendor->Guid) != NULL) { + if (CompareGuid (&Vendor->Guid, &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS))) { + Result = BBS_TYPE_HARDDRIVE; + } + } + break; + + default: + Result = BBS_TYPE_UNKNOWN; + break; + } + break; + + default: + Result = BBS_TYPE_UNKNOWN; + break; + } + + if (Result != BBS_TYPE_UNKNOWN) { + return Result; + } + + Node = NextDevicePathNode (Node); + } + + return Result; +} + +STATIC +EFI_STATUS +GetBmcBootOptionsSetting ( + OUT IPMI_GET_BOOT_OPTION *BmcBootOpt + ) +{ + EFI_STATUS Status; + + Status = IpmiCmdGetSysBootOptions (BmcBootOpt); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Get iBMC BootOpts %r!\n", Status)); + return Status; + } + + if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) { + return EFI_NOT_FOUND; + } + + if (BmcBootOpt->Persistent) { + BmcBootOpt->BootFlagsValid = BOOT_OPTION_BOOT_FLAG_VALID; + } else { + BmcBootOpt->BootFlagsValid = BOOT_OPTION_BOOT_FLAG_INVALID; + } + + Status = IpmiCmdSetSysBootOptions (BmcBootOpt); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Set iBMC BootOpts %r!\n", Status)); + } + + return Status; +} + +STATIC +VOID +RestoreBootOrder ( + VOID + ) +{ + EFI_STATUS Status; + UINT16 *BootOrder; + UINTN BootOrderSize; + + GetVariable2 (L"BootOrderBak", &gOemBootVaraibleGuid, (VOID **) &BootOrder, &BootOrderSize); + if (BootOrder == NULL) { + return ; + }OK, here is the only _functional_ feedback I have on this implementation: I would prefer if this solution did not use an environment variable to keep track of whether "BootOrder" had been modified or not. A STATIC global variable called mBootOrderBackup could hold the original value. (With a comment next to its initialization explaining that this will be used to restore BootOrder on ReadyToBoot.)
Think about this case: suppose that the
board was reset after the BootOrder variable was set, but not run
to ReadyToBoot,
The following code will restore BootOrder
variable from BootOrderBak vairiable in next boot.
PlatformBdsInit (
VOID
)
{
//Signal EndofDxe Event
EfiEventGroupSignal(&gEfiEndOfDxeEventGroupGuid);
// restore BootOrder variable if
necessary.
RestoreBootOrder();
+ + Print(L"Restore BootOrder(%d).\n", BootOrderSize / sizeof (UINT16)); + + Status = gRT->SetVariable ( + L"BootOrder", + &gEfiGlobalVariableGuid, + EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, + BootOrderSize, + BootOrder + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SetVariable BootOrder %r!\n", Status)); + } + + Status = gRT->SetVariable ( + L"BootOrderBak", + &gOemBootVaraibleGuid, + EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, + 0, + NULL + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "SetVariable BootOrderBak %r!\n", Status)); + } + + FreePool (BootOrder); + + return; +} + + +VOID +RestoreBootOrderOnReadyToBoot ( + IN EFI_EVENT Event, + IN VOID *Context + ) +{ + // restore BootOrder variable in normal condition. + RestoreBootOrder(); +} + +STATIC +VOID +AlterBootOrder ( + IN UINT16 BootType + ) +{ + EFI_STATUS Status; + EFI_EVENT Event; + CHAR8 *Buf; + UINT16 *NewOrder; + UINT16 *RemainBoots; + UINT16 *BootOrder; + UINTN BootOrderSize; + CHAR16 OptionName[sizeof ("Boot####")]; + UINTN Index; + LIST_ENTRY BootOptionList; + BDS_COMMON_OPTION *Option; + UINTN SelectCnt; + UINTN RemainCnt; + + InitializeListHead (&BootOptionList); + + GetEfiGlobalVariable2 (L"BootOrder", (VOID **) &BootOrder, &BootOrderSize); + if (BootOrder == NULL) { + return ; + } + + Buf = AllocatePool(BootOrderSize * 2);Space before '('.+ if (Buf == NULL) { + DEBUG ((DEBUG_ERROR, "Out of resources.")); + goto Exit; + } + + NewOrder = (UINT16 *) Buf; + RemainBoots = (UINT16 *) (Buf + BootOrderSize);As far as I can see, the CHAR8* "Buf" is used only from phobia of pointer arithmetic, and is ignored beyond having been casted to a higher alignment than its type guarantees. Please use only pointers of the appropriate alignment contraints, and get rid of the spurious casts.+ SelectCnt = 0; + RemainCnt = 0; + + for (Index = 0; Index < BootOrderSize / sizeof (UINT16); Index++) { + UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x", BootOrder[Index]); + Option = BdsLibVariableToOption (&BootOptionList, OptionName); + if (Option == NULL) { + DEBUG ((DEBUG_ERROR, "Boot%04x is invalid option!\n", BootOrder[Index])); + continue; + } + + if (GetBBSTypeByDevicePath (Option->DevicePath) == BootType) { + NewOrder[SelectCnt++] = BootOrder[Index]; + } else { + RemainBoots[RemainCnt++] = BootOrder[Index]; + } + } + + if (SelectCnt != 0) { + // append RemainBoots to NewOrder + for (Index = 0; Index < RemainCnt; Index++) { + NewOrder[SelectCnt + Index] = RemainBoots[Index]; + } + + if (CompareMem (NewOrder, BootOrder, BootOrderSize) != 0) { + Status = gRT->SetVariable ( + L"BootOrderBak", + &gOemBootVaraibleGuid, + EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, + BootOrderSize, + BootOrder + ); + if (EFI_ERROR (Status)) { + goto Exit; + }mBootOrderBackup = BootOrder;+ + Status = gRT->SetVariable ( + L"BootOrder", + &gEfiGlobalVariableGuid, + EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE, + BootOrderSize, + NewOrder + ); + if (EFI_ERROR (Status)) { + goto Exit; + } + + // Register notify function to restore BootOrder variable on ReadyToBoot Event. + Status = gBS->CreateEventEx ( + EVT_NOTIFY_SIGNAL, + TPL_CALLBACK, + RestoreBootOrderOnReadyToBoot, + NULL, + &gEfiEventReadyToBootGuid, + &Event + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "Create ready to boot event %r!\n", Status)); + } + } + } + +Exit:if (!mBootOrderBackup)+ FreePool (BootOrder); + if (Buf != NULL) { + FreePool (Buf); + } + + return ; +} + +STATIC +VOID +DealBmcBootType(I would recommend calling it HandleBmcBootType().+ VOID + ) +{ + EFI_STATUS Status; + IPMI_GET_BOOT_OPTION BmcBootOpt; + UINT16 BootType; + + Status = GetBmcBootOptionsSetting (&BmcBootOpt); + if (EFI_ERROR (Status)) { + return; + } + + Print(L"Boot Type from BMC is %x\n", BmcBootOpt.BootDeviceSelector);Space before '('.+ + switch (BmcBootOpt.BootDeviceSelector) { + case ForcePxe: + BootType = BBS_TYPE_EMBEDDED_NETWORK; + break; + + case ForcePrimaryRemovableMedia: + BootType = BBS_TYPE_USB; + break; + + case ForceDefaultHardDisk: + BootType = BBS_TYPE_HARDDRIVE; + break; + + case ForceDefaultCD: + BootType = BBS_TYPE_CDROM; + break; + + default: + return; + } + + AlterBootOrder (BootType);Actually, given what that function does, I think it would make sense to call it SetBootOrder().+} // // BDS Platform Functions @@ -159,6 +548,9 @@ PlatformBdsInit ( { //Signal EndofDxe Event EfiEventGroupSignal(&gEfiEndOfDxeEventGroupGuid); + + // restore BootOrder variable if necessary. + RestoreBootOrder();Why restore both here and on ReadyToBoot?} @@ -473,6 +865,7 @@ PlatformBdsPolicyBehavior ( Print (L"Press Enter to boot OS immediately.\n"); Print (L"Press any other key in %d seconds to stop automatical booting...\n", PcdGet16(PcdPlatformBootTimeOut)); PlatformBdsEnterFrontPage (PcdGet16(PcdPlatformBootTimeOut), TRUE); + DealBmcBootType(); } /** diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf index baceb57..1b58c3b 100644 --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf @@ -42,6 +42,7 @@ IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec MdeModulePkg/MdeModulePkg.dec MdePkg/MdePkg.dec + OpenPlatformPkg/Chips/Hisilicon/HisiPkg.dec [LibraryClasses] BaseLib @@ -49,6 +50,7 @@ DebugLib DevicePathLib GenericBdsLib + IpmiCmdLib MemoryAllocationLib PcdLib PrintLib @@ -71,6 +73,7 @@ gEfiFileInfoGuid gEfiFileSystemInfoGuid gEfiFileSystemVolumeLabelInfoIdGuid + gEfiEventReadyToBootGuidPlease insert alphabetically sorted.[Protocols] gEfiDevicePathProtocolGuid @@ -78,3 +81,4 @@ gEfiLoadedImageProtocolGuid gEfiPciRootBridgeIoProtocolGuid gEfiSimpleFileSystemProtocolGuid + gEfiDevicePathToTextProtocolGuidPlease insert alphabetically sorted. / Leif-- 1.9.1