Re: 回复: [ ** SPAMMAIL ** ]Re: [edk2-devel] [edk2][PATCH v2 1/1] MdeModulePkg: Add EDKII Platform Boot Manager Protocol v2

["Tinh Nguyen" <tinhnguyen@os.amperecomputing.com>]

+ Leif, as the maintainer of ARM Platforms

Hi Liming,

I think about it, too. But like I said with Ray (in private mail) "I did 
not put it in PlatformBootManagerLib because this library is being 
generalized by Arm and

not every Arm platform needs to change the BootOrder. By modifying this 
protocol, we can avoid affecting existing platforms and ensure that the 
boot order is sorted every time we call RefreshBootOption."

Let's check the code, to use EfiBootManagerSortLoadOptionVariable(), 
platform must provide the CompareFunction().

How does 
https://github.com/tianocore/edk2/blob/master/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c 
provide a hook "CompareFunction" for platforms?

1. Library: all Arm-platforms that are currently use 
PlatformBootManagerLib need modify

2. Protocol: I don't think it is a good idea for Arm introducing new 
protocol for BDS phase, which is very specific for Arm.

Liming, I think this change is just a minor change. That provides 
another way to help platforms modify DriverOrder/BootOrder, doesn’t 
break the old design or break the old platforms


Leif, would you mind sharing your idea about it. Can we introduce a new 
library in ArmPkg to modify DriverOrder/BootOrder?

Regards,

Tinh Nguyen


On 19/04/2023 10:25, gaoliming wrote:
> Tinh:
>   By design, EfiBootManagerRefreshAllBootOption() is used to find the available boot options; EfiBootManagerSortLoadOptionVariable() is used to sort the boot option order. They are both called in PlatformBootManagerLib. So, I think you should handle this issue in PlatformBootManagerLib.
>
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人:devel@edk2.groups.io  <devel@edk2.groups.io>  代表 Tinh Nguyen
>> via groups.io
>> 发送时间: 2023年4月18日 12:25
>> 收件人: Ni, Ray<ray.ni@intel.com>;devel@edk2.groups.io;
>> tinhnguyen@os.amperecomputing.com
>> 抄送:patches@amperecomputing.com;nhi@os.amperecomputing.com;
>> chuong@os.amperecomputing.com;minhnguyen@os.amperecomputing.com;
>> Wang, Jian J<jian.j.wang@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>; Gao, Zhichao<zhichao.gao@intel.com>;
>> rebecca@bsdio.com
>> 主题: [ ** SPAMMAIL ** ]Re: [edk2-devel] [edk2][PATCH v2 1/1]
>> MdeModulePkg: Add EDKII Platform Boot Manager Protocol v2
>>
>> Hi Ray,
>>
>> Many thanks for your feedback.
>>
>> Yes, but it is just for Boot Options that are automatically created by
>> BDS. If users/OS input 2, 3, 4 … boot options without option data
>> (mBmAutoCreateBootOptionGuid), the RefreshAllBootOptions() API should
>> not modify/delete them, right? Users will be confused when their Boot
>> Options change. And I also cannot sort them.
>>
>> And the RefreshAllBootOptions() API just provides a Boot Options List;
>> it doesn’t directly write those to NVRAM. BDS has its rules and then
>> selects a Boot option from that list to write into NVRAM.
>> After BDS has done its work, we can confirm what exact boot options we
>> have. This is a suitable time to sort the Boot Options and write this
>> sorting to the BootOrder.
>>
>> Another aspect is that trying to put everything into the
>> RefreshAllBootOptions() API makes this function very complex and hard to
>> maintain.
>>
>> Please, let me know your idea.
>>
>> Thanks
>>
>> -Tinh
>>
>>
>> On 4/18/2023 8:24 AM, Ni, Ray wrote:
>>> RefreshAllBootOptions() API internally can keep an ordering rule so that
>> platform-level-higher-priority boot options are before
>>> those platform-level-lower-priority options.
>>>
>>> So, that means, you could only change RefreshAllOptions() implementation
>> to control the order without adding a new API.
>>> Thanks,
>>> ray
>>>
>>>> -----Original Message-----
>>>> From:devel@edk2.groups.io  <devel@edk2.groups.io>  On Behalf Of Tinh
>>>> Nguyen via groups.io
>>>> Sent: Tuesday, April 11, 2023 1:35 AM
>>>> To:devel@edk2.groups.io;tinhnguyen@os.amperecomputing.com
>>>> Cc:patches@amperecomputing.com;nhi@os.amperecomputing.com;
>>>> chuong@os.amperecomputing.com;
>>>> minhnguyen@os.amperecomputing.com; Wang, Jian J
>>>> <jian.j.wang@intel.com>; Gao, Liming<gaoliming@byosoft.com.cn>; Gao,
>>>> Zhichao<zhichao.gao@intel.com>; Ni, Ray<ray.ni@intel.com>;
>>>> rebecca@bsdio.com
>>>> Subject: Re: [edk2-devel] [edk2][PATCH v2 1/1] MdeModulePkg: Add EDKII
>>>> Platform Boot Manager Protocol v2
>>>>
>>>> Gentle ping, I’m looking for some feedback on this patch. Could someone
>>>> please help me to review it?
>>>>
>>>> On 4/6/2023 3:19 PM, Tinh Nguyen via groups.io wrote:
>>>>> This introduces the
>>>> EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION2,
>>>>> which adds a new UpdateBootOrder() function to support customizing
>>>>> the boot options order according to the platform-specific policy.
>>>>>
>>>>> Signed-off-by: Tinh Nguyen<tinhnguyen@os.amperecomputing.com>
>>>>> ---
>>>>>
>>>>> Changes since v1: Correct the format of the email.
>>>>>
>>>>>     MdeModulePkg/Include/Protocol/PlatformBootManager.h | 24
>>>> +++++++++++++++++++-
>>>>>     MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c    |  9
>> ++++++++
>>>>>     2 files changed, 32 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/MdeModulePkg/Include/Protocol/PlatformBootManager.h
>>>> b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
>>>>> index e527b0ee0eaf..758bc2deb774 100644
>>>>> --- a/MdeModulePkg/Include/Protocol/PlatformBootManager.h
>>>>> +++ b/MdeModulePkg/Include/Protocol/PlatformBootManager.h
>>>>> @@ -1,6 +1,7 @@
>>>>>     /** @file
>>>>>
>>>>>       Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
>>>>> +  Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>>>>>
>>>>>       SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>>
>>>>> @@ -29,7 +30,8 @@ typedef struct
>>>> _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL
>>>> EDKII_PLATFORM_BOOT_MANAGER
>>>>>     //          All future revisions must be backwards compatible.
>>>>>     //          If a future version is not back wards compatible it is
>> not the same
>>>> GUID.
>>>>>     //
>>>>> -#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION
>>>> 0x00000001
>>>>> +#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION
>>>> 0x00000001
>>>>> +#define EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION2
>>>> 0x00000002
>>>>>     //
>>>>>     // Function Prototypes
>>>>> @@ -72,9 +74,29 @@ EFI_STATUS
>>>>>       OUT       UINTN
>> *UpdatedBootOptionsCount
>>>>>       );
>>>>>
>>>>> +/**
>>>>> +  This function allows platform to update the DriverOrder/BootOrder
>>>> variables.
>>>>> +  And it is available from version 2 of the EDKII Platform Boot Manager
>>>> protocol.
>>>>> +
>>>>> +  @retval EFI_SUCCESS  Platform successfully modifies
>>>>> +                       the DriverOrder/BootOrder variables as
>> wanted.
>>>>> +  @retval Others       There are some errors that happen. Check
>> the status
>>>> code
>>>>> +                       for details.
>>>>> +
>>>>> +**/
>>>>> +typedef
>>>>> +EFI_STATUS
>>>>> +(EFIAPI *PLATFORM_BOOT_MANAGER_UPDATE_BOOT_ORDER)(
>>>>> +  IN VOID
>>>>> +  );
>>>>> +
>>>>>     struct _EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL {
>>>>>       UINT64
>> Revision;
>>>>>       PLATFORM_BOOT_MANAGER_REFRESH_ALL_BOOT_OPTIONS
>>>> RefreshAllBootOptions;
>>>>> +  //
>>>>> +  // EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION2
>>>>> +  //
>>>>> +  PLATFORM_BOOT_MANAGER_UPDATE_BOOT_ORDER
>>>> UpdateBootOrder;
>>>>>     };
>>>>>
>>>>>     extern EFI_GUID  gEdkiiPlatformBootManagerProtocolGuid;
>>>>> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>> b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> index bde22fa6590e..67d1d54b3c24 100644
>>>>> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>>>>> @@ -4,6 +4,7 @@
>>>>>     Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved.
>>>>>     Copyright (c) 2011 - 2021, Intel Corporation. All rights reserved.<BR>
>>>>>     (C) Copyright 2015-2021 Hewlett Packard Enterprise Development
>> LP<BR>
>>>>> +Copyright (c) 2023, Ampere Computing LLC. All rights reserved.<BR>
>>>>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>>
>>>>>     **/
>>>>> @@ -2412,6 +2413,8 @@ EfiBootManagerRefreshAllBootOption (
>>>>>           BootOptions     = UpdatedBootOptions;
>>>>>           BootOptionCount = UpdatedBootOptionCount;
>>>>>         }
>>>>> +  } else {
>>>>> +    PlatformBootManager = NULL;
>>>>>       }
>>>>>
>>>>>       NvBootOptions = EfiBootManagerGetLoadOptions
>>>> (&NvBootOptionCount, LoadOptionTypeBoot);
>>>>> @@ -2453,6 +2456,12 @@ EfiBootManagerRefreshAllBootOption (
>>>>>
>>>>>       EfiBootManagerFreeLoadOptions (BootOptions,
>> BootOptionCount);
>>>>>       EfiBootManagerFreeLoadOptions (NvBootOptions,
>> NvBootOptionCount);
>>>>> +
>>>>> +  if ((PlatformBootManager != NULL) &&
>>>>> +      (PlatformBootManager->Revision >=
>>>> EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL_REVISION2))
>>>>> +  {
>>>>> +    PlatformBootManager->UpdateBootOrder ();
>>>>> +  }
>>>>>     }
>>>>>
>>>>>     /**
>>>>> --
>>>>> 2.40.0
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>> 
>>