Re: [Linaro-uefi] [PATCH 1/3] Hisilicon D03/D05: set boot option by BMC

18 May 2017

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

    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

Re: [Linaro-uefi] [PATCH 1/3] Hisilicon D03/D05: set boot option by BMC