Hi Leif,



在 2017/5/17 1:37, Leif Lindholm 写道:
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();

 The other commends are modified.

I will send out the v2 patch soon, please review it, thank you.

Regards,

Chenhui Sun


+
+  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
+  gEfiEventReadyToBootGuid
Please insert alphabetically sorted.

 
 [Protocols]
   gEfiDevicePathProtocolGuid
@@ -78,3 +81,4 @@
   gEfiLoadedImageProtocolGuid
   gEfiPciRootBridgeIoProtocolGuid
   gEfiSimpleFileSystemProtocolGuid
+  gEfiDevicePathToTextProtocolGuid
Please insert alphabetically sorted.

/
    Leif

-- 
1.9.1