Re: [PATCH v3 1/2] EmbeddedPkg: add platform boot manager protocol

[Laszlo Ersek <lersek@redhat.com>]

Hello Haojian,

On 04/19/18 12:42, Haojian Zhuang wrote:
> Create the PlatformBootManagerProtocol that is used to add
> predefined boot options in platform driver. This interface
> will be used in ArmPkg/PlatformBootManagerLib.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                        |  1 +
>  EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 46 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index eb135340b173..9cd50738363b 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -81,6 +81,7 @@ [Protocols.common]
>    gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }}
>    gAndroidBootImgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }}
>    gPlatformVirtualKeyboardProtocolGuid = { 0x0e3606d2, 0x1dc3, 0x4e6f, { 0xbe, 0x65, 0x39, 0x49, 0x82, 0xa2, 0x65, 0x47 }}
> +  gPlatformBootManagerProtocolGuid = { 0x7197c8a7, 0x6559, 0x4c93, { 0x93, 0xd5, 0x8a, 0x84, 0xf9, 0x88, 0x79, 0x8b }}
>
>  [Ppis]
>    gEdkiiEmbeddedGpioPpiGuid = { 0x21c3b115, 0x4e0b, 0x470c, { 0x85, 0xc7, 0xe1, 0x05, 0xa5, 0x75, 0xc9, 0x7b }}
> diff --git a/EmbeddedPkg/Include/Protocol/PlatformBootManager.h b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
> new file mode 100644
> index 000000000000..8fd63c8b45d6
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
> @@ -0,0 +1,46 @@
> +/** @file
> +
> +  Copyright (c) 2018, Linaro. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +#define __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +
> +//
> +// Protocol interface structure
> +//
> +typedef struct _PLATFORM_BOOT_MANAGER_PROTOCOL    PLATFORM_BOOT_MANAGER_PROTOCOL;
> +
> +//
> +// Function Prototypes
> +//
> +
> +/*
> +  Get predefined boot options for platform.
> +
> +  @param[out] BootOptions   An array of predefined boot options.
> +  @param[out] BootKeys      An array of predefined keys.
> +*/
> +typedef
> +EFI_STATUS
> +(EFIAPI *GET_PLATFORM_BOOT_OPTIONS_AND_KEYS) (
> +  OUT EFI_BOOT_MANAGER_LOAD_OPTION       **BootOptions,
> +  OUT EFI_INPUT_KEY                      **BootKeys
> +  );
> +
> +struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
> +  GET_PLATFORM_BOOT_OPTIONS_AND_KEYS     GetPlatformBootOptionsAndKeys;
> +};
> +
> +extern EFI_GUID gPlatformBootManagerProtocolGuid;
> +
> +#endif /* __PLATFORM_BOOT_MANAGER_PROTOCOL_H__ */
>

this looks good generally, but the protocol documentation is still
seriously lacking. The documentation should enable anyone to call the
member and know everything about the intended use and lifecycle of the
output parameters.

Also, because the two output arrays are supposed to be used in tandem, I
suggest that we replace the terminator zero element with an explicit
element count (a UINTN output parameter). For example:

  @param[out] Count         The number of elements in each of
                            BootOptions and BootKeys. On successful
                            return, Count is at least one.

  @param[out] BootOptions   An array of platform boot options.
                            BootOptions is pool-allocated by
                            GET_PLATFORM_BOOT_OPTIONS_AND_KEYS, and
                            GET_PLATFORM_BOOT_OPTIONS_AND_KEYS populates
                            every element in BootOptions with
                            EfiBootManagerInitializeLoadOption().
                            BootOptions shall not contain duplicate
                            entries. The caller is responsible for
                            releasing BootOptions after use with
                            EfiBootManagerFreeLoadOptions().

  @param[out] BootKeys      A pool-allocated array of platform boot
                            hotkeys. For every 0 <= Index < Count, If
                            BootOptions[Index] is not to be associated
                            with a hotkey, then BootKeys[Index] is
                            zero-filled. Otherwise, BootKeys[Index]
                            specifies the boot hotkey for
                            BootOptions[Index]. BootKeys shall not
                            contain duplicate entries (other than
                            zero-filled entries). The caller is
                            responsible for releasing BootKeys with
                            FreePool() after use.

  @retval EFI_SUCCESS           Count, BootOptions and BootKeys have
                                been set.

  @retval EFI_OUT_OF_RESOURCES  Memory allocation failed.

  @retval EFI_UNSUPPORTED       The platform doesn't provide boot
                                options as a feature.

  @retval EFI_NOT_FOUND         The platform could provide boot options
                                as a feature, but none have been
                                configured.

  @return                       Error codes propagated from underlying
                                functions.

Thanks
Laszlo