[U-Boot] [PATCH v2 01/12] efi_loader: refactor efi_open_protocol

Rob Clark robdclark at gmail.com
Mon Jul 24 09:48:23 UTC 2017


On Tue, Jul 11, 2017 at 4:06 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> efi_open_protocol was implemented to call a protocol specific open
> function to retrieve the protocol interface.
>
> The UEFI specification does not know of such a function.
>
> It is not possible to implement InstallProtocolInterface with the
> current design.
>
> With the patch the protocol interface itself is stored in the list
> of installed protocols of an efi_object instead of an open function.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> v2
>         Remove implementation of efi_return_handle.
> ---
>  cmd/bootefi.c                     | 14 +++-----------
>  include/efi_loader.h              | 17 ++---------------
>  lib/efi_loader/efi_boottime.c     | 18 ++++++++++++++----
>  lib/efi_loader/efi_disk.c         | 29 +++--------------------------
>  lib/efi_loader/efi_gop.c          |  2 +-
>  lib/efi_loader/efi_image_loader.c |  8 --------
>  lib/efi_loader/efi_net.c          | 30 +++---------------------------
>  7 files changed, 26 insertions(+), 92 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 7ddeead671..7cf0129a18 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -54,14 +54,6 @@ static struct efi_device_path_file_path bootefi_device_path[] = {
>         }
>  };
>
> -static efi_status_t EFIAPI bootefi_open_dp(void *handle, efi_guid_t *protocol,
> -                       void **protocol_interface, void *agent_handle,
> -                       void *controller_handle, uint32_t attributes)
> -{
> -       *protocol_interface = bootefi_device_path;
> -       return EFI_SUCCESS;
> -}
> -
>  /* The EFI loaded_image interface for the image executed via "bootefi" */
>  static struct efi_loaded_image loaded_image_info = {
>         .device_handle = bootefi_device_path,
> @@ -78,7 +70,7 @@ static struct efi_object loaded_image_info_obj = {
>                          * return handle which points to loaded_image_info
>                          */
>                         .guid = &efi_guid_loaded_image,
> -                       .open = &efi_return_handle,
> +                       .protocol_interface = &loaded_image_info,

btw, I probably should have looked at this patchset earlier.. in
general, I like it, since it removes a lot of boilerplate.  But there
are some cases where ->open() allows deferred allocation.  For
example, I'm working on efi_file and simple-file-system-protocol
implementation.  A file object can have a device-path accessed by
opening device-path protocol.  99% of the time this isn't used, so the
->open() approach lets me defer constructing a file's device-path.

How would you feel if I re-introduced ->open() as an optional thing
(where the normal case if open is NULL would be to just return
protocol_interface)?

BR,
-R


>                 },
>                 {
>                         /*
> @@ -86,7 +78,7 @@ static struct efi_object loaded_image_info_obj = {
>                          * bootefi_device_path
>                          */
>                         .guid = &efi_guid_device_path,
> -                       .open = &bootefi_open_dp,
> +                       .protocol_interface = bootefi_device_path,
>                 },
>         },
>  };
> @@ -99,7 +91,7 @@ static struct efi_object bootefi_device_obj = {
>                         /* When asking for the device path interface, return
>                          * bootefi_device_path */
>                         .guid = &efi_guid_device_path,
> -                       .open = &bootefi_open_dp,
> +                       .protocol_interface = bootefi_device_path
>                 }
>         },
>  };
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 99619f53a9..c620652307 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -49,15 +49,10 @@ struct efi_class_map {
>  /*
>   * When the UEFI payload wants to open a protocol on an object to get its
>   * interface (usually a struct with callback functions), this struct maps the
> - * protocol GUID to the respective protocol handler open function for that
> - * object protocol combination.
> - */
> + * protocol GUID to the respective protocol interface */
>  struct efi_handler {
>         const efi_guid_t *guid;
> -       efi_status_t (EFIAPI *open)(void *handle,
> -                       efi_guid_t *protocol, void **protocol_interface,
> -                       void *agent_handle, void *controller_handle,
> -                       uint32_t attributes);
> +       void *protocol_interface;
>  };
>
>  /*
> @@ -91,14 +86,6 @@ void efi_smbios_register(void);
>  /* Called by networking code to memorize the dhcp ack package */
>  void efi_net_set_dhcp_ack(void *pkt, int len);
>
> -/*
> - * Stub implementation for a protocol opener that just returns the handle as
> - * interface
> - */
> -efi_status_t EFIAPI efi_return_handle(void *handle,
> -               efi_guid_t *protocol, void **protocol_interface,
> -               void *agent_handle, void *controller_handle,
> -               uint32_t attributes);
>  /* Called from places to check whether a timer expired */
>  void efi_timer_check(void);
>  /* PE loader implementation */
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 2ecec60281..0f67dc2037 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -443,7 +443,6 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>                 .protocols = {
>                         {
>                                 .guid = &efi_guid_loaded_image,
> -                               .open = &efi_return_handle,
>                         },
>                 },
>         };
> @@ -453,6 +452,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>         EFI_ENTRY("%d, %p, %p, %p, %ld, %p", boot_policy, parent_image,
>                   file_path, source_buffer, source_size, image_handle);
>         info = malloc(sizeof(*info));
> +       loaded_image_info_obj.protocols[0].protocol_interface = info;
>         obj = malloc(sizeof(loaded_image_info_obj));
>         memset(info, 0, sizeof(*info));
>         memcpy(obj, &loaded_image_info_obj, sizeof(loaded_image_info_obj));
> @@ -724,6 +724,13 @@ static efi_status_t EFIAPI efi_open_protocol(
>         EFI_ENTRY("%p, %p, %p, %p, %p, 0x%x", handle, protocol,
>                   protocol_interface, agent_handle, controller_handle,
>                   attributes);
> +
> +       if (!protocol_interface && attributes !=
> +           EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> +               r = EFI_INVALID_PARAMETER;
> +               goto out;
> +       }
> +
>         list_for_each(lhandle, &efi_obj_list) {
>                 struct efi_object *efiobj;
>                 efiobj = list_entry(lhandle, struct efi_object, link);
> @@ -737,9 +744,12 @@ static efi_status_t EFIAPI efi_open_protocol(
>                         if (!hprotocol)
>                                 break;
>                         if (!guidcmp(hprotocol, protocol)) {
> -                               r = handler->open(handle, protocol,
> -                                   protocol_interface, agent_handle,
> -                                   controller_handle, attributes);
> +                               if (attributes !=
> +                                   EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> +                                       *protocol_interface =
> +                                               handler->protocol_interface;
> +                               }
> +                               r = EFI_SUCCESS;
>                                 goto out;
>                         }
>                 }
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 4e8e7d0ad6..7f8970496f 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -35,29 +35,6 @@ struct efi_disk_obj {
>         const struct blk_desc *desc;
>  };
>
> -static efi_status_t EFIAPI efi_disk_open_block(void *handle,
> -                       efi_guid_t *protocol, void **protocol_interface,
> -                       void *agent_handle, void *controller_handle,
> -                       uint32_t attributes)
> -{
> -       struct efi_disk_obj *diskobj = handle;
> -
> -       *protocol_interface = &diskobj->ops;
> -
> -       return EFI_SUCCESS;
> -}
> -
> -static efi_status_t EFIAPI efi_disk_open_dp(void *handle, efi_guid_t *protocol,
> -                       void **protocol_interface, void *agent_handle,
> -                       void *controller_handle, uint32_t attributes)
> -{
> -       struct efi_disk_obj *diskobj = handle;
> -
> -       *protocol_interface = diskobj->dp;
> -
> -       return EFI_SUCCESS;
> -}
> -
>  static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this,
>                         char extended_verification)
>  {
> @@ -210,10 +187,11 @@ static void efi_disk_add_dev(const char *name,
>         diskobj = calloc(1, objlen);
>
>         /* Fill in object data */
> +       dp = (void *)&diskobj[1];
>         diskobj->parent.protocols[0].guid = &efi_block_io_guid;
> -       diskobj->parent.protocols[0].open = efi_disk_open_block;
> +       diskobj->parent.protocols[0].protocol_interface = &diskobj->ops;
>         diskobj->parent.protocols[1].guid = &efi_guid_device_path;
> -       diskobj->parent.protocols[1].open = efi_disk_open_dp;
> +       diskobj->parent.protocols[1].protocol_interface = dp;
>         diskobj->parent.handle = diskobj;
>         diskobj->ops = block_io_disk_template;
>         diskobj->ifname = if_typename;
> @@ -230,7 +208,6 @@ static void efi_disk_add_dev(const char *name,
>         diskobj->ops.media = &diskobj->media;
>
>         /* Fill in device path */
> -       dp = (void*)&diskobj[1];
>         diskobj->dp = dp;
>         dp[0].dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
>         dp[0].dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
> diff --git a/lib/efi_loader/efi_gop.c b/lib/efi_loader/efi_gop.c
> index 286ad83097..c491ca0815 100644
> --- a/lib/efi_loader/efi_gop.c
> +++ b/lib/efi_loader/efi_gop.c
> @@ -172,7 +172,7 @@ int efi_gop_register(void)
>
>         /* Fill in object data */
>         gopobj->parent.protocols[0].guid = &efi_gop_guid;
> -       gopobj->parent.protocols[0].open = efi_return_handle;
> +       gopobj->parent.protocols[0].open = &gopobj->ops;
>         gopobj->parent.handle = &gopobj->ops;
>         gopobj->ops.query_mode = gop_query_mode;
>         gopobj->ops.set_mode = gop_set_mode;
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index bc8e04d6d9..749093c656 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -18,14 +18,6 @@ DECLARE_GLOBAL_DATA_PTR;
>  const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
>  const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
>
> -efi_status_t EFIAPI efi_return_handle(void *handle, efi_guid_t *protocol,
> -                       void **protocol_interface, void *agent_handle,
> -                       void *controller_handle, uint32_t attributes)
> -{
> -       *protocol_interface = handle;
> -       return EFI_SUCCESS;
> -}
> -
>  static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
>                         unsigned long rel_size, void *efi_reloc)
>  {
> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> index 604ac6e040..0b949d86e8 100644
> --- a/lib/efi_loader/efi_net.c
> +++ b/lib/efi_loader/efi_net.c
> @@ -199,30 +199,6 @@ static efi_status_t EFIAPI efi_net_receive(struct efi_simple_network *this,
>         return EFI_EXIT(EFI_SUCCESS);
>  }
>
> -static efi_status_t EFIAPI efi_net_open_dp(void *handle, efi_guid_t *protocol,
> -                       void **protocol_interface, void *agent_handle,
> -                       void *controller_handle, uint32_t attributes)
> -{
> -       struct efi_simple_network *net = handle;
> -       struct efi_net_obj *netobj = container_of(net, struct efi_net_obj, net);
> -
> -       *protocol_interface = &netobj->dp_mac;
> -
> -       return EFI_SUCCESS;
> -}
> -
> -static efi_status_t EFIAPI efi_net_open_pxe(void *handle, efi_guid_t *protocol,
> -                       void **protocol_interface, void *agent_handle,
> -                       void *controller_handle, uint32_t attributes)
> -{
> -       struct efi_simple_network *net = handle;
> -       struct efi_net_obj *netobj = container_of(net, struct efi_net_obj, net);
> -
> -       *protocol_interface = &netobj->pxe;
> -
> -       return EFI_SUCCESS;
> -}
> -
>  void efi_net_set_dhcp_ack(void *pkt, int len)
>  {
>         int maxsize = sizeof(*dhcp_ack);
> @@ -258,11 +234,11 @@ int efi_net_register(void **handle)
>
>         /* Fill in object data */
>         netobj->parent.protocols[0].guid = &efi_net_guid;
> -       netobj->parent.protocols[0].open = efi_return_handle;
> +       netobj->parent.protocols[0].protocol_interface = &netobj->net;
>         netobj->parent.protocols[1].guid = &efi_guid_device_path;
> -       netobj->parent.protocols[1].open = efi_net_open_dp;
> +       netobj->parent.protocols[1].protocol_interface = &netobj->dp_mac;
>         netobj->parent.protocols[2].guid = &efi_pxe_guid;
> -       netobj->parent.protocols[2].open = efi_net_open_pxe;
> +       netobj->parent.protocols[2].protocol_interface = &netobj->pxe;
>         netobj->parent.handle = &netobj->net;
>         netobj->net.start = efi_net_start;
>         netobj->net.stop = efi_net_stop;
> --
> 2.11.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot


More information about the U-Boot mailing list