[v5] add platform boot options

[edk2] [PATCH edk2-platforms v5 0/2] add platform boot options

Posted by Haojian Zhuang 5 years, 12 months ago

Changelog:
v5:
  * Avoid to merge device path and grub's file path in driver.
    Merge them directly in DSC file.
  * Avoid duplicated code to create boot options.
  * Use goto to handle error condition.
v4:
  * Add BootCount parameter in the interface.
v3:
  * Move in initilization of boot entry.
  * Update the name of interface in platform boot manager protocol.
v2:
  * Update with platform boot manager protocol.

Haojian Zhuang (2):
  Platform/HiKey960: register predefined boot options
  Platform/HiKey: create 4 boot options

 Platform/Hisilicon/HiKey/HiKey.dec                 |   8 +-
 Platform/Hisilicon/HiKey/HiKey.dsc                 |   7 +
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c       | 171 ++++++++++++++++++++
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf     |  11 ++
 .../{HiKey/HiKey.dec => HiKey960/HiKey960.dec}     |  17 +-
 Platform/Hisilicon/HiKey960/HiKey960.dsc           |   6 +
 .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c   | 172 +++++++++++++++++++++
 .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf |  11 ++
 8 files changed, 391 insertions(+), 12 deletions(-)
 copy Platform/Hisilicon/{HiKey/HiKey.dec => HiKey960/HiKey960.dec} (56%)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms v5 0/2] add platform boot options

Posted by Laszlo Ersek 5 years, 12 months ago

Hello Haojian,

On 04/25/18 06:59, Haojian Zhuang wrote:
> Changelog:
> v5:
>   * Avoid to merge device path and grub's file path in driver.
>     Merge them directly in DSC file.
>   * Avoid duplicated code to create boot options.
>   * Use goto to handle error condition.
> v4:
>   * Add BootCount parameter in the interface.
> v3:
>   * Move in initilization of boot entry.
>   * Update the name of interface in platform boot manager protocol.
> v2:
>   * Update with platform boot manager protocol.
> 
> Haojian Zhuang (2):
>   Platform/HiKey960: register predefined boot options
>   Platform/HiKey: create 4 boot options
> 
>  Platform/Hisilicon/HiKey/HiKey.dec                 |   8 +-
>  Platform/Hisilicon/HiKey/HiKey.dsc                 |   7 +
>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c       | 171 ++++++++++++++++++++
>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf     |  11 ++
>  .../{HiKey/HiKey.dec => HiKey960/HiKey960.dec}     |  17 +-
>  Platform/Hisilicon/HiKey960/HiKey960.dsc           |   6 +
>  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c   | 172 +++++++++++++++++++++
>  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf |  11 ++
>  8 files changed, 391 insertions(+), 12 deletions(-)
>  copy Platform/Hisilicon/{HiKey/HiKey.dec => HiKey960/HiKey960.dec} (56%)
> 

(1) in the future, please CC the blurb -- patch 0/xxx -- to everyone who
are CC'd on any of the patches. What I usually do is:

- format all the patches
- grep them for '^Cc:'
- sort the resultant list uniquely
- paste the output into the blurb message

This way, if a person is CC'd at least on one of the patches in the
series, they will receive a copy of the cover letter too.

(2) As of commit b581a4934d8f ("ARM/JunoPkg: Add HDLCD platform
library", 2018-04-23), I cannot find HiKeyDxe or HiKey960Dxe in the
edk2-platforms tree. That's not a problem, it just means that I cannot
verify the resource management / error handling in the
HiKey960EntryPoint() and HiKeyEntryPoint() functions. For this reason, I
cannot give R-b, just A-b. I hope that will suffice -- please do not
repost because of this.

(3) The GRUB_FILE_NAME macro is now superfluous in both of the patches.
Again, it's not necessary for me to review the series just because of
such an update. Perhaps Leif or Ard can remove the macro defs for you
when they push the patches.

(4) In both patches, in both of the CreatePlatformBootOptionFromPath()
and CreatePlatformBootOptionFromGuid() helper functions, the
"DevicePath" variable is leaked when
EfiBootManagerInitializeLoadOption() fails.

"DevicePath" should be freed unconditionally at that point, in both
patches and in both helper functions (4 instances in total). Simply
eliminate the following:

  if (EFI_ERROR (Status)) {
    return Status;
  }

and you will be left with:

  Status = EfiBootManagerInitializeLoadOption (
             ...
             );
  FreePool (DevicePath);
  return Status;

I believe I need not separately review this update either.

With (3) and (4) addressed, for the series:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms v5 0/2] add platform boot options

Posted by Haojian Zhuang 5 years, 12 months ago

On 26 April 2018 at 00:10, Laszlo Ersek <lersek@redhat.com> wrote:
> Hello Haojian,
>
> On 04/25/18 06:59, Haojian Zhuang wrote:
>> Changelog:
>> v5:
>>   * Avoid to merge device path and grub's file path in driver.
>>     Merge them directly in DSC file.
>>   * Avoid duplicated code to create boot options.
>>   * Use goto to handle error condition.
>> v4:
>>   * Add BootCount parameter in the interface.
>> v3:
>>   * Move in initilization of boot entry.
>>   * Update the name of interface in platform boot manager protocol.
>> v2:
>>   * Update with platform boot manager protocol.
>>
>> Haojian Zhuang (2):
>>   Platform/HiKey960: register predefined boot options
>>   Platform/HiKey: create 4 boot options
>>
>>  Platform/Hisilicon/HiKey/HiKey.dec                 |   8 +-
>>  Platform/Hisilicon/HiKey/HiKey.dsc                 |   7 +
>>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c       | 171 ++++++++++++++++++++
>>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf     |  11 ++
>>  .../{HiKey/HiKey.dec => HiKey960/HiKey960.dec}     |  17 +-
>>  Platform/Hisilicon/HiKey960/HiKey960.dsc           |   6 +
>>  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c   | 172 +++++++++++++++++++++
>>  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf |  11 ++
>>  8 files changed, 391 insertions(+), 12 deletions(-)
>>  copy Platform/Hisilicon/{HiKey/HiKey.dec => HiKey960/HiKey960.dec} (56%)
>>
>
> (1) in the future, please CC the blurb -- patch 0/xxx -- to everyone who
> are CC'd on any of the patches. What I usually do is:
>
> - format all the patches
> - grep them for '^Cc:'
> - sort the resultant list uniquely
> - paste the output into the blurb message
>
> This way, if a person is CC'd at least on one of the patches in the
> series, they will receive a copy of the cover letter too.
>
OK

>
> (2) As of commit b581a4934d8f ("ARM/JunoPkg: Add HDLCD platform
> library", 2018-04-23), I cannot find HiKeyDxe or HiKey960Dxe in the
> edk2-platforms tree. That's not a problem, it just means that I cannot
> verify the resource management / error handling in the
> HiKey960EntryPoint() and HiKeyEntryPoint() functions. For this reason, I
> cannot give R-b, just A-b. I hope that will suffice -- please do not
> repost because of this.
>

Yes, it's depend on another patch series on virtual keyboard.

>
> (3) The GRUB_FILE_NAME macro is now superfluous in both of the patches.
> Again, it's not necessary for me to review the series just because of
> such an update. Perhaps Leif or Ard can remove the macro defs for you
> when they push the patches.
>

OK. I'll remove it.

>
> (4) In both patches, in both of the CreatePlatformBootOptionFromPath()
> and CreatePlatformBootOptionFromGuid() helper functions, the
> "DevicePath" variable is leaked when
> EfiBootManagerInitializeLoadOption() fails.
>
> "DevicePath" should be freed unconditionally at that point, in both
> patches and in both helper functions (4 instances in total). Simply
> eliminate the following:
>
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }
>
> and you will be left with:
>
>   Status = EfiBootManagerInitializeLoadOption (
>              ...
>              );
>   FreePool (DevicePath);
>   return Status;
>
> I believe I need not separately review this update either.
>

For this one, I have a different opinion.

EFI_STATUS
EFIAPI
EfiBootManagerInitializeLoadOption (
  ...
  )
{
  if ((Option == NULL) || (Description == NULL) || (FilePath == NULL)) {
    return EFI_INVALID_PARAMETER;
  }

  if (((OptionalData != NULL) && (OptionalDataSize == 0)) ||
      ((OptionalData == NULL) && (OptionalDataSize != 0))) {
    return EFI_INVALID_PARAMETER;
  }
  if ((UINT32) OptionType >= LoadOptionTypeMax) {
    return EFI_INVALID_PARAMETER;
  }

  ZeroMem (Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION));
  Option->OptionNumber       = OptionNumber;
  Option->OptionType         = OptionType;
  Option->Attributes         = Attributes;
  Option->Description        = AllocateCopyPool (StrSize
(Description), Description);
  Option->FilePath           = DuplicateDevicePath (FilePath);
  if (OptionalData != NULL) {
    Option->OptionalData     = AllocateCopyPool (OptionalDataSize,
OptionalData);
    Option->OptionalDataSize = OptionalDataSize;
  }

  return EFI_SUCCESS;
}

We can find that no memory is allocated to "DevicePath" if it returns
with error.

So we shouldn't free memory on "DevicePath" if "Status" is error code.

Other issues will be fixed in v6.

>
> With (3) and (4) addressed, for the series:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks a lot.

Best Regards
Haojian
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms v5 0/2] add platform boot options

Posted by Laszlo Ersek 5 years, 12 months ago

On 04/26/18 05:12, Haojian Zhuang wrote:
> On 26 April 2018 at 00:10, Laszlo Ersek <lersek@redhat.com> wrote:

>> (4) In both patches, in both of the CreatePlatformBootOptionFromPath()
>> and CreatePlatformBootOptionFromGuid() helper functions, the
>> "DevicePath" variable is leaked when
>> EfiBootManagerInitializeLoadOption() fails.
>>
>> "DevicePath" should be freed unconditionally at that point, in both
>> patches and in both helper functions (4 instances in total). Simply
>> eliminate the following:
>>
>>   if (EFI_ERROR (Status)) {
>>     return Status;
>>   }
>>
>> and you will be left with:
>>
>>   Status = EfiBootManagerInitializeLoadOption (
>>              ...
>>              );
>>   FreePool (DevicePath);
>>   return Status;
>>
>> I believe I need not separately review this update either.
>>
> 
> For this one, I have a different opinion.
> 
> EFI_STATUS
> EFIAPI
> EfiBootManagerInitializeLoadOption (
>   ...
>   )
> {
>   if ((Option == NULL) || (Description == NULL) || (FilePath == NULL)) {
>     return EFI_INVALID_PARAMETER;
>   }
> 
>   if (((OptionalData != NULL) && (OptionalDataSize == 0)) ||
>       ((OptionalData == NULL) && (OptionalDataSize != 0))) {
>     return EFI_INVALID_PARAMETER;
>   }
>   if ((UINT32) OptionType >= LoadOptionTypeMax) {
>     return EFI_INVALID_PARAMETER;
>   }
> 
>   ZeroMem (Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION));
>   Option->OptionNumber       = OptionNumber;
>   Option->OptionType         = OptionType;
>   Option->Attributes         = Attributes;
>   Option->Description        = AllocateCopyPool (StrSize
> (Description), Description);
>   Option->FilePath           = DuplicateDevicePath (FilePath);
>   if (OptionalData != NULL) {
>     Option->OptionalData     = AllocateCopyPool (OptionalDataSize,
> OptionalData);
>     Option->OptionalDataSize = OptionalDataSize;
>   }
> 
>   return EFI_SUCCESS;
> }
> 
> We can find that no memory is allocated to "DevicePath" if it returns
> with error.
> 
> So we shouldn't free memory on "DevicePath" if "Status" is error code.

I disagree. We have:

  CreatePlatformBootOptionFromPath()
    DevicePath = ConvertTextToDevicePath(...)              #1
    EfiBootManagerInitializeLoadOption(... DevicePath ...)
      DuplicateDevicePath(FilePath=DevicePath)             #2

The ConvertTextToDevicePath() function returns the binary representation
of the device path in a dynamically allocated area. That is dynamic
object #1, tracked by the "DevicePath" variable. Then,
EfiBootManagerInitializeLoadOption() creates a deep copy, by calling the
DuplicateDevicePath() function. That creates dynamic object #2, tracked
by Option->FilePath.

- If EfiBootManagerInitializeLoadOption() returns with failure, then
dynamic object #2 does not exist, and we no longer need dynamic object
#1, so we have to free dynamic object #1.

- If EfiBootManagerInitializeLoadOption() returns with success, then
dynamic object #2 exists, and is correctly tracked by Option->FilePath.
We no longer need dynamic object #1, so we have to free it.

In other words, regardless of the return status of
EfiBootManagerInitializeLoadOption(), we must release dynamic object #1.
We need dynamic object #1 only temporarily, so we can call
EfiBootManagerInitializeLoadOption(), and let it make a copy.

The same applies to CreatePlatformBootOptionFromGuid(), except replace
ConvertTextToDevicePath() with AppendDevicePathNode():

  CreatePlatformBootOptionFromGuid()
    DevicePath = AppendDevicePathNode(...)                  #1
    EfiBootManagerInitializeLoadOption (... DevicePath ...)
      DuplicateDevicePath(FilePath=DevicePath)              #2

Again, AppendDevicePathNode() produces dynamic object #1, similarly to
ConvertTextToDevicePath().

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel