[Linaro-uefi] IntelFrameworkModulePkg/BdsEntry: support BMC boot option

Message ID 1490099187-54849-1-git-send-email-chenhui.sun@linaro.org
State New
Headers show

Commit Message

Chenhui Sun March 21, 2017, 12:26 p.m.
Support the feature that BIOS get boot option from BMC and set
it to the first boot order.

The feature works dependding on this patch and the OPP patch
"Hisilicon/D03/D05: get boot option from BMC" both be added.

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.

so is there any solution? expect your comments.

Signed-off-by: Huang ming <huangming23@linaro.org>
Signed-off-by: Chenhui Sun <chenhui.sun@linaro.org>
---
 .../Universal/BdsDxe/BdsEntry.c                    | 44 ++++++++++++----------
 1 file changed, 24 insertions(+), 20 deletions(-)

Comments

Chenhui Sun March 21, 2017, 12:36 p.m. | #1
Hi Leif,

This patch is under IntelFrameworkModulePkg,

So I sent it out indenpdently, expect your comments about the

solution and the limitation.

Thanks and Regards,

Chenhui


在 2017/3/21 20:26, Chenhui Sun 写道:
> Support the feature that BIOS get boot option from BMC and set
> it to the first boot order.
>
> The feature works dependding on this patch and the OPP patch
> "Hisilicon/D03/D05: get boot option from BMC" both be added.
>
> 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.
>
> so is there any solution? expect your comments.
>
> Signed-off-by: Huang ming <huangming23@linaro.org>
> Signed-off-by: Chenhui Sun <chenhui.sun@linaro.org>
> ---
>   .../Universal/BdsDxe/BdsEntry.c                    | 44 ++++++++++++----------
>   1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> index bf81de4..ee51055 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -36,7 +36,6 @@ EFI_BDS_ARCH_PROTOCOL  gBds = {
>     BdsEntry
>   };
>   
> -UINT16                          *mBootNext = NULL;
>   
>   ///
>   /// The read-only variables defined in UEFI Spec.
> @@ -123,6 +122,9 @@ BdsBootDeviceSelect (
>     BOOLEAN           BootNextExist;
>     LIST_ENTRY        *LinkBootNext;
>     EFI_EVENT         ConnectConInEvent;
> +  UINTN             BootNextSize;
> +  UINT16            *BootNext = NULL;
> +  UINT16            Index;
>   
>     //
>     // Got the latest boot option
> @@ -154,7 +156,16 @@ BdsBootDeviceSelect (
>       }
>     }
>   
> -  if (mBootNext != NULL) {
> +  //
> +  // Check if we have the boot next option
> +  //
> +  BootNext = BdsLibGetVariableAndSize (
> +                L"BootNext",
> +                &gEfiGlobalVariableGuid,
> +                &BootNextSize
> +                );
> +
> +  if (BootNext != NULL) {
>       //
>       // Indicate we have the boot next variable, so this time
>       // boot will always have this boot option
> @@ -179,17 +190,19 @@ BdsBootDeviceSelect (
>       //
>       // Add the boot next boot option
>       //
> -    UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", *mBootNext);
> -    BootOption = BdsLibVariableToOption (&BootLists, Buffer);
> +    for (Index = 0; Index < (BootNextSize/sizeof(UINT16)); Index++) {
> +      UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", BootNext[Index]);
> +      BootOption = BdsLibVariableToOption (&BootLists, Buffer);
>   
> -    //
> -    // If fail to get boot option from variable, just return and do nothing.
> -    //
> -    if (BootOption == NULL) {
> -      return;
> -    }
> +      //
> +      // If fail to get boot option from variable, just return and do nothing.
> +      //
> +      if (BootOption == NULL) {
> +        return;
> +      }
>   
> -    BootOption->BootCurrent = *mBootNext;
> +      BootOption->BootCurrent = BootNext[Index];
> +    }
>     }
>     //
>     // Parse the boot order to get boot option
> @@ -528,7 +541,6 @@ BdsEntry (
>   {
>     LIST_ENTRY                      DriverOptionList;
>     LIST_ENTRY                      BootOptionList;
> -  UINTN                           BootNextSize;
>     CHAR16                          *FirmwareVendor;
>     EFI_STATUS                      Status;
>     UINT16                          BootTimeOut;
> @@ -638,14 +650,6 @@ BdsEntry (
>     if (!IsListEmpty (&DriverOptionList)) {
>       BdsLibLoadDrivers (&DriverOptionList);
>     }
> -  //
> -  // Check if we have the boot next option
> -  //
> -  mBootNext = BdsLibGetVariableAndSize (
> -                L"BootNext",
> -                &gEfiGlobalVariableGuid,
> -                &BootNextSize
> -                );
>   
>     //
>     // Setup some platform policy here
Ard Biesheuvel March 27, 2017, 5:11 p.m. | #2
On 21 March 2017 at 12:26, Chenhui Sun <chenhui.sun@linaro.org> wrote:
> Support the feature that BIOS get boot option from BMC and set
> it to the first boot order.
>
> The feature works dependding on this patch and the OPP patch
> "Hisilicon/D03/D05: get boot option from BMC" both be added.
>
> 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.
>
> so is there any solution? expect your comments.
>
> Signed-off-by: Huang ming <huangming23@linaro.org>
> Signed-off-by: Chenhui Sun <chenhui.sun@linaro.org>

I understand that this change is required to allow the boot order to
be set via the BMC.

But could you please explain why you need to make the changes below?
Why do you have to change the way BootNext is handled?

> ---
>  .../Universal/BdsDxe/BdsEntry.c                    | 44 ++++++++++++----------
>  1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> index bf81de4..ee51055 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -36,7 +36,6 @@ EFI_BDS_ARCH_PROTOCOL  gBds = {
>    BdsEntry
>  };
>
> -UINT16                          *mBootNext = NULL;
>
>  ///
>  /// The read-only variables defined in UEFI Spec.
> @@ -123,6 +122,9 @@ BdsBootDeviceSelect (
>    BOOLEAN           BootNextExist;
>    LIST_ENTRY        *LinkBootNext;
>    EFI_EVENT         ConnectConInEvent;
> +  UINTN             BootNextSize;
> +  UINT16            *BootNext = NULL;
> +  UINT16            Index;
>
>    //
>    // Got the latest boot option
> @@ -154,7 +156,16 @@ BdsBootDeviceSelect (
>      }
>    }
>
> -  if (mBootNext != NULL) {
> +  //
> +  // Check if we have the boot next option
> +  //
> +  BootNext = BdsLibGetVariableAndSize (
> +                L"BootNext",
> +                &gEfiGlobalVariableGuid,
> +                &BootNextSize
> +                );
> +
> +  if (BootNext != NULL) {
>      //
>      // Indicate we have the boot next variable, so this time
>      // boot will always have this boot option
> @@ -179,17 +190,19 @@ BdsBootDeviceSelect (
>      //
>      // Add the boot next boot option
>      //
> -    UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", *mBootNext);
> -    BootOption = BdsLibVariableToOption (&BootLists, Buffer);
> +    for (Index = 0; Index < (BootNextSize/sizeof(UINT16)); Index++) {
> +      UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", BootNext[Index]);
> +      BootOption = BdsLibVariableToOption (&BootLists, Buffer);
>
> -    //
> -    // If fail to get boot option from variable, just return and do nothing.
> -    //
> -    if (BootOption == NULL) {
> -      return;
> -    }
> +      //
> +      // If fail to get boot option from variable, just return and do nothing.
> +      //
> +      if (BootOption == NULL) {
> +        return;
> +      }
>
> -    BootOption->BootCurrent = *mBootNext;
> +      BootOption->BootCurrent = BootNext[Index];
> +    }
>    }
>    //
>    // Parse the boot order to get boot option
> @@ -528,7 +541,6 @@ BdsEntry (
>  {
>    LIST_ENTRY                      DriverOptionList;
>    LIST_ENTRY                      BootOptionList;
> -  UINTN                           BootNextSize;
>    CHAR16                          *FirmwareVendor;
>    EFI_STATUS                      Status;
>    UINT16                          BootTimeOut;
> @@ -638,14 +650,6 @@ BdsEntry (
>    if (!IsListEmpty (&DriverOptionList)) {
>      BdsLibLoadDrivers (&DriverOptionList);
>    }
> -  //
> -  // Check if we have the boot next option
> -  //
> -  mBootNext = BdsLibGetVariableAndSize (
> -                L"BootNext",
> -                &gEfiGlobalVariableGuid,
> -                &BootNextSize
> -                );
>
>    //
>    // Setup some platform policy here
> --
> 1.9.1
>
Chenhui Sun April 20, 2017, 1:01 p.m. | #3
Hi Ard,


在 2017/3/28 1:11, Ard Biesheuvel 写道:
> On 21 March 2017 at 12:26, Chenhui Sun <chenhui.sun@linaro.org> wrote:

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

>> it to the first boot order.

>>

>> The feature works dependding on this patch and the OPP patch

>> "Hisilicon/D03/D05: get boot option from BMC" both be added.

>>

>> 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.

>>

>> so is there any solution? expect your comments.

>>

>> Signed-off-by: Huang ming <huangming23@linaro.org>

>> Signed-off-by: Chenhui Sun <chenhui.sun@linaro.org>

> I understand that this change is required to allow the boot order to

> be set via the BMC.

>

> But could you please explain why you need to make the changes below?

> Why do you have to change the way BootNext is handled?

Sorry for replying late.

In BdsEntry function, geting the BootNext Variable is before the calling 
of PlatformBdsPolicyBehavior(),
but the function ProductBdsPolicyAfterSetup  that get boot option type 
from BMC is called in the PlatformBdsPolicyBehavior  function.
The function ProductBdsPolicyAfterSetup  will update the BootNext variable.
We think that the operation of getting the BootNext variable should be 
in BdsBootDeviceSelect function, just before using the BootNext variable,
and the global variable mBootNext is no need.


Regards,
Chenhui

>> ---

>>   .../Universal/BdsDxe/BdsEntry.c                    | 44 ++++++++++++----------

>>   1 file changed, 24 insertions(+), 20 deletions(-)

>>

>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c

>> index bf81de4..ee51055 100644

>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c

>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c

>> @@ -36,7 +36,6 @@ EFI_BDS_ARCH_PROTOCOL  gBds = {

>>     BdsEntry

>>   };

>>

>> -UINT16                          *mBootNext = NULL;

>>

>>   ///

>>   /// The read-only variables defined in UEFI Spec.

>> @@ -123,6 +122,9 @@ BdsBootDeviceSelect (

>>     BOOLEAN           BootNextExist;

>>     LIST_ENTRY        *LinkBootNext;

>>     EFI_EVENT         ConnectConInEvent;

>> +  UINTN             BootNextSize;

>> +  UINT16            *BootNext = NULL;

>> +  UINT16            Index;

>>

>>     //

>>     // Got the latest boot option

>> @@ -154,7 +156,16 @@ BdsBootDeviceSelect (

>>       }

>>     }

>>

>> -  if (mBootNext != NULL) {

>> +  //

>> +  // Check if we have the boot next option

>> +  //

>> +  BootNext = BdsLibGetVariableAndSize (

>> +                L"BootNext",

>> +                &gEfiGlobalVariableGuid,

>> +                &BootNextSize

>> +                );

>> +

>> +  if (BootNext != NULL) {

>>       //

>>       // Indicate we have the boot next variable, so this time

>>       // boot will always have this boot option

>> @@ -179,17 +190,19 @@ BdsBootDeviceSelect (

>>       //

>>       // Add the boot next boot option

>>       //

>> -    UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", *mBootNext);

>> -    BootOption = BdsLibVariableToOption (&BootLists, Buffer);

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

>> +      UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", BootNext[Index]);

>> +      BootOption = BdsLibVariableToOption (&BootLists, Buffer);

>>

>> -    //

>> -    // If fail to get boot option from variable, just return and do nothing.

>> -    //

>> -    if (BootOption == NULL) {

>> -      return;

>> -    }

>> +      //

>> +      // If fail to get boot option from variable, just return and do nothing.

>> +      //

>> +      if (BootOption == NULL) {

>> +        return;

>> +      }

>>

>> -    BootOption->BootCurrent = *mBootNext;

>> +      BootOption->BootCurrent = BootNext[Index];

>> +    }

>>     }

>>     //

>>     // Parse the boot order to get boot option

>> @@ -528,7 +541,6 @@ BdsEntry (

>>   {

>>     LIST_ENTRY                      DriverOptionList;

>>     LIST_ENTRY                      BootOptionList;

>> -  UINTN                           BootNextSize;

>>     CHAR16                          *FirmwareVendor;

>>     EFI_STATUS                      Status;

>>     UINT16                          BootTimeOut;

>> @@ -638,14 +650,6 @@ BdsEntry (

>>     if (!IsListEmpty (&DriverOptionList)) {

>>       BdsLibLoadDrivers (&DriverOptionList);

>>     }

>> -  //

>> -  // Check if we have the boot next option

>> -  //

>> -  mBootNext = BdsLibGetVariableAndSize (

>> -                L"BootNext",

>> -                &gEfiGlobalVariableGuid,

>> -                &BootNextSize

>> -                );

>>

>>     //

>>     // Setup some platform policy here

>> --

>> 1.9.1

>>

Patch hide | download patch | download mbox

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index bf81de4..ee51055 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -36,7 +36,6 @@  EFI_BDS_ARCH_PROTOCOL  gBds = {
   BdsEntry
 };
 
-UINT16                          *mBootNext = NULL;
 
 ///
 /// The read-only variables defined in UEFI Spec.
@@ -123,6 +122,9 @@  BdsBootDeviceSelect (
   BOOLEAN           BootNextExist;
   LIST_ENTRY        *LinkBootNext;
   EFI_EVENT         ConnectConInEvent;
+  UINTN             BootNextSize;
+  UINT16            *BootNext = NULL;
+  UINT16            Index;
 
   //
   // Got the latest boot option
@@ -154,7 +156,16 @@  BdsBootDeviceSelect (
     }
   }
 
-  if (mBootNext != NULL) {
+  //
+  // Check if we have the boot next option
+  //
+  BootNext = BdsLibGetVariableAndSize (
+                L"BootNext",
+                &gEfiGlobalVariableGuid,
+                &BootNextSize
+                );
+
+  if (BootNext != NULL) {
     //
     // Indicate we have the boot next variable, so this time
     // boot will always have this boot option
@@ -179,17 +190,19 @@  BdsBootDeviceSelect (
     //
     // Add the boot next boot option
     //
-    UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", *mBootNext);
-    BootOption = BdsLibVariableToOption (&BootLists, Buffer);
+    for (Index = 0; Index < (BootNextSize/sizeof(UINT16)); Index++) {
+      UnicodeSPrint (Buffer, sizeof (Buffer), L"Boot%04x", BootNext[Index]);
+      BootOption = BdsLibVariableToOption (&BootLists, Buffer);
 
-    //
-    // If fail to get boot option from variable, just return and do nothing.
-    //
-    if (BootOption == NULL) {
-      return;
-    }
+      //
+      // If fail to get boot option from variable, just return and do nothing.
+      //
+      if (BootOption == NULL) {
+        return;
+      }
 
-    BootOption->BootCurrent = *mBootNext;
+      BootOption->BootCurrent = BootNext[Index];
+    }
   }
   //
   // Parse the boot order to get boot option
@@ -528,7 +541,6 @@  BdsEntry (
 {
   LIST_ENTRY                      DriverOptionList;
   LIST_ENTRY                      BootOptionList;
-  UINTN                           BootNextSize;
   CHAR16                          *FirmwareVendor;
   EFI_STATUS                      Status;
   UINT16                          BootTimeOut;
@@ -638,14 +650,6 @@  BdsEntry (
   if (!IsListEmpty (&DriverOptionList)) {
     BdsLibLoadDrivers (&DriverOptionList);
   }
-  //
-  // Check if we have the boot next option
-  //
-  mBootNext = BdsLibGetVariableAndSize (
-                L"BootNext",
-                &gEfiGlobalVariableGuid,
-                &BootNextSize
-                );
 
   //
   // Setup some platform policy here