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