diff mbox series

[Linaro-uefi,Linaro-uefi,v1,21/21] Hisilicon D03/D05: get boot option from BMC

Message ID 1490015485-53685-22-git-send-email-chenhui.sun@linaro.org
State New
Headers show
Series D02/D03 platforms bug fix | expand

Commit Message

Chenhui Sun March 20, 2017, 1:11 p.m. UTC 
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: Chenhui Sun <sunchenhui@huawei.com>
---
 .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 310 +++++++++++++++++++++
 .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf    |   2 +
 Platforms/Hisilicon/D03/D03.dsc                    |   1 -
 Platforms/Hisilicon/D05/D05.dsc                    |   1 -
 4 files changed, 312 insertions(+), 2 deletions(-)

Comments

Leif Lindholm March 21, 2017, 6:59 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 09:11:25PM +0800, Chenhui Sun wrote:
> Support the feature that BIOS get boot option from BMC and
> put it in the first boot order.

So first of all - I am really happy to see this support. It will be a
huge improvement for validation.

But I will mention that we now have a common Bds, and most other
platforms have migrated away from the IntelBds. It would be very good
if the Hisilicon enterprise platforms could also migrate to
MdeModulePkg/Universal/BdsDxe/.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: huangming <huangming23@huawei.com>
> Signed-off-by: Chenhui Sun <sunchenhui@huawei.com>
> ---
>  .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 310 +++++++++++++++++++++
>  .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf    |   2 +
>  Platforms/Hisilicon/D03/D03.dsc                    |   1 -
>  Platforms/Hisilicon/D05/D05.dsc                    |   1 -
>  4 files changed, 312 insertions(+), 2 deletions(-)
> 
> diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> index efefeb6..7bba2f4 100644
> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
> @@ -19,18 +19,108 @@
>  
>  **/
>  
> +#include <Guid/GlobalVariable.h>
>  #include <IndustryStandard/Pci22.h>
>  #include <Library/DevicePathLib.h>
> +#include <Library/GenericBdsLib.h>
> +#include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/PlatformBdsLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/DevicePath.h>
> +#include <Protocol/DevicePathToText.h>
>  #include <Protocol/GraphicsOutput.h>
>  #include <Protocol/PciIo.h>
>  #include <Protocol/PciRootBridgeIo.h>
>  
>  #include "IntelBdsPlatform.h"
>  
> +#define BOOT_OPTION_BOOT_FLAG_VALID         1
> +#define BOOT_OPTION_BOOT_FLAG_INVALID         0

Please align the above.

> +
> +typedef enum {
> +  NoOverride = 0x0,
> +  ForcePxe,
> +  ForceDefaultHardDisk,
> +  ForceDefaultHardDiskSafeMode,
> +  ForceDefaultDiagnosticPartition,
> +  ForceDefaultCD,
> +  ForceSetupUtility,
> +  ForceRemoteRemovableMedia,
> +  ForceRemoteCD,
> +  ForcePrimaryRemoteMedia,
> +  ForceRemoteHardDisk = 0xB,
> +  ForcePrimaryRemovableMedia = 0xF
> +} BOOT_DEVICE_SELECTOR;

Please move these #defines, enums and structs into a .h file in the
same directory.

> +
> +// 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;
> +
> +#define EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID \
> +  { \
> +    0xA0441D0, 0x0, 0x0, {0x1, 0x1, 0x06, 0x0, 0x0, 0x0, 0x1, 0x1 } \

This does not look like a GUID generated properly?

> +  }
> +
> +typedef struct{
> +    UINT8 NodeType;
> +    UINT8 NodeSubType;
> +    EFI_GUID *Guid;
> +    UINTN DeviceType;
> +}OemDeviceType;
> +
> +OemDeviceType DeviceTypeArray[]={
> +    {MESSAGING_DEVICE_PATH, MSG_SATA_DP,     NULL, BBS_TYPE_HARDDRIVE},
> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID), BBS_TYPE_HARDDRIVE},
> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS), BBS_TYPE_HARDDRIVE},
> +    {MESSAGING_DEVICE_PATH, MSG_USB_DP,      NULL, BBS_TYPE_USB},
> +    {MESSAGING_DEVICE_PATH, MSG_MAC_ADDR_DP, NULL, BBS_TYPE_EMBEDDED_NETWORK},
> +    {MEDIA_DEVICE_PATH, MEDIA_CDROM_DP,      NULL, BBS_TYPE_CDROM}
> +};
> +
> +EFI_STATUS IpmiCmdGetSysBootOptions(OUT IPMI_GET_BOOT_OPTION *BootOption  );
> +EFI_STATUS IpmiCmdSetSysBootOptions(IPMI_GET_BOOT_OPTION    *BootOption  );

No, these prototypes need to be added to some kind of exported header
for IpmiCmdLib. Hmm, I noticed there is another inline declaration in
SmbiosMiscDxe, of IpmiGetChassisType.

But really, all of the exported symbols from
Chips/Hisilicon/Binary/Hi1610/Library/IpmiCmdLib/IpmiCmdLib.lib need
to be declared in a single header file.

>  //3CEF354A-3B7A-4519-AD70-72A134698311
>  GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70,
> @@ -361,6 +451,223 @@ AddOutput (
>      ReportText));
>  }
>  
> +UINT16 DeviceTypeFoundInFileSystemHandles (EFI_DEVICE_PATH_PROTOCOL *DevicePathIn)

STATIC
UINT16
DeviceTypeFoundInFileSystemHandles (
  EFI_DEVICE_PATH_PROTOCOL *DevicePathIn
  )

Also, add IN OUT indicators as appropriate.

> +{
> +  EFI_STATUS                    Status;
> +  EFI_HANDLE                    *FileSystemHandles;
> +  UINTN                         NumberFileSystemHandles;
> +  UINTN                         Index;
> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathGot;
> +  EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;
> +  CHAR16*                       DevicePathTxtIn = NULL;
> +  CHAR16*                       DevicePathTxtGot = NULL;

* goes with the variable name, not with the type.

> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathNode = NULL;
> +  UINT16                        Result = BBS_TYPE_UNKNOWN;
> +
> +  Status = gBS->LocateHandleBuffer (
> +            ByProtocol,

Indentation is supposed to be 2 spaces from function name,
in this case:

  Status = gBS->LocateHandleBuffer (
                  ByProtocol,

> +            &gEfiSimpleFileSystemProtocolGuid,
> +            NULL,
> +            &NumberFileSystemHandles,
> +            &FileSystemHandles
> +            );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
> +    return BBS_TYPE_UNKNOWN;
> +  }
> +
> +  Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL, (VOID **)&DevicePathToTextProtocol);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
> +    return BBS_TYPE_UNKNOWN;
> +  }
> +  DevicePathTxtIn = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathIn, TRUE, TRUE);
> +
> +  for (Index = 0; Index < NumberFileSystemHandles; Index++) {
> +    DevicePathGot = DevicePathFromHandle (FileSystemHandles[Index]);
> +    DevicePathTxtGot = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathGot, TRUE, TRUE);
> +
> +    if (StrnCmp(DevicePathTxtIn, DevicePathTxtGot, StrLen(DevicePathTxtIn)) == 0) {

Spaces after function names.

> +      DevicePathNode = DevicePathGot;
> +      while (!IsDevicePathEnd (DevicePathNode)) {
> +        if ((DevicePathNode->Type == MEDIA_DEVICE_PATH) && (DevicePathNode->SubType == MEDIA_CDROM_DP)) {
> +          Result = BBS_TYPE_CDROM;
> +          break;
> +        }
> +        DevicePathNode = NextDevicePathNode (DevicePathNode);
> +      }
> +    }
> +
> +    if (Result != BBS_TYPE_UNKNOWN) {
> +      break;
> +    }
> +  }
> +
> +  if (NumberFileSystemHandles != 0) {
> +    FreePool (FileSystemHandles);
> +  }
> +  if (DevicePathTxtGot != NULL) {
> +    FreePool (DevicePathTxtGot);
> +  }
> +  if (DevicePathTxtIn != NULL) {
> +    FreePool (DevicePathTxtIn);
> +  }
> +
> +  return Result;
> +}
> +
> +UINT16 UniGetEfiDeviceType(
> +  IN  BDS_COMMON_OPTION *BootOption
> +)

STATIC
UINT16
UniGetEfiDeviceType (
  IN  BDS_COMMON_OPTION *BootOption
  )

> +{
> +  EFI_DEVICE_PATH_PROTOCOL*  DevicePathNode;
> +  UINTN DeviceCnt;
> +  UINTN Loop;
> +  VENDOR_DEVICE_PATH *Vender;

"Vender" -> "Vendor".

> +  UINT16 Result;
> +
> +  DeviceCnt = sizeof (DeviceTypeArray) / sizeof (OemDeviceType);
> +  DevicePathNode = BootOption->DevicePath;
> +  while (!IsDevicePathEnd (DevicePathNode)) {
> +    for (Loop = 0; Loop < DeviceCnt; Loop++) {
> +      if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
> +         (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {

Continuation of test should align with start:

      if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
          (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {

> +        Vender = (VENDOR_DEVICE_PATH*)(DevicePathNode);
> +        if (CompareMem(&(Vender->Guid), DeviceTypeArray[Loop].Guid, sizeof(EFI_GUID)) == 0) {

Use CompareGuid () instead?
(And space after function name.)

> +            return DeviceTypeArray[Loop].DeviceType;
> +        }
> +      } else if ((DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH) &&
> +        (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {

      } else if ((DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH) &&
                 (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {

> +        Result = DeviceTypeFoundInFileSystemHandles (BootOption->DevicePath);
> +        if (Result != BBS_TYPE_UNKNOWN) {
> +          return Result;
> +        }
> +      } else if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
> +        (DevicePathSubType (DevicePathNode) == DeviceTypeArray[Loop].NodeSubType)) {

      } else if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
                 (DevicePathSubType (DevicePathNode) == DeviceTypeArray[Loop].NodeSubType)) {

> +        return DeviceTypeArray[Loop].DeviceType;
> +      }
> +    }
> +
> +    DevicePathNode = NextDevicePathNode (DevicePathNode);
> +  }
> +
> +  return BBS_TYPE_UNKNOWN;
> +}
> +
> +EFI_STATUS GetBmcBootOptionsSetting (IPMI_GET_BOOT_OPTION *BmcBootOpt)

STATIC
EFI_STATUS
GetBmcBootOptionsSetting (
  IPMI_GET_BOOT_OPTION *BmcBootOpt
  )

And add IN OUT indicators.

> +{
> +  EFI_STATUS   Status = EFI_SUCCESS;
> +
> +  Status = IpmiCmdGetSysBootOptions (BmcBootOpt);
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a - %d  Get iBMC BootOpts %r!\n", __FUNCTION__, __LINE__,Status));
> +    return Status;
> +  }
> +
> +  if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) {
> +    DEBUG ((DEBUG_ERROR, "%a - %d  BootFlags is Invalid !\n", __FUNCTION__, __LINE__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  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, "%a - %d  Set iBMC BootOpts %r!\n", __FUNCTION__, __LINE__, Status));
> +  }
> +
> +  return Status;
> +}
> +
> +VOID ProductBdsPolicyAfterSetup ( VOID )

STATIC
VOID
ProductBdsPolicyAfterSetup (
  VOID
  )

> +{
> +  EFI_STATUS                Status = EFI_SUCCESS;
> +  IPMI_GET_BOOT_OPTION      BmcBootOpt;
> +  UINT16                    *OptionOrder;
> +  UINTN                     OptionOrderSize;
> +  UINTN                     DeviceType = BBS_TYPE_UNKNOWN;
> +  UINTN                     Index;
> +  BDS_COMMON_OPTION         *Option;
> +  CHAR16                    OptionName[20];

Why 20?

> +  LIST_ENTRY                BootOptionList;
> +  UINT16                    BootIdx;
> +  UINT16                    *BootNextBuf;
> +
> +  InitializeListHead (&BootOptionList);
> +
> +  Status = GetBmcBootOptionsSetting (&BmcBootOpt);
> +  if (EFI_ERROR(Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a - %d : %r!\n", __FUNCTION__, __LINE__,Status));
> +    return;
> +  }
> +
> +  if (BmcBootOpt.BootDeviceSelector == ForcePrimaryRemovableMedia) {
> +    DeviceType = BBS_TYPE_USB;
> +  } else if (BmcBootOpt.BootDeviceSelector == ForcePxe) {
> +    DeviceType = BBS_TYPE_EMBEDDED_NETWORK;
> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultHardDisk) {
> +    DeviceType = BBS_TYPE_HARDDRIVE;
> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultCD) {
> +    DeviceType = BBS_TYPE_CDROM;
> +  } else {
> +    return;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "BMC set BootType=%x\n", DeviceType));
> +
> +  OptionOrder = BdsLibGetVariableAndSize (
> +                  L"BootOrder",
> +                  &gEfiGlobalVariableGuid,
> +                  &OptionOrderSize
> +                  );
> +  if (OptionOrder == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a - %d error\n", __FUNCTION__, __LINE__));
> +    return;
> +  }
> +
> +  BootIdx = 0;
> +  BootNextBuf = (UINT16*)AllocatePool(OptionOrderSize);

Space after function name.

> +  if (BootNextBuf == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Out of resources.\n"));
> +    return;
> +  }
> +
> +  for (Index = 0; Index < OptionOrderSize / sizeof (UINT16); Index++) {
> +    UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x", OptionOrder[Index]);
> +    Option = BdsLibVariableToOption (&BootOptionList, OptionName);
> +    if (Option == NULL) {
> +      DEBUG((DEBUG_ERROR, "%a - %d  Boot%04x is Null!\n", __FUNCTION__, __LINE__, OptionOrder[Index]));
> +      continue;
> +    }
> +
> +    if (DeviceType == UniGetEfiDeviceType(Option)) {

Space after function name.

> +      BootNextBuf[BootIdx] = OptionOrder[Index];
> +      BootIdx++;
> +    }
> +    RemoveEntryList (&Option->Link);
> +    FreePool (Option);
> +  }
> +
> +  if (BootIdx > 0) {
> +    Status = gRT->SetVariable (
> +      L"BootNext",
> +      &gEfiGlobalVariableGuid,
> +      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,

I think a temporary variable would make this more readable.

> +      BootIdx*sizeof(UINT16),

Spaces around "*".
Space after function name. (OK, I know sizeof isn't technically a
function, but...)

> +      BootNextBuf
> +      );
> +    DEBUG ((DEBUG_ERROR, "Set BootNext %r, size=%x\n", Status, BootIdx*sizeof(UINT16)));
> +  }
> +
> +  FreePool (OptionOrder);
> +  FreePool (BootNextBuf);
> +
> +  return;
> +}
>  
>  /**
>    The function will execute with as the platform policy, current policy
> @@ -469,6 +776,9 @@ PlatformBdsPolicyBehavior (
>    //
>    BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder");
>  
> +  //get boot option from BMC
> +  ProductBdsPolicyAfterSetup();
> +
>    //PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE);
>    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));
> diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
> index baceb57..a09683d 100644
> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
> @@ -49,6 +49,7 @@
>    DebugLib
>    DevicePathLib
>    GenericBdsLib
> +  IpmiCmdLib
>    MemoryAllocationLib
>    PcdLib
>    PrintLib
> @@ -78,3 +79,4 @@
>    gEfiLoadedImageProtocolGuid
>    gEfiPciRootBridgeIoProtocolGuid
>    gEfiSimpleFileSystemProtocolGuid
> +  gEfiDevicePathToTextProtocolGuid
> diff --git a/Platforms/Hisilicon/D03/D03.dsc b/Platforms/Hisilicon/D03/D03.dsc
> index 24c88a3..05dd5d8 100644
> --- a/Platforms/Hisilicon/D03/D03.dsc
> +++ b/Platforms/Hisilicon/D03/D03.dsc
> @@ -367,7 +367,6 @@
>    OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>      <LibraryClasses>
> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf

This deletion....

>        BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    }
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> diff --git a/Platforms/Hisilicon/D05/D05.dsc b/Platforms/Hisilicon/D05/D05.dsc
> index 9de1be6..efdedfd 100644
> --- a/Platforms/Hisilicon/D05/D05.dsc
> +++ b/Platforms/Hisilicon/D05/D05.dsc
> @@ -487,7 +487,6 @@
>    OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>    MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>      <LibraryClasses>
> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf

... and this one.
It is not clear to me how this is related to the overall changeset of
this patch. Is it separate cleanup that should be a separate patch?

Regards,

Leif

>        BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>    }
>    MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
> -- 
> 1.9.1
>
Chenhui Sun March 28, 2017, 12:17 p.m. UTC | #2 
Hi Leif,


在 2017/3/22 2:59, Leif Lindholm 写道:
> On Mon, Mar 20, 2017 at 09:11:25PM +0800, Chenhui Sun wrote:
>> Support the feature that BIOS get boot option from BMC and
>> put it in the first boot order.
> So first of all - I am really happy to see this support. It will be a
> huge improvement for validation.
>
> But I will mention that we now have a common Bds, and most other
> platforms have migrated away from the IntelBds. It would be very good
> if the Hisilicon enterprise platforms could also migrate to
> MdeModulePkg/Universal/BdsDxe/.
Yes, we tried to switch MdeModulePkg/Universal/BdsDxe in ERP16.12, but 
there were two issues at that time,
we plan to switch MdeModulePkg Bds after fix that issue.
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: huangming <huangming23@huawei.com>
>> Signed-off-by: Chenhui Sun <sunchenhui@huawei.com>
>> ---
>>   .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 310 +++++++++++++++++++++
>>   .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf    |   2 +
>>   Platforms/Hisilicon/D03/D03.dsc                    |   1 -
>>   Platforms/Hisilicon/D05/D05.dsc                    |   1 -
>>   4 files changed, 312 insertions(+), 2 deletions(-)
>>
>> diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> index efefeb6..7bba2f4 100644
>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>> @@ -19,18 +19,108 @@
>>   
>>   **/
>>   
>> +#include <Guid/GlobalVariable.h>
>>   #include <IndustryStandard/Pci22.h>
>>   #include <Library/DevicePathLib.h>
>> +#include <Library/GenericBdsLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>>   #include <Library/PcdLib.h>
>>   #include <Library/PlatformBdsLib.h>
>> +#include <Library/PrintLib.h>
>>   #include <Library/UefiLib.h>
>>   #include <Protocol/DevicePath.h>
>> +#include <Protocol/DevicePathToText.h>
>>   #include <Protocol/GraphicsOutput.h>
>>   #include <Protocol/PciIo.h>
>>   #include <Protocol/PciRootBridgeIo.h>
>>   
>>   #include "IntelBdsPlatform.h"
>>   
>> +#define BOOT_OPTION_BOOT_FLAG_VALID         1
>> +#define BOOT_OPTION_BOOT_FLAG_INVALID         0
> Please align the above.
OK
>> +
>> +typedef enum {
>> +  NoOverride = 0x0,
>> +  ForcePxe,
>> +  ForceDefaultHardDisk,
>> +  ForceDefaultHardDiskSafeMode,
>> +  ForceDefaultDiagnosticPartition,
>> +  ForceDefaultCD,
>> +  ForceSetupUtility,
>> +  ForceRemoteRemovableMedia,
>> +  ForceRemoteCD,
>> +  ForcePrimaryRemoteMedia,
>> +  ForceRemoteHardDisk = 0xB,
>> +  ForcePrimaryRemovableMedia = 0xF
>> +} BOOT_DEVICE_SELECTOR;
> Please move these #defines, enums and structs into a .h file in the
> same directory.
Ok
>> +
>> +// 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;
>> +
>> +#define EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID \
>> +  { \
>> +    0xA0441D0, 0x0, 0x0, {0x1, 0x1, 0x06, 0x0, 0x0, 0x0, 0x1, 0x1 } \
> This does not look like a GUID generated properly?
we will modify it using the GUID generated tool.
>
>> +  }
>> +
>> +typedef struct{
>> +    UINT8 NodeType;
>> +    UINT8 NodeSubType;
>> +    EFI_GUID *Guid;
>> +    UINTN DeviceType;
>> +}OemDeviceType;
>> +
>> +OemDeviceType DeviceTypeArray[]={
>> +    {MESSAGING_DEVICE_PATH, MSG_SATA_DP,     NULL, BBS_TYPE_HARDDRIVE},
>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID), BBS_TYPE_HARDDRIVE},
>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS), BBS_TYPE_HARDDRIVE},
>> +    {MESSAGING_DEVICE_PATH, MSG_USB_DP,      NULL, BBS_TYPE_USB},
>> +    {MESSAGING_DEVICE_PATH, MSG_MAC_ADDR_DP, NULL, BBS_TYPE_EMBEDDED_NETWORK},
>> +    {MEDIA_DEVICE_PATH, MEDIA_CDROM_DP,      NULL, BBS_TYPE_CDROM}
>> +};
>> +
>> +EFI_STATUS IpmiCmdGetSysBootOptions(OUT IPMI_GET_BOOT_OPTION *BootOption  );
>> +EFI_STATUS IpmiCmdSetSysBootOptions(IPMI_GET_BOOT_OPTION    *BootOption  );
> No, these prototypes need to be added to some kind of exported header
> for IpmiCmdLib. Hmm, I noticed there is another inline declaration in
> SmbiosMiscDxe, of IpmiGetChassisType.
>
> But really, all of the exported symbols from
> Chips/Hisilicon/Binary/Hi1610/Library/IpmiCmdLib/IpmiCmdLib.lib need
> to be declared in a single header file.
ok
>>   //3CEF354A-3B7A-4519-AD70-72A134698311
>>   GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70,
>> @@ -361,6 +451,223 @@ AddOutput (
>>       ReportText));
>>   }
>>   
>> +UINT16 DeviceTypeFoundInFileSystemHandles (EFI_DEVICE_PATH_PROTOCOL *DevicePathIn)
> STATIC
> UINT16
> DeviceTypeFoundInFileSystemHandles (
>    EFI_DEVICE_PATH_PROTOCOL *DevicePathIn
>    )
>
> Also, add IN OUT indicators as appropriate.
ok
>> +{
>> +  EFI_STATUS                    Status;
>> +  EFI_HANDLE                    *FileSystemHandles;
>> +  UINTN                         NumberFileSystemHandles;
>> +  UINTN                         Index;
>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathGot;
>> +  EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;
>> +  CHAR16*                       DevicePathTxtIn = NULL;
>> +  CHAR16*                       DevicePathTxtGot = NULL;
> * goes with the variable name, not with the type.
>
>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathNode = NULL;
>> +  UINT16                        Result = BBS_TYPE_UNKNOWN;
>> +
>> +  Status = gBS->LocateHandleBuffer (
>> +            ByProtocol,
> Indentation is supposed to be 2 spaces from function name,
> in this case:
>
>    Status = gBS->LocateHandleBuffer (
>                    ByProtocol,
ok
>> +            &gEfiSimpleFileSystemProtocolGuid,
>> +            NULL,
>> +            &NumberFileSystemHandles,
>> +            &FileSystemHandles
>> +            );
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
>> +    return BBS_TYPE_UNKNOWN;
>> +  }
>> +
>> +  Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL, (VOID **)&DevicePathToTextProtocol);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
>> +    return BBS_TYPE_UNKNOWN;
>> +  }
>> +  DevicePathTxtIn = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathIn, TRUE, TRUE);
>> +
>> +  for (Index = 0; Index < NumberFileSystemHandles; Index++) {
>> +    DevicePathGot = DevicePathFromHandle (FileSystemHandles[Index]);
>> +    DevicePathTxtGot = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathGot, TRUE, TRUE);
>> +
>> +    if (StrnCmp(DevicePathTxtIn, DevicePathTxtGot, StrLen(DevicePathTxtIn)) == 0) {
> Spaces after function names.
will be modified
>> +      DevicePathNode = DevicePathGot;
>> +      while (!IsDevicePathEnd (DevicePathNode)) {
>> +        if ((DevicePathNode->Type == MEDIA_DEVICE_PATH) && (DevicePathNode->SubType == MEDIA_CDROM_DP)) {
>> +          Result = BBS_TYPE_CDROM;
>> +          break;
>> +        }
>> +        DevicePathNode = NextDevicePathNode (DevicePathNode);
>> +      }
>> +    }
>> +
>> +    if (Result != BBS_TYPE_UNKNOWN) {
>> +      break;
>> +    }
>> +  }
>> +
>> +  if (NumberFileSystemHandles != 0) {
>> +    FreePool (FileSystemHandles);
>> +  }
>> +  if (DevicePathTxtGot != NULL) {
>> +    FreePool (DevicePathTxtGot);
>> +  }
>> +  if (DevicePathTxtIn != NULL) {
>> +    FreePool (DevicePathTxtIn);
>> +  }
>> +
>> +  return Result;
>> +}
>> +
>> +UINT16 UniGetEfiDeviceType(
>> +  IN  BDS_COMMON_OPTION *BootOption
>> +)
> STATIC
> UINT16
> UniGetEfiDeviceType (
>    IN  BDS_COMMON_OPTION *BootOption
>    )
will be modified.
>> +{
>> +  EFI_DEVICE_PATH_PROTOCOL*  DevicePathNode;
>> +  UINTN DeviceCnt;
>> +  UINTN Loop;
>> +  VENDOR_DEVICE_PATH *Vender;
> "Vender" -> "Vendor".
Thanks for pointing this.
>> +  UINT16 Result;
>> +
>> +  DeviceCnt = sizeof (DeviceTypeArray) / sizeof (OemDeviceType);
>> +  DevicePathNode = BootOption->DevicePath;
>> +  while (!IsDevicePathEnd (DevicePathNode)) {
>> +    for (Loop = 0; Loop < DeviceCnt; Loop++) {
>> +      if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
>> +         (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {
> Continuation of test should align with start:
>
>        if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
>            (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {
ok
>> +        Vender = (VENDOR_DEVICE_PATH*)(DevicePathNode);
>> +        if (CompareMem(&(Vender->Guid), DeviceTypeArray[Loop].Guid, sizeof(EFI_GUID)) == 0) {
> Use CompareGuid () instead?
> (And space after function name.)
ok
>> +            return DeviceTypeArray[Loop].DeviceType;
>> +        }
>> +      } else if ((DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH) &&
>> +        (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {
>        } else if ((DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH) &&
>                   (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {
ok
>> +        Result = DeviceTypeFoundInFileSystemHandles (BootOption->DevicePath);
>> +        if (Result != BBS_TYPE_UNKNOWN) {
>> +          return Result;
>> +        }
>> +      } else if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
>> +        (DevicePathSubType (DevicePathNode) == DeviceTypeArray[Loop].NodeSubType)) {
>        } else if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
>                   (DevicePathSubType (DevicePathNode) == DeviceTypeArray[Loop].NodeSubType)) {
will be modified.
>> +        return DeviceTypeArray[Loop].DeviceType;
>> +      }
>> +    }
>> +
>> +    DevicePathNode = NextDevicePathNode (DevicePathNode);
>> +  }
>> +
>> +  return BBS_TYPE_UNKNOWN;
>> +}
>> +
>> +EFI_STATUS GetBmcBootOptionsSetting (IPMI_GET_BOOT_OPTION *BmcBootOpt)
> STATIC
> EFI_STATUS
> GetBmcBootOptionsSetting (
>    IPMI_GET_BOOT_OPTION *BmcBootOpt
>    )
>
> And add IN OUT indicators.
ok
>> +{
>> +  EFI_STATUS   Status = EFI_SUCCESS;
>> +
>> +  Status = IpmiCmdGetSysBootOptions (BmcBootOpt);
>> +  if (EFI_ERROR(Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a - %d  Get iBMC BootOpts %r!\n", __FUNCTION__, __LINE__,Status));
>> +    return Status;
>> +  }
>> +
>> +  if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) {
>> +    DEBUG ((DEBUG_ERROR, "%a - %d  BootFlags is Invalid !\n", __FUNCTION__, __LINE__));
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  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, "%a - %d  Set iBMC BootOpts %r!\n", __FUNCTION__, __LINE__, Status));
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> +VOID ProductBdsPolicyAfterSetup ( VOID )
> STATIC
> VOID
> ProductBdsPolicyAfterSetup (
>    VOID
>    )
>
>> +{
>> +  EFI_STATUS                Status = EFI_SUCCESS;
>> +  IPMI_GET_BOOT_OPTION      BmcBootOpt;
>> +  UINT16                    *OptionOrder;
>> +  UINTN                     OptionOrderSize;
>> +  UINTN                     DeviceType = BBS_TYPE_UNKNOWN;
>> +  UINTN                     Index;
>> +  BDS_COMMON_OPTION         *Option;
>> +  CHAR16                    OptionName[20];
> Why 20?
um..we think the length 20 is enough.
>> +  LIST_ENTRY                BootOptionList;
>> +  UINT16                    BootIdx;
>> +  UINT16                    *BootNextBuf;
>> +
>> +  InitializeListHead (&BootOptionList);
>> +
>> +  Status = GetBmcBootOptionsSetting (&BmcBootOpt);
>> +  if (EFI_ERROR(Status)) {
>> +    DEBUG ((DEBUG_ERROR, "%a - %d : %r!\n", __FUNCTION__, __LINE__,Status));
>> +    return;
>> +  }
>> +
>> +  if (BmcBootOpt.BootDeviceSelector == ForcePrimaryRemovableMedia) {
>> +    DeviceType = BBS_TYPE_USB;
>> +  } else if (BmcBootOpt.BootDeviceSelector == ForcePxe) {
>> +    DeviceType = BBS_TYPE_EMBEDDED_NETWORK;
>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultHardDisk) {
>> +    DeviceType = BBS_TYPE_HARDDRIVE;
>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultCD) {
>> +    DeviceType = BBS_TYPE_CDROM;
>> +  } else {
>> +    return;
>> +  }
>> +
>> +  DEBUG ((DEBUG_ERROR, "BMC set BootType=%x\n", DeviceType));
>> +
>> +  OptionOrder = BdsLibGetVariableAndSize (
>> +                  L"BootOrder",
>> +                  &gEfiGlobalVariableGuid,
>> +                  &OptionOrderSize
>> +                  );
>> +  if (OptionOrder == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "%a - %d error\n", __FUNCTION__, __LINE__));
>> +    return;
>> +  }
>> +
>> +  BootIdx = 0;
>> +  BootNextBuf = (UINT16*)AllocatePool(OptionOrderSize);
> Space after function name.
ok
>
>> +  if (BootNextBuf == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "Out of resources.\n"));
>> +    return;
>> +  }
>> +
>> +  for (Index = 0; Index < OptionOrderSize / sizeof (UINT16); Index++) {
>> +    UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x", OptionOrder[Index]);
>> +    Option = BdsLibVariableToOption (&BootOptionList, OptionName);
>> +    if (Option == NULL) {
>> +      DEBUG((DEBUG_ERROR, "%a - %d  Boot%04x is Null!\n", __FUNCTION__, __LINE__, OptionOrder[Index]));
>> +      continue;
>> +    }
>> +
>> +    if (DeviceType == UniGetEfiDeviceType(Option)) {
> Space after function name.
ok
>> +      BootNextBuf[BootIdx] = OptionOrder[Index];
>> +      BootIdx++;
>> +    }
>> +    RemoveEntryList (&Option->Link);
>> +    FreePool (Option);
>> +  }
>> +
>> +  if (BootIdx > 0) {
>> +    Status = gRT->SetVariable (
>> +      L"BootNext",
>> +      &gEfiGlobalVariableGuid,
>> +      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
> I think a temporary variable would make this more readable.
ok
>> +      BootIdx*sizeof(UINT16),
> Spaces around "*".
> Space after function name. (OK, I know sizeof isn't technically a
> function, but...)
ok, will be modified.
>> +      BootNextBuf
>> +      );
>> +    DEBUG ((DEBUG_ERROR, "Set BootNext %r, size=%x\n", Status, BootIdx*sizeof(UINT16)));
>> +  }
>> +
>> +  FreePool (OptionOrder);
>> +  FreePool (BootNextBuf);
>> +
>> +  return;
>> +}
>>   
>>   /**
>>     The function will execute with as the platform policy, current policy
>> @@ -469,6 +776,9 @@ PlatformBdsPolicyBehavior (
>>     //
>>     BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder");
>>   
>> +  //get boot option from BMC
>> +  ProductBdsPolicyAfterSetup();
>> +
>>     //PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE);
>>     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));
>> diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> index baceb57..a09683d 100644
>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>> @@ -49,6 +49,7 @@
>>     DebugLib
>>     DevicePathLib
>>     GenericBdsLib
>> +  IpmiCmdLib
>>     MemoryAllocationLib
>>     PcdLib
>>     PrintLib
>> @@ -78,3 +79,4 @@
>>     gEfiLoadedImageProtocolGuid
>>     gEfiPciRootBridgeIoProtocolGuid
>>     gEfiSimpleFileSystemProtocolGuid
>> +  gEfiDevicePathToTextProtocolGuid
>> diff --git a/Platforms/Hisilicon/D03/D03.dsc b/Platforms/Hisilicon/D03/D03.dsc
>> index 24c88a3..05dd5d8 100644
>> --- a/Platforms/Hisilicon/D03/D03.dsc
>> +++ b/Platforms/Hisilicon/D03/D03.dsc
>> @@ -367,7 +367,6 @@
>>     OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>>     MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>       <LibraryClasses>
>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> This deletion....
>
>>         BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>     }
>>     MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>> diff --git a/Platforms/Hisilicon/D05/D05.dsc b/Platforms/Hisilicon/D05/D05.dsc
>> index 9de1be6..efdedfd 100644
>> --- a/Platforms/Hisilicon/D05/D05.dsc
>> +++ b/Platforms/Hisilicon/D05/D05.dsc
>> @@ -487,7 +487,6 @@
>>     OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>>     MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>       <LibraryClasses>
>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> ... and this one.
> It is not clear to me how this is related to the overall changeset of
> this patch. Is it separate cleanup that should be a separate patch?

The BootNext will be limited to sizeof(UINT16) in VarCheckUefiLibNullClass.c,
but in the case of 4 network ports , the BootNext should be like 0x01020304 by the UniBootNextVariableUpdate function.

Thanks and Regards,
Chenhui

> Regards,
>
> Leif
>
>>         BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>     }
>>     MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>> -- 
>> 1.9.1
>>
Graeme Gregory April 20, 2017, 8:56 a.m. UTC | #3 
Hi Chenhui,

Was there a reason this patch got dropped for inclusion?

Thanks

Graeme

On 28 March 2017 at 13:17, Chenhui Sun <chenhui.sun@linaro.org> wrote:
> Hi Leif,
>
>
> 在 2017/3/22 2:59, Leif Lindholm 写道:
>>
>> On Mon, Mar 20, 2017 at 09:11:25PM +0800, Chenhui Sun wrote:
>>>
>>> Support the feature that BIOS get boot option from BMC and
>>> put it in the first boot order.
>>
>> So first of all - I am really happy to see this support. It will be a
>> huge improvement for validation.
>>
>> But I will mention that we now have a common Bds, and most other
>> platforms have migrated away from the IntelBds. It would be very good
>> if the Hisilicon enterprise platforms could also migrate to
>> MdeModulePkg/Universal/BdsDxe/.
>
> Yes, we tried to switch MdeModulePkg/Universal/BdsDxe in ERP16.12, but there
> were two issues at that time,
> we plan to switch MdeModulePkg Bds after fix that issue.
>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: huangming <huangming23@huawei.com>
>>> Signed-off-by: Chenhui Sun <sunchenhui@huawei.com>
>>> ---
>>>   .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 310
>>> +++++++++++++++++++++
>>>   .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf    |   2 +
>>>   Platforms/Hisilicon/D03/D03.dsc                    |   1 -
>>>   Platforms/Hisilicon/D05/D05.dsc                    |   1 -
>>>   4 files changed, 312 insertions(+), 2 deletions(-)
>>>
>>> diff --git
>>> a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> index efefeb6..7bba2f4 100644
>>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> @@ -19,18 +19,108 @@
>>>     **/
>>>   +#include <Guid/GlobalVariable.h>
>>>   #include <IndustryStandard/Pci22.h>
>>>   #include <Library/DevicePathLib.h>
>>> +#include <Library/GenericBdsLib.h>
>>> +#include <Library/MemoryAllocationLib.h>
>>>   #include <Library/PcdLib.h>
>>>   #include <Library/PlatformBdsLib.h>
>>> +#include <Library/PrintLib.h>
>>>   #include <Library/UefiLib.h>
>>>   #include <Protocol/DevicePath.h>
>>> +#include <Protocol/DevicePathToText.h>
>>>   #include <Protocol/GraphicsOutput.h>
>>>   #include <Protocol/PciIo.h>
>>>   #include <Protocol/PciRootBridgeIo.h>
>>>     #include "IntelBdsPlatform.h"
>>>   +#define BOOT_OPTION_BOOT_FLAG_VALID         1
>>> +#define BOOT_OPTION_BOOT_FLAG_INVALID         0
>>
>> Please align the above.
>
> OK
>>>
>>> +
>>> +typedef enum {
>>> +  NoOverride = 0x0,
>>> +  ForcePxe,
>>> +  ForceDefaultHardDisk,
>>> +  ForceDefaultHardDiskSafeMode,
>>> +  ForceDefaultDiagnosticPartition,
>>> +  ForceDefaultCD,
>>> +  ForceSetupUtility,
>>> +  ForceRemoteRemovableMedia,
>>> +  ForceRemoteCD,
>>> +  ForcePrimaryRemoteMedia,
>>> +  ForceRemoteHardDisk = 0xB,
>>> +  ForcePrimaryRemovableMedia = 0xF
>>> +} BOOT_DEVICE_SELECTOR;
>>
>> Please move these #defines, enums and structs into a .h file in the
>> same directory.
>
> Ok
>
>>> +
>>> +// 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;
>>> +
>>> +#define EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID \
>>> +  { \
>>> +    0xA0441D0, 0x0, 0x0, {0x1, 0x1, 0x06, 0x0, 0x0, 0x0, 0x1, 0x1 } \
>>
>> This does not look like a GUID generated properly?
>
> we will modify it using the GUID generated tool.
>
>>
>>> +  }
>>> +
>>> +typedef struct{
>>> +    UINT8 NodeType;
>>> +    UINT8 NodeSubType;
>>> +    EFI_GUID *Guid;
>>> +    UINTN DeviceType;
>>> +}OemDeviceType;
>>> +
>>> +OemDeviceType DeviceTypeArray[]={
>>> +    {MESSAGING_DEVICE_PATH, MSG_SATA_DP,     NULL, BBS_TYPE_HARDDRIVE},
>>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
>>> &((EFI_GUID)EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID), BBS_TYPE_HARDDRIVE},
>>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,
>>> &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS), BBS_TYPE_HARDDRIVE},
>>> +    {MESSAGING_DEVICE_PATH, MSG_USB_DP,      NULL, BBS_TYPE_USB},
>>> +    {MESSAGING_DEVICE_PATH, MSG_MAC_ADDR_DP, NULL,
>>> BBS_TYPE_EMBEDDED_NETWORK},
>>> +    {MEDIA_DEVICE_PATH, MEDIA_CDROM_DP,      NULL, BBS_TYPE_CDROM}
>>> +};
>>> +
>>> +EFI_STATUS IpmiCmdGetSysBootOptions(OUT IPMI_GET_BOOT_OPTION *BootOption
>>> );
>>> +EFI_STATUS IpmiCmdSetSysBootOptions(IPMI_GET_BOOT_OPTION    *BootOption
>>> );
>>
>> No, these prototypes need to be added to some kind of exported header
>> for IpmiCmdLib. Hmm, I noticed there is another inline declaration in
>> SmbiosMiscDxe, of IpmiGetChassisType.
>>
>> But really, all of the exported symbols from
>> Chips/Hisilicon/Binary/Hi1610/Library/IpmiCmdLib/IpmiCmdLib.lib need
>> to be declared in a single header file.
>
> ok
>>>
>>>   //3CEF354A-3B7A-4519-AD70-72A134698311
>>>   GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70,
>>> @@ -361,6 +451,223 @@ AddOutput (
>>>       ReportText));
>>>   }
>>>   +UINT16 DeviceTypeFoundInFileSystemHandles (EFI_DEVICE_PATH_PROTOCOL
>>> *DevicePathIn)
>>
>> STATIC
>> UINT16
>> DeviceTypeFoundInFileSystemHandles (
>>    EFI_DEVICE_PATH_PROTOCOL *DevicePathIn
>>    )
>>
>> Also, add IN OUT indicators as appropriate.
>
> ok
>>>
>>> +{
>>> +  EFI_STATUS                    Status;
>>> +  EFI_HANDLE                    *FileSystemHandles;
>>> +  UINTN                         NumberFileSystemHandles;
>>> +  UINTN                         Index;
>>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathGot;
>>> +  EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;
>>> +  CHAR16*                       DevicePathTxtIn = NULL;
>>> +  CHAR16*                       DevicePathTxtGot = NULL;
>>
>> * goes with the variable name, not with the type.
>>
>>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathNode = NULL;
>>> +  UINT16                        Result = BBS_TYPE_UNKNOWN;
>>> +
>>> +  Status = gBS->LocateHandleBuffer (
>>> +            ByProtocol,
>>
>> Indentation is supposed to be 2 spaces from function name,
>> in this case:
>>
>>    Status = gBS->LocateHandleBuffer (
>>                    ByProtocol,
>
> ok
>>>
>>> +            &gEfiSimpleFileSystemProtocolGuid,
>>> +            NULL,
>>> +            &NumberFileSystemHandles,
>>> +            &FileSystemHandles
>>> +            );
>>> +  if (EFI_ERROR (Status)) {
>>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
>>> +    return BBS_TYPE_UNKNOWN;
>>> +  }
>>> +
>>> +  Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL,
>>> (VOID **)&DevicePathToTextProtocol);
>>> +  if (EFI_ERROR (Status)) {
>>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
>>> +    return BBS_TYPE_UNKNOWN;
>>> +  }
>>> +  DevicePathTxtIn = DevicePathToTextProtocol->ConvertDevicePathToText
>>> (DevicePathIn, TRUE, TRUE);
>>> +
>>> +  for (Index = 0; Index < NumberFileSystemHandles; Index++) {
>>> +    DevicePathGot = DevicePathFromHandle (FileSystemHandles[Index]);
>>> +    DevicePathTxtGot = DevicePathToTextProtocol->ConvertDevicePathToText
>>> (DevicePathGot, TRUE, TRUE);
>>> +
>>> +    if (StrnCmp(DevicePathTxtIn, DevicePathTxtGot,
>>> StrLen(DevicePathTxtIn)) == 0) {
>>
>> Spaces after function names.
>
> will be modified
>
>>> +      DevicePathNode = DevicePathGot;
>>> +      while (!IsDevicePathEnd (DevicePathNode)) {
>>> +        if ((DevicePathNode->Type == MEDIA_DEVICE_PATH) &&
>>> (DevicePathNode->SubType == MEDIA_CDROM_DP)) {
>>> +          Result = BBS_TYPE_CDROM;
>>> +          break;
>>> +        }
>>> +        DevicePathNode = NextDevicePathNode (DevicePathNode);
>>> +      }
>>> +    }
>>> +
>>> +    if (Result != BBS_TYPE_UNKNOWN) {
>>> +      break;
>>> +    }
>>> +  }
>>> +
>>> +  if (NumberFileSystemHandles != 0) {
>>> +    FreePool (FileSystemHandles);
>>> +  }
>>> +  if (DevicePathTxtGot != NULL) {
>>> +    FreePool (DevicePathTxtGot);
>>> +  }
>>> +  if (DevicePathTxtIn != NULL) {
>>> +    FreePool (DevicePathTxtIn);
>>> +  }
>>> +
>>> +  return Result;
>>> +}
>>> +
>>> +UINT16 UniGetEfiDeviceType(
>>> +  IN  BDS_COMMON_OPTION *BootOption
>>> +)
>>
>> STATIC
>> UINT16
>> UniGetEfiDeviceType (
>>    IN  BDS_COMMON_OPTION *BootOption
>>    )
>
> will be modified.
>>>
>>> +{
>>> +  EFI_DEVICE_PATH_PROTOCOL*  DevicePathNode;
>>> +  UINTN DeviceCnt;
>>> +  UINTN Loop;
>>> +  VENDOR_DEVICE_PATH *Vender;
>>
>> "Vender" -> "Vendor".
>
> Thanks for pointing this.
>>>
>>> +  UINT16 Result;
>>> +
>>> +  DeviceCnt = sizeof (DeviceTypeArray) / sizeof (OemDeviceType);
>>> +  DevicePathNode = BootOption->DevicePath;
>>> +  while (!IsDevicePathEnd (DevicePathNode)) {
>>> +    for (Loop = 0; Loop < DeviceCnt; Loop++) {
>>> +      if ((DevicePathType (DevicePathNode) ==
>>> DeviceTypeArray[Loop].NodeType) &&
>>> +         (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {
>>
>> Continuation of test should align with start:
>>
>>        if ((DevicePathType (DevicePathNode) ==
>> DeviceTypeArray[Loop].NodeType) &&
>>            (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {
>
> ok
>>>
>>> +        Vender = (VENDOR_DEVICE_PATH*)(DevicePathNode);
>>> +        if (CompareMem(&(Vender->Guid), DeviceTypeArray[Loop].Guid,
>>> sizeof(EFI_GUID)) == 0) {
>>
>> Use CompareGuid () instead?
>> (And space after function name.)
>
> ok
>>>
>>> +            return DeviceTypeArray[Loop].DeviceType;
>>> +        }
>>> +      } else if ((DevicePathType (DevicePathNode) ==
>>> MESSAGING_DEVICE_PATH) &&
>>> +        (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {
>>
>>        } else if ((DevicePathType (DevicePathNode) ==
>> MESSAGING_DEVICE_PATH) &&
>>                   (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {
>
> ok
>>>
>>> +        Result = DeviceTypeFoundInFileSystemHandles
>>> (BootOption->DevicePath);
>>> +        if (Result != BBS_TYPE_UNKNOWN) {
>>> +          return Result;
>>> +        }
>>> +      } else if ((DevicePathType (DevicePathNode) ==
>>> DeviceTypeArray[Loop].NodeType) &&
>>> +        (DevicePathSubType (DevicePathNode) ==
>>> DeviceTypeArray[Loop].NodeSubType)) {
>>
>>        } else if ((DevicePathType (DevicePathNode) ==
>> DeviceTypeArray[Loop].NodeType) &&
>>                   (DevicePathSubType (DevicePathNode) ==
>> DeviceTypeArray[Loop].NodeSubType)) {
>
> will be modified.
>>>
>>> +        return DeviceTypeArray[Loop].DeviceType;
>>> +      }
>>> +    }
>>> +
>>> +    DevicePathNode = NextDevicePathNode (DevicePathNode);
>>> +  }
>>> +
>>> +  return BBS_TYPE_UNKNOWN;
>>> +}
>>> +
>>> +EFI_STATUS GetBmcBootOptionsSetting (IPMI_GET_BOOT_OPTION *BmcBootOpt)
>>
>> STATIC
>> EFI_STATUS
>> GetBmcBootOptionsSetting (
>>    IPMI_GET_BOOT_OPTION *BmcBootOpt
>>    )
>>
>> And add IN OUT indicators.
>
> ok
>
>>> +{
>>> +  EFI_STATUS   Status = EFI_SUCCESS;
>>> +
>>> +  Status = IpmiCmdGetSysBootOptions (BmcBootOpt);
>>> +  if (EFI_ERROR(Status)) {
>>> +    DEBUG ((DEBUG_ERROR, "%a - %d  Get iBMC BootOpts %r!\n",
>>> __FUNCTION__, __LINE__,Status));
>>> +    return Status;
>>> +  }
>>> +
>>> +  if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) {
>>> +    DEBUG ((DEBUG_ERROR, "%a - %d  BootFlags is Invalid !\n",
>>> __FUNCTION__, __LINE__));
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  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, "%a - %d  Set iBMC BootOpts %r!\n",
>>> __FUNCTION__, __LINE__, Status));
>>> +  }
>>> +
>>> +  return Status;
>>> +}
>>> +
>>> +VOID ProductBdsPolicyAfterSetup ( VOID )
>>
>> STATIC
>> VOID
>> ProductBdsPolicyAfterSetup (
>>    VOID
>>    )
>>
>>> +{
>>> +  EFI_STATUS                Status = EFI_SUCCESS;
>>> +  IPMI_GET_BOOT_OPTION      BmcBootOpt;
>>> +  UINT16                    *OptionOrder;
>>> +  UINTN                     OptionOrderSize;
>>> +  UINTN                     DeviceType = BBS_TYPE_UNKNOWN;
>>> +  UINTN                     Index;
>>> +  BDS_COMMON_OPTION         *Option;
>>> +  CHAR16                    OptionName[20];
>>
>> Why 20?
>
> um..we think the length 20 is enough.
>
>>> +  LIST_ENTRY                BootOptionList;
>>> +  UINT16                    BootIdx;
>>> +  UINT16                    *BootNextBuf;
>>> +
>>> +  InitializeListHead (&BootOptionList);
>>> +
>>> +  Status = GetBmcBootOptionsSetting (&BmcBootOpt);
>>> +  if (EFI_ERROR(Status)) {
>>> +    DEBUG ((DEBUG_ERROR, "%a - %d : %r!\n", __FUNCTION__,
>>> __LINE__,Status));
>>> +    return;
>>> +  }
>>> +
>>> +  if (BmcBootOpt.BootDeviceSelector == ForcePrimaryRemovableMedia) {
>>> +    DeviceType = BBS_TYPE_USB;
>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForcePxe) {
>>> +    DeviceType = BBS_TYPE_EMBEDDED_NETWORK;
>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultHardDisk) {
>>> +    DeviceType = BBS_TYPE_HARDDRIVE;
>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultCD) {
>>> +    DeviceType = BBS_TYPE_CDROM;
>>> +  } else {
>>> +    return;
>>> +  }
>>> +
>>> +  DEBUG ((DEBUG_ERROR, "BMC set BootType=%x\n", DeviceType));
>>> +
>>> +  OptionOrder = BdsLibGetVariableAndSize (
>>> +                  L"BootOrder",
>>> +                  &gEfiGlobalVariableGuid,
>>> +                  &OptionOrderSize
>>> +                  );
>>> +  if (OptionOrder == NULL) {
>>> +    DEBUG ((DEBUG_ERROR, "%a - %d error\n", __FUNCTION__, __LINE__));
>>> +    return;
>>> +  }
>>> +
>>> +  BootIdx = 0;
>>> +  BootNextBuf = (UINT16*)AllocatePool(OptionOrderSize);
>>
>> Space after function name.
>
> ok
>>
>>
>>> +  if (BootNextBuf == NULL) {
>>> +    DEBUG ((DEBUG_ERROR, "Out of resources.\n"));
>>> +    return;
>>> +  }
>>> +
>>> +  for (Index = 0; Index < OptionOrderSize / sizeof (UINT16); Index++) {
>>> +    UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x",
>>> OptionOrder[Index]);
>>> +    Option = BdsLibVariableToOption (&BootOptionList, OptionName);
>>> +    if (Option == NULL) {
>>> +      DEBUG((DEBUG_ERROR, "%a - %d  Boot%04x is Null!\n", __FUNCTION__,
>>> __LINE__, OptionOrder[Index]));
>>> +      continue;
>>> +    }
>>> +
>>> +    if (DeviceType == UniGetEfiDeviceType(Option)) {
>>
>> Space after function name.
>
> ok
>>>
>>> +      BootNextBuf[BootIdx] = OptionOrder[Index];
>>> +      BootIdx++;
>>> +    }
>>> +    RemoveEntryList (&Option->Link);
>>> +    FreePool (Option);
>>> +  }
>>> +
>>> +  if (BootIdx > 0) {
>>> +    Status = gRT->SetVariable (
>>> +      L"BootNext",
>>> +      &gEfiGlobalVariableGuid,
>>> +      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS |
>>> EFI_VARIABLE_NON_VOLATILE,
>>
>> I think a temporary variable would make this more readable.
>
> ok
>>>
>>> +      BootIdx*sizeof(UINT16),
>>
>> Spaces around "*".
>> Space after function name. (OK, I know sizeof isn't technically a
>> function, but...)
>
> ok, will be modified.
>
>>> +      BootNextBuf
>>> +      );
>>> +    DEBUG ((DEBUG_ERROR, "Set BootNext %r, size=%x\n", Status,
>>> BootIdx*sizeof(UINT16)));
>>> +  }
>>> +
>>> +  FreePool (OptionOrder);
>>> +  FreePool (BootNextBuf);
>>> +
>>> +  return;
>>> +}
>>>     /**
>>>     The function will execute with as the platform policy, current policy
>>> @@ -469,6 +776,9 @@ PlatformBdsPolicyBehavior (
>>>     //
>>>     BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder");
>>>   +  //get boot option from BMC
>>> +  ProductBdsPolicyAfterSetup();
>>> +
>>>     //PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE);
>>>     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));
>>> diff --git
>>> a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>>> b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>>> index baceb57..a09683d 100644
>>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
>>> @@ -49,6 +49,7 @@
>>>     DebugLib
>>>     DevicePathLib
>>>     GenericBdsLib
>>> +  IpmiCmdLib
>>>     MemoryAllocationLib
>>>     PcdLib
>>>     PrintLib
>>> @@ -78,3 +79,4 @@
>>>     gEfiLoadedImageProtocolGuid
>>>     gEfiPciRootBridgeIoProtocolGuid
>>>     gEfiSimpleFileSystemProtocolGuid
>>> +  gEfiDevicePathToTextProtocolGuid
>>> diff --git a/Platforms/Hisilicon/D03/D03.dsc
>>> b/Platforms/Hisilicon/D03/D03.dsc
>>> index 24c88a3..05dd5d8 100644
>>> --- a/Platforms/Hisilicon/D03/D03.dsc
>>> +++ b/Platforms/Hisilicon/D03/D03.dsc
>>> @@ -367,7 +367,6 @@
>>>     OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>>>     MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>>       <LibraryClasses>
>>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>
>> This deletion....
>>
>>>         BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>>     }
>>>     MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>>> diff --git a/Platforms/Hisilicon/D05/D05.dsc
>>> b/Platforms/Hisilicon/D05/D05.dsc
>>> index 9de1be6..efdedfd 100644
>>> --- a/Platforms/Hisilicon/D05/D05.dsc
>>> +++ b/Platforms/Hisilicon/D05/D05.dsc
>>> @@ -487,7 +487,6 @@
>>>     OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
>>>     MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
>>>       <LibraryClasses>
>>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
>>
>> ... and this one.
>> It is not clear to me how this is related to the overall changeset of
>> this patch. Is it separate cleanup that should be a separate patch?
>
>
> The BootNext will be limited to sizeof(UINT16) in
> VarCheckUefiLibNullClass.c,
> but in the case of 4 network ports , the BootNext should be like 0x01020304
> by the UniBootNextVariableUpdate function.
>
> Thanks and Regards,
> Chenhui
>
>
>> Regards,
>>
>> Leif
>>
>>>         BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
>>>     }
>>>     MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
>>> --
>>> 1.9.1
>>>
>
Ard Biesheuvel April 20, 2017, 9:16 a.m. UTC | #4 
On 20 April 2017 at 09:56, Graeme Gregory <graeme.gregory@linaro.org> wrote:
> Hi Chenhui,
>
> Was there a reason this patch got dropped for inclusion?
>

IIRC it depends on an upstream change that is not entirely
uncontroversial (and I never got a reply to the question why *exactly*
that change was necessary)
Chenhui Sun April 20, 2017, 9:18 a.m. UTC | #5 
Hi Graeme,

There are two related patches to support this function.

The other is "[PATCH] IntelFrameworkModulePkg/BdsEntry: support BMC boot 
option",

In this patch, we override the IntelFrameworkModulePkg, we are not sure 
is this the best solution,

So I just send it out, and like to get more feedback.


Once more, this solution has a limitation which I wrote on the commit 
message:

"And it have a limitation, only set the boot order by type, can't by
the specfic devices.
example: there have 4 ethernet ports at D05 board, it can only be booted
from the ethernet port, but which port can not be defined. so it try from
the first port to the end."
*And we will switch the BDS to MdeModulePkg, we have not investigate this 
problem whether disappear or not after doing that. So I dropped this 
patch first.******Thanks and Regards,****Chenhui *




在 2017/4/20 16:56, Graeme Gregory 写道:
> Hi Chenhui,

>

> Was there a reason this patch got dropped for inclusion?

>

> Thanks

>

> Graeme

>

> On 28 March 2017 at 13:17, Chenhui Sun <chenhui.sun@linaro.org> wrote:

>> Hi Leif,

>>

>>

>> 在 2017/3/22 2:59, Leif Lindholm 写道:

>>> On Mon, Mar 20, 2017 at 09:11:25PM +0800, Chenhui Sun wrote:

>>>> Support the feature that BIOS get boot option from BMC and

>>>> put it in the first boot order.

>>> So first of all - I am really happy to see this support. It will be a

>>> huge improvement for validation.

>>>

>>> But I will mention that we now have a common Bds, and most other

>>> platforms have migrated away from the IntelBds. It would be very good

>>> if the Hisilicon enterprise platforms could also migrate to

>>> MdeModulePkg/Universal/BdsDxe/.

>> Yes, we tried to switch MdeModulePkg/Universal/BdsDxe in ERP16.12, but there

>> were two issues at that time,

>> we plan to switch MdeModulePkg Bds after fix that issue.

>>

>>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>>> Signed-off-by: huangming <huangming23@huawei.com>

>>>> Signed-off-by: Chenhui Sun <sunchenhui@huawei.com>

>>>> ---

>>>>    .../Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 310

>>>> +++++++++++++++++++++

>>>>    .../PlatformIntelBdsLib/PlatformIntelBdsLib.inf    |   2 +

>>>>    Platforms/Hisilicon/D03/D03.dsc                    |   1 -

>>>>    Platforms/Hisilicon/D05/D05.dsc                    |   1 -

>>>>    4 files changed, 312 insertions(+), 2 deletions(-)

>>>>

>>>> diff --git

>>>> a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> index efefeb6..7bba2f4 100644

>>>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c

>>>> @@ -19,18 +19,108 @@

>>>>      **/

>>>>    +#include <Guid/GlobalVariable.h>

>>>>    #include <IndustryStandard/Pci22.h>

>>>>    #include <Library/DevicePathLib.h>

>>>> +#include <Library/GenericBdsLib.h>

>>>> +#include <Library/MemoryAllocationLib.h>

>>>>    #include <Library/PcdLib.h>

>>>>    #include <Library/PlatformBdsLib.h>

>>>> +#include <Library/PrintLib.h>

>>>>    #include <Library/UefiLib.h>

>>>>    #include <Protocol/DevicePath.h>

>>>> +#include <Protocol/DevicePathToText.h>

>>>>    #include <Protocol/GraphicsOutput.h>

>>>>    #include <Protocol/PciIo.h>

>>>>    #include <Protocol/PciRootBridgeIo.h>

>>>>      #include "IntelBdsPlatform.h"

>>>>    +#define BOOT_OPTION_BOOT_FLAG_VALID         1

>>>> +#define BOOT_OPTION_BOOT_FLAG_INVALID         0

>>> Please align the above.

>> OK

>>>> +

>>>> +typedef enum {

>>>> +  NoOverride = 0x0,

>>>> +  ForcePxe,

>>>> +  ForceDefaultHardDisk,

>>>> +  ForceDefaultHardDiskSafeMode,

>>>> +  ForceDefaultDiagnosticPartition,

>>>> +  ForceDefaultCD,

>>>> +  ForceSetupUtility,

>>>> +  ForceRemoteRemovableMedia,

>>>> +  ForceRemoteCD,

>>>> +  ForcePrimaryRemoteMedia,

>>>> +  ForceRemoteHardDisk = 0xB,

>>>> +  ForcePrimaryRemovableMedia = 0xF

>>>> +} BOOT_DEVICE_SELECTOR;

>>> Please move these #defines, enums and structs into a .h file in the

>>> same directory.

>> Ok

>>

>>>> +

>>>> +// 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;

>>>> +

>>>> +#define EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID \

>>>> +  { \

>>>> +    0xA0441D0, 0x0, 0x0, {0x1, 0x1, 0x06, 0x0, 0x0, 0x0, 0x1, 0x1 } \

>>> This does not look like a GUID generated properly?

>> we will modify it using the GUID generated tool.

>>

>>>> +  }

>>>> +

>>>> +typedef struct{

>>>> +    UINT8 NodeType;

>>>> +    UINT8 NodeSubType;

>>>> +    EFI_GUID *Guid;

>>>> +    UINTN DeviceType;

>>>> +}OemDeviceType;

>>>> +

>>>> +OemDeviceType DeviceTypeArray[]={

>>>> +    {MESSAGING_DEVICE_PATH, MSG_SATA_DP,     NULL, BBS_TYPE_HARDDRIVE},

>>>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,

>>>> &((EFI_GUID)EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID), BBS_TYPE_HARDDRIVE},

>>>> +    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,

>>>> &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS), BBS_TYPE_HARDDRIVE},

>>>> +    {MESSAGING_DEVICE_PATH, MSG_USB_DP,      NULL, BBS_TYPE_USB},

>>>> +    {MESSAGING_DEVICE_PATH, MSG_MAC_ADDR_DP, NULL,

>>>> BBS_TYPE_EMBEDDED_NETWORK},

>>>> +    {MEDIA_DEVICE_PATH, MEDIA_CDROM_DP,      NULL, BBS_TYPE_CDROM}

>>>> +};

>>>> +

>>>> +EFI_STATUS IpmiCmdGetSysBootOptions(OUT IPMI_GET_BOOT_OPTION *BootOption

>>>> );

>>>> +EFI_STATUS IpmiCmdSetSysBootOptions(IPMI_GET_BOOT_OPTION    *BootOption

>>>> );

>>> No, these prototypes need to be added to some kind of exported header

>>> for IpmiCmdLib. Hmm, I noticed there is another inline declaration in

>>> SmbiosMiscDxe, of IpmiGetChassisType.

>>>

>>> But really, all of the exported symbols from

>>> Chips/Hisilicon/Binary/Hi1610/Library/IpmiCmdLib/IpmiCmdLib.lib need

>>> to be declared in a single header file.

>> ok

>>>>    //3CEF354A-3B7A-4519-AD70-72A134698311

>>>>    GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70,

>>>> @@ -361,6 +451,223 @@ AddOutput (

>>>>        ReportText));

>>>>    }

>>>>    +UINT16 DeviceTypeFoundInFileSystemHandles (EFI_DEVICE_PATH_PROTOCOL

>>>> *DevicePathIn)

>>> STATIC

>>> UINT16

>>> DeviceTypeFoundInFileSystemHandles (

>>>     EFI_DEVICE_PATH_PROTOCOL *DevicePathIn

>>>     )

>>>

>>> Also, add IN OUT indicators as appropriate.

>> ok

>>>> +{

>>>> +  EFI_STATUS                    Status;

>>>> +  EFI_HANDLE                    *FileSystemHandles;

>>>> +  UINTN                         NumberFileSystemHandles;

>>>> +  UINTN                         Index;

>>>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathGot;

>>>> +  EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;

>>>> +  CHAR16*                       DevicePathTxtIn = NULL;

>>>> +  CHAR16*                       DevicePathTxtGot = NULL;

>>> * goes with the variable name, not with the type.

>>>

>>>> +  EFI_DEVICE_PATH_PROTOCOL      *DevicePathNode = NULL;

>>>> +  UINT16                        Result = BBS_TYPE_UNKNOWN;

>>>> +

>>>> +  Status = gBS->LocateHandleBuffer (

>>>> +            ByProtocol,

>>> Indentation is supposed to be 2 spaces from function name,

>>> in this case:

>>>

>>>     Status = gBS->LocateHandleBuffer (

>>>                     ByProtocol,

>> ok

>>>> +            &gEfiSimpleFileSystemProtocolGuid,

>>>> +            NULL,

>>>> +            &NumberFileSystemHandles,

>>>> +            &FileSystemHandles

>>>> +            );

>>>> +  if (EFI_ERROR (Status)) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));

>>>> +    return BBS_TYPE_UNKNOWN;

>>>> +  }

>>>> +

>>>> +  Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL,

>>>> (VOID **)&DevicePathToTextProtocol);

>>>> +  if (EFI_ERROR (Status)) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));

>>>> +    return BBS_TYPE_UNKNOWN;

>>>> +  }

>>>> +  DevicePathTxtIn = DevicePathToTextProtocol->ConvertDevicePathToText

>>>> (DevicePathIn, TRUE, TRUE);

>>>> +

>>>> +  for (Index = 0; Index < NumberFileSystemHandles; Index++) {

>>>> +    DevicePathGot = DevicePathFromHandle (FileSystemHandles[Index]);

>>>> +    DevicePathTxtGot = DevicePathToTextProtocol->ConvertDevicePathToText

>>>> (DevicePathGot, TRUE, TRUE);

>>>> +

>>>> +    if (StrnCmp(DevicePathTxtIn, DevicePathTxtGot,

>>>> StrLen(DevicePathTxtIn)) == 0) {

>>> Spaces after function names.

>> will be modified

>>

>>>> +      DevicePathNode = DevicePathGot;

>>>> +      while (!IsDevicePathEnd (DevicePathNode)) {

>>>> +        if ((DevicePathNode->Type == MEDIA_DEVICE_PATH) &&

>>>> (DevicePathNode->SubType == MEDIA_CDROM_DP)) {

>>>> +          Result = BBS_TYPE_CDROM;

>>>> +          break;

>>>> +        }

>>>> +        DevicePathNode = NextDevicePathNode (DevicePathNode);

>>>> +      }

>>>> +    }

>>>> +

>>>> +    if (Result != BBS_TYPE_UNKNOWN) {

>>>> +      break;

>>>> +    }

>>>> +  }

>>>> +

>>>> +  if (NumberFileSystemHandles != 0) {

>>>> +    FreePool (FileSystemHandles);

>>>> +  }

>>>> +  if (DevicePathTxtGot != NULL) {

>>>> +    FreePool (DevicePathTxtGot);

>>>> +  }

>>>> +  if (DevicePathTxtIn != NULL) {

>>>> +    FreePool (DevicePathTxtIn);

>>>> +  }

>>>> +

>>>> +  return Result;

>>>> +}

>>>> +

>>>> +UINT16 UniGetEfiDeviceType(

>>>> +  IN  BDS_COMMON_OPTION *BootOption

>>>> +)

>>> STATIC

>>> UINT16

>>> UniGetEfiDeviceType (

>>>     IN  BDS_COMMON_OPTION *BootOption

>>>     )

>> will be modified.

>>>> +{

>>>> +  EFI_DEVICE_PATH_PROTOCOL*  DevicePathNode;

>>>> +  UINTN DeviceCnt;

>>>> +  UINTN Loop;

>>>> +  VENDOR_DEVICE_PATH *Vender;

>>> "Vender" -> "Vendor".

>> Thanks for pointing this.

>>>> +  UINT16 Result;

>>>> +

>>>> +  DeviceCnt = sizeof (DeviceTypeArray) / sizeof (OemDeviceType);

>>>> +  DevicePathNode = BootOption->DevicePath;

>>>> +  while (!IsDevicePathEnd (DevicePathNode)) {

>>>> +    for (Loop = 0; Loop < DeviceCnt; Loop++) {

>>>> +      if ((DevicePathType (DevicePathNode) ==

>>>> DeviceTypeArray[Loop].NodeType) &&

>>>> +         (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {

>>> Continuation of test should align with start:

>>>

>>>         if ((DevicePathType (DevicePathNode) ==

>>> DeviceTypeArray[Loop].NodeType) &&

>>>             (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {

>> ok

>>>> +        Vender = (VENDOR_DEVICE_PATH*)(DevicePathNode);

>>>> +        if (CompareMem(&(Vender->Guid), DeviceTypeArray[Loop].Guid,

>>>> sizeof(EFI_GUID)) == 0) {

>>> Use CompareGuid () instead?

>>> (And space after function name.)

>> ok

>>>> +            return DeviceTypeArray[Loop].DeviceType;

>>>> +        }

>>>> +      } else if ((DevicePathType (DevicePathNode) ==

>>>> MESSAGING_DEVICE_PATH) &&

>>>> +        (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {

>>>         } else if ((DevicePathType (DevicePathNode) ==

>>> MESSAGING_DEVICE_PATH) &&

>>>                    (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {

>> ok

>>>> +        Result = DeviceTypeFoundInFileSystemHandles

>>>> (BootOption->DevicePath);

>>>> +        if (Result != BBS_TYPE_UNKNOWN) {

>>>> +          return Result;

>>>> +        }

>>>> +      } else if ((DevicePathType (DevicePathNode) ==

>>>> DeviceTypeArray[Loop].NodeType) &&

>>>> +        (DevicePathSubType (DevicePathNode) ==

>>>> DeviceTypeArray[Loop].NodeSubType)) {

>>>         } else if ((DevicePathType (DevicePathNode) ==

>>> DeviceTypeArray[Loop].NodeType) &&

>>>                    (DevicePathSubType (DevicePathNode) ==

>>> DeviceTypeArray[Loop].NodeSubType)) {

>> will be modified.

>>>> +        return DeviceTypeArray[Loop].DeviceType;

>>>> +      }

>>>> +    }

>>>> +

>>>> +    DevicePathNode = NextDevicePathNode (DevicePathNode);

>>>> +  }

>>>> +

>>>> +  return BBS_TYPE_UNKNOWN;

>>>> +}

>>>> +

>>>> +EFI_STATUS GetBmcBootOptionsSetting (IPMI_GET_BOOT_OPTION *BmcBootOpt)

>>> STATIC

>>> EFI_STATUS

>>> GetBmcBootOptionsSetting (

>>>     IPMI_GET_BOOT_OPTION *BmcBootOpt

>>>     )

>>>

>>> And add IN OUT indicators.

>> ok

>>

>>>> +{

>>>> +  EFI_STATUS   Status = EFI_SUCCESS;

>>>> +

>>>> +  Status = IpmiCmdGetSysBootOptions (BmcBootOpt);

>>>> +  if (EFI_ERROR(Status)) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a - %d  Get iBMC BootOpts %r!\n",

>>>> __FUNCTION__, __LINE__,Status));

>>>> +    return Status;

>>>> +  }

>>>> +

>>>> +  if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a - %d  BootFlags is Invalid !\n",

>>>> __FUNCTION__, __LINE__));

>>>> +    return EFI_INVALID_PARAMETER;

>>>> +  }

>>>> +

>>>> +  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, "%a - %d  Set iBMC BootOpts %r!\n",

>>>> __FUNCTION__, __LINE__, Status));

>>>> +  }

>>>> +

>>>> +  return Status;

>>>> +}

>>>> +

>>>> +VOID ProductBdsPolicyAfterSetup ( VOID )

>>> STATIC

>>> VOID

>>> ProductBdsPolicyAfterSetup (

>>>     VOID

>>>     )

>>>

>>>> +{

>>>> +  EFI_STATUS                Status = EFI_SUCCESS;

>>>> +  IPMI_GET_BOOT_OPTION      BmcBootOpt;

>>>> +  UINT16                    *OptionOrder;

>>>> +  UINTN                     OptionOrderSize;

>>>> +  UINTN                     DeviceType = BBS_TYPE_UNKNOWN;

>>>> +  UINTN                     Index;

>>>> +  BDS_COMMON_OPTION         *Option;

>>>> +  CHAR16                    OptionName[20];

>>> Why 20?

>> um..we think the length 20 is enough.

>>

>>>> +  LIST_ENTRY                BootOptionList;

>>>> +  UINT16                    BootIdx;

>>>> +  UINT16                    *BootNextBuf;

>>>> +

>>>> +  InitializeListHead (&BootOptionList);

>>>> +

>>>> +  Status = GetBmcBootOptionsSetting (&BmcBootOpt);

>>>> +  if (EFI_ERROR(Status)) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a - %d : %r!\n", __FUNCTION__,

>>>> __LINE__,Status));

>>>> +    return;

>>>> +  }

>>>> +

>>>> +  if (BmcBootOpt.BootDeviceSelector == ForcePrimaryRemovableMedia) {

>>>> +    DeviceType = BBS_TYPE_USB;

>>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForcePxe) {

>>>> +    DeviceType = BBS_TYPE_EMBEDDED_NETWORK;

>>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultHardDisk) {

>>>> +    DeviceType = BBS_TYPE_HARDDRIVE;

>>>> +  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultCD) {

>>>> +    DeviceType = BBS_TYPE_CDROM;

>>>> +  } else {

>>>> +    return;

>>>> +  }

>>>> +

>>>> +  DEBUG ((DEBUG_ERROR, "BMC set BootType=%x\n", DeviceType));

>>>> +

>>>> +  OptionOrder = BdsLibGetVariableAndSize (

>>>> +                  L"BootOrder",

>>>> +                  &gEfiGlobalVariableGuid,

>>>> +                  &OptionOrderSize

>>>> +                  );

>>>> +  if (OptionOrder == NULL) {

>>>> +    DEBUG ((DEBUG_ERROR, "%a - %d error\n", __FUNCTION__, __LINE__));

>>>> +    return;

>>>> +  }

>>>> +

>>>> +  BootIdx = 0;

>>>> +  BootNextBuf = (UINT16*)AllocatePool(OptionOrderSize);

>>> Space after function name.

>> ok

>>>

>>>> +  if (BootNextBuf == NULL) {

>>>> +    DEBUG ((DEBUG_ERROR, "Out of resources.\n"));

>>>> +    return;

>>>> +  }

>>>> +

>>>> +  for (Index = 0; Index < OptionOrderSize / sizeof (UINT16); Index++) {

>>>> +    UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x",

>>>> OptionOrder[Index]);

>>>> +    Option = BdsLibVariableToOption (&BootOptionList, OptionName);

>>>> +    if (Option == NULL) {

>>>> +      DEBUG((DEBUG_ERROR, "%a - %d  Boot%04x is Null!\n", __FUNCTION__,

>>>> __LINE__, OptionOrder[Index]));

>>>> +      continue;

>>>> +    }

>>>> +

>>>> +    if (DeviceType == UniGetEfiDeviceType(Option)) {

>>> Space after function name.

>> ok

>>>> +      BootNextBuf[BootIdx] = OptionOrder[Index];

>>>> +      BootIdx++;

>>>> +    }

>>>> +    RemoveEntryList (&Option->Link);

>>>> +    FreePool (Option);

>>>> +  }

>>>> +

>>>> +  if (BootIdx > 0) {

>>>> +    Status = gRT->SetVariable (

>>>> +      L"BootNext",

>>>> +      &gEfiGlobalVariableGuid,

>>>> +      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS |

>>>> EFI_VARIABLE_NON_VOLATILE,

>>> I think a temporary variable would make this more readable.

>> ok

>>>> +      BootIdx*sizeof(UINT16),

>>> Spaces around "*".

>>> Space after function name. (OK, I know sizeof isn't technically a

>>> function, but...)

>> ok, will be modified.

>>

>>>> +      BootNextBuf

>>>> +      );

>>>> +    DEBUG ((DEBUG_ERROR, "Set BootNext %r, size=%x\n", Status,

>>>> BootIdx*sizeof(UINT16)));

>>>> +  }

>>>> +

>>>> +  FreePool (OptionOrder);

>>>> +  FreePool (BootNextBuf);

>>>> +

>>>> +  return;

>>>> +}

>>>>      /**

>>>>      The function will execute with as the platform policy, current policy

>>>> @@ -469,6 +776,9 @@ PlatformBdsPolicyBehavior (

>>>>      //

>>>>      BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder");

>>>>    +  //get boot option from BMC

>>>> +  ProductBdsPolicyAfterSetup();

>>>> +

>>>>      //PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE);

>>>>      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));

>>>> diff --git

>>>> a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> index baceb57..a09683d 100644

>>>> --- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> +++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf

>>>> @@ -49,6 +49,7 @@

>>>>      DebugLib

>>>>      DevicePathLib

>>>>      GenericBdsLib

>>>> +  IpmiCmdLib

>>>>      MemoryAllocationLib

>>>>      PcdLib

>>>>      PrintLib

>>>> @@ -78,3 +79,4 @@

>>>>      gEfiLoadedImageProtocolGuid

>>>>      gEfiPciRootBridgeIoProtocolGuid

>>>>      gEfiSimpleFileSystemProtocolGuid

>>>> +  gEfiDevicePathToTextProtocolGuid

>>>> diff --git a/Platforms/Hisilicon/D03/D03.dsc

>>>> b/Platforms/Hisilicon/D03/D03.dsc

>>>> index 24c88a3..05dd5d8 100644

>>>> --- a/Platforms/Hisilicon/D03/D03.dsc

>>>> +++ b/Platforms/Hisilicon/D03/D03.dsc

>>>> @@ -367,7 +367,6 @@

>>>>      OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

>>>>      MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

>>>>        <LibraryClasses>

>>>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf

>>> This deletion....

>>>

>>>>          BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf

>>>>      }

>>>>      MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf

>>>> diff --git a/Platforms/Hisilicon/D05/D05.dsc

>>>> b/Platforms/Hisilicon/D05/D05.dsc

>>>> index 9de1be6..efdedfd 100644

>>>> --- a/Platforms/Hisilicon/D05/D05.dsc

>>>> +++ b/Platforms/Hisilicon/D05/D05.dsc

>>>> @@ -487,7 +487,6 @@

>>>>      OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf

>>>>      MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

>>>>        <LibraryClasses>

>>>> -      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf

>>> ... and this one.

>>> It is not clear to me how this is related to the overall changeset of

>>> this patch. Is it separate cleanup that should be a separate patch?

>>

>> The BootNext will be limited to sizeof(UINT16) in

>> VarCheckUefiLibNullClass.c,

>> but in the case of 4 network ports , the BootNext should be like 0x01020304

>> by the UniBootNextVariableUpdate function.

>>

>> Thanks and Regards,

>> Chenhui

>>

>>

>>> Regards,

>>>

>>> Leif

>>>

>>>>          BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf

>>>>      }

>>>>      MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf

>>>> --

>>>> 1.9.1

>>>>
diff mbox series

Patch

diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
index efefeb6..7bba2f4 100644
--- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
+++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
@@ -19,18 +19,108 @@ 
 
 **/
 
+#include <Guid/GlobalVariable.h>
 #include <IndustryStandard/Pci22.h>
 #include <Library/DevicePathLib.h>
+#include <Library/GenericBdsLib.h>
+#include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PlatformBdsLib.h>
+#include <Library/PrintLib.h>
 #include <Library/UefiLib.h>
 #include <Protocol/DevicePath.h>
+#include <Protocol/DevicePathToText.h>
 #include <Protocol/GraphicsOutput.h>
 #include <Protocol/PciIo.h>
 #include <Protocol/PciRootBridgeIo.h>
 
 #include "IntelBdsPlatform.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;
+
+#define EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID \
+  { \
+    0xA0441D0, 0x0, 0x0, {0x1, 0x1, 0x06, 0x0, 0x0, 0x0, 0x1, 0x1 } \
+  }
+
+typedef struct{
+    UINT8 NodeType;
+    UINT8 NodeSubType;
+    EFI_GUID *Guid;
+    UINTN DeviceType;
+}OemDeviceType;
+
+OemDeviceType DeviceTypeArray[]={
+    {MESSAGING_DEVICE_PATH, MSG_SATA_DP,     NULL, BBS_TYPE_HARDDRIVE},
+    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)EFI_ACPI_PCI_SAS_DEVICE_PATH_GUID), BBS_TYPE_HARDDRIVE},
+    {MESSAGING_DEVICE_PATH, MSG_VENDOR_DP,   &((EFI_GUID)DEVICE_PATH_MESSAGING_SAS), BBS_TYPE_HARDDRIVE},
+    {MESSAGING_DEVICE_PATH, MSG_USB_DP,      NULL, BBS_TYPE_USB},
+    {MESSAGING_DEVICE_PATH, MSG_MAC_ADDR_DP, NULL, BBS_TYPE_EMBEDDED_NETWORK},
+    {MEDIA_DEVICE_PATH, MEDIA_CDROM_DP,      NULL, BBS_TYPE_CDROM}
+};
+
+EFI_STATUS IpmiCmdGetSysBootOptions(OUT IPMI_GET_BOOT_OPTION *BootOption  );
+EFI_STATUS IpmiCmdSetSysBootOptions(IPMI_GET_BOOT_OPTION    *BootOption  );
 
 //3CEF354A-3B7A-4519-AD70-72A134698311
 GUID gEblFileGuid = {0x3CEF354A, 0x3B7A, 0x4519, {0xAD, 0x70,
@@ -361,6 +451,223 @@  AddOutput (
     ReportText));
 }
 
+UINT16 DeviceTypeFoundInFileSystemHandles (EFI_DEVICE_PATH_PROTOCOL *DevicePathIn)
+{
+  EFI_STATUS                    Status;
+  EFI_HANDLE                    *FileSystemHandles;
+  UINTN                         NumberFileSystemHandles;
+  UINTN                         Index;
+  EFI_DEVICE_PATH_PROTOCOL      *DevicePathGot;
+  EFI_DEVICE_PATH_TO_TEXT_PROTOCOL* DevicePathToTextProtocol;
+  CHAR16*                       DevicePathTxtIn = NULL;
+  CHAR16*                       DevicePathTxtGot = NULL;
+  EFI_DEVICE_PATH_PROTOCOL      *DevicePathNode = NULL;
+  UINT16                        Result = BBS_TYPE_UNKNOWN;
+
+  Status = gBS->LocateHandleBuffer (
+            ByProtocol,
+            &gEfiSimpleFileSystemProtocolGuid,
+            NULL,
+            &NumberFileSystemHandles,
+            &FileSystemHandles
+            );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
+    return BBS_TYPE_UNKNOWN;
+  }
+
+  Status = gBS->LocateProtocol (&gEfiDevicePathToTextProtocolGuid, NULL, (VOID **)&DevicePathToTextProtocol);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a(%d):error!\n", __FUNCTION__,__LINE__));
+    return BBS_TYPE_UNKNOWN;
+  }
+  DevicePathTxtIn = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathIn, TRUE, TRUE);
+
+  for (Index = 0; Index < NumberFileSystemHandles; Index++) {
+    DevicePathGot = DevicePathFromHandle (FileSystemHandles[Index]);
+    DevicePathTxtGot = DevicePathToTextProtocol->ConvertDevicePathToText (DevicePathGot, TRUE, TRUE);
+
+    if (StrnCmp(DevicePathTxtIn, DevicePathTxtGot, StrLen(DevicePathTxtIn)) == 0) {
+      DevicePathNode = DevicePathGot;
+      while (!IsDevicePathEnd (DevicePathNode)) {
+        if ((DevicePathNode->Type == MEDIA_DEVICE_PATH) && (DevicePathNode->SubType == MEDIA_CDROM_DP)) {
+          Result = BBS_TYPE_CDROM;
+          break;
+        }
+        DevicePathNode = NextDevicePathNode (DevicePathNode);
+      }
+    }
+
+    if (Result != BBS_TYPE_UNKNOWN) {
+      break;
+    }
+  }
+
+  if (NumberFileSystemHandles != 0) {
+    FreePool (FileSystemHandles);
+  }
+  if (DevicePathTxtGot != NULL) {
+    FreePool (DevicePathTxtGot);
+  }
+  if (DevicePathTxtIn != NULL) {
+    FreePool (DevicePathTxtIn);
+  }
+
+  return Result;
+}
+
+UINT16 UniGetEfiDeviceType(
+  IN  BDS_COMMON_OPTION *BootOption
+)
+{
+  EFI_DEVICE_PATH_PROTOCOL*  DevicePathNode;
+  UINTN DeviceCnt;
+  UINTN Loop;
+  VENDOR_DEVICE_PATH *Vender;
+  UINT16 Result;
+
+  DeviceCnt = sizeof (DeviceTypeArray) / sizeof (OemDeviceType);
+  DevicePathNode = BootOption->DevicePath;
+  while (!IsDevicePathEnd (DevicePathNode)) {
+    for (Loop = 0; Loop < DeviceCnt; Loop++) {
+      if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
+         (DevicePathSubType (DevicePathNode) == MSG_VENDOR_DP)) {
+        Vender = (VENDOR_DEVICE_PATH*)(DevicePathNode);
+        if (CompareMem(&(Vender->Guid), DeviceTypeArray[Loop].Guid, sizeof(EFI_GUID)) == 0) {
+            return DeviceTypeArray[Loop].DeviceType;
+        }
+      } else if ((DevicePathType (DevicePathNode) == MESSAGING_DEVICE_PATH) &&
+        (DevicePathSubType (DevicePathNode) == MSG_USB_DP)) {
+        Result = DeviceTypeFoundInFileSystemHandles (BootOption->DevicePath);
+        if (Result != BBS_TYPE_UNKNOWN) {
+          return Result;
+        }
+      } else if ((DevicePathType (DevicePathNode) == DeviceTypeArray[Loop].NodeType) &&
+        (DevicePathSubType (DevicePathNode) == DeviceTypeArray[Loop].NodeSubType)) {
+        return DeviceTypeArray[Loop].DeviceType;
+      }
+    }
+
+    DevicePathNode = NextDevicePathNode (DevicePathNode);
+  }
+
+  return BBS_TYPE_UNKNOWN;
+}
+
+EFI_STATUS GetBmcBootOptionsSetting (IPMI_GET_BOOT_OPTION *BmcBootOpt)
+{
+  EFI_STATUS   Status = EFI_SUCCESS;
+
+  Status = IpmiCmdGetSysBootOptions (BmcBootOpt);
+  if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR, "%a - %d  Get iBMC BootOpts %r!\n", __FUNCTION__, __LINE__,Status));
+    return Status;
+  }
+
+  if (BmcBootOpt->BootFlagsValid != BOOT_OPTION_BOOT_FLAG_VALID) {
+    DEBUG ((DEBUG_ERROR, "%a - %d  BootFlags is Invalid !\n", __FUNCTION__, __LINE__));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  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, "%a - %d  Set iBMC BootOpts %r!\n", __FUNCTION__, __LINE__, Status));
+  }
+
+  return Status;
+}
+
+VOID ProductBdsPolicyAfterSetup ( VOID )
+{
+  EFI_STATUS                Status = EFI_SUCCESS;
+  IPMI_GET_BOOT_OPTION      BmcBootOpt;
+  UINT16                    *OptionOrder;
+  UINTN                     OptionOrderSize;
+  UINTN                     DeviceType = BBS_TYPE_UNKNOWN;
+  UINTN                     Index;
+  BDS_COMMON_OPTION         *Option;
+  CHAR16                    OptionName[20];
+  LIST_ENTRY                BootOptionList;
+  UINT16                    BootIdx;
+  UINT16                    *BootNextBuf;
+
+  InitializeListHead (&BootOptionList);
+
+  Status = GetBmcBootOptionsSetting (&BmcBootOpt);
+  if (EFI_ERROR(Status)) {
+    DEBUG ((DEBUG_ERROR, "%a - %d : %r!\n", __FUNCTION__, __LINE__,Status));
+    return;
+  }
+
+  if (BmcBootOpt.BootDeviceSelector == ForcePrimaryRemovableMedia) {
+    DeviceType = BBS_TYPE_USB;
+  } else if (BmcBootOpt.BootDeviceSelector == ForcePxe) {
+    DeviceType = BBS_TYPE_EMBEDDED_NETWORK;
+  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultHardDisk) {
+    DeviceType = BBS_TYPE_HARDDRIVE;
+  } else if (BmcBootOpt.BootDeviceSelector == ForceDefaultCD) {
+    DeviceType = BBS_TYPE_CDROM;
+  } else {
+    return;
+  }
+
+  DEBUG ((DEBUG_ERROR, "BMC set BootType=%x\n", DeviceType));
+
+  OptionOrder = BdsLibGetVariableAndSize (
+                  L"BootOrder",
+                  &gEfiGlobalVariableGuid,
+                  &OptionOrderSize
+                  );
+  if (OptionOrder == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a - %d error\n", __FUNCTION__, __LINE__));
+    return;
+  }
+
+  BootIdx = 0;
+  BootNextBuf = (UINT16*)AllocatePool(OptionOrderSize);
+  if (BootNextBuf == NULL) {
+    DEBUG ((DEBUG_ERROR, "Out of resources.\n"));
+    return;
+  }
+
+  for (Index = 0; Index < OptionOrderSize / sizeof (UINT16); Index++) {
+    UnicodeSPrint (OptionName, sizeof (OptionName), L"Boot%04x", OptionOrder[Index]);
+    Option = BdsLibVariableToOption (&BootOptionList, OptionName);
+    if (Option == NULL) {
+      DEBUG((DEBUG_ERROR, "%a - %d  Boot%04x is Null!\n", __FUNCTION__, __LINE__, OptionOrder[Index]));
+      continue;
+    }
+
+    if (DeviceType == UniGetEfiDeviceType(Option)) {
+      BootNextBuf[BootIdx] = OptionOrder[Index];
+      BootIdx++;
+    }
+    RemoveEntryList (&Option->Link);
+    FreePool (Option);
+  }
+
+  if (BootIdx > 0) {
+    Status = gRT->SetVariable (
+      L"BootNext",
+      &gEfiGlobalVariableGuid,
+      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
+      BootIdx*sizeof(UINT16),
+      BootNextBuf
+      );
+    DEBUG ((DEBUG_ERROR, "Set BootNext %r, size=%x\n", Status, BootIdx*sizeof(UINT16)));
+  }
+
+  FreePool (OptionOrder);
+  FreePool (BootNextBuf);
+
+  return;
+}
 
 /**
   The function will execute with as the platform policy, current policy
@@ -469,6 +776,9 @@  PlatformBdsPolicyBehavior (
   //
   BdsLibBuildOptionFromVar (BootOptionList, L"BootOrder");
 
+  //get boot option from BMC
+  ProductBdsPolicyAfterSetup();
+
   //PlatformBdsEnterFrontPage (GetFrontPageTimeoutFromQemu(), TRUE);
   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));
diff --git a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
index baceb57..a09683d 100644
--- a/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
+++ b/Chips/Hisilicon/Library/PlatformIntelBdsLib/PlatformIntelBdsLib.inf
@@ -49,6 +49,7 @@ 
   DebugLib
   DevicePathLib
   GenericBdsLib
+  IpmiCmdLib
   MemoryAllocationLib
   PcdLib
   PrintLib
@@ -78,3 +79,4 @@ 
   gEfiLoadedImageProtocolGuid
   gEfiPciRootBridgeIoProtocolGuid
   gEfiSimpleFileSystemProtocolGuid
+  gEfiDevicePathToTextProtocolGuid
diff --git a/Platforms/Hisilicon/D03/D03.dsc b/Platforms/Hisilicon/D03/D03.dsc
index 24c88a3..05dd5d8 100644
--- a/Platforms/Hisilicon/D03/D03.dsc
+++ b/Platforms/Hisilicon/D03/D03.dsc
@@ -367,7 +367,6 @@ 
   OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
-      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
       BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   }
   MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf
diff --git a/Platforms/Hisilicon/D05/D05.dsc b/Platforms/Hisilicon/D05/D05.dsc
index 9de1be6..efdedfd 100644
--- a/Platforms/Hisilicon/D05/D05.dsc
+++ b/Platforms/Hisilicon/D05/D05.dsc
@@ -487,7 +487,6 @@ 
   OpenPlatformPkg/Chips/Hisilicon/Drivers/FlashFvbDxe/FlashFvbDxe.inf
   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {
     <LibraryClasses>
-      NULL|MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
       BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   }
   MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf