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.infThis deletion....
BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf } MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.infdiff --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