[U-Boot] [PATCH v2 2/3] efi_loader: open_info in OpenProtocol, CloseProtocol

Rob Clark robdclark at gmail.com
Sat Aug 5 17:42:12 UTC 2017


On Sat, Aug 5, 2017 at 12:58 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> efi_open_protocol and close_protocol have to keep track of
> opened protocols.
>
> So we add an array open_info to each protocol of each handle.
>
> OpenProtocol has to call DisconnectController internally so we need
> a wrapper efi_disconnect_controller.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
> v2:
>         new patch
> ---
>  include/efi_loader.h          |   1 +
>  lib/efi_loader/efi_boottime.c | 178 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 170 insertions(+), 9 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 553c615f11..222b251a38 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -88,6 +88,7 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>  struct efi_handler {
>         const efi_guid_t *guid;
>         void *protocol_interface;
> +       struct efi_open_protocol_info_entry open_info[4];
>  };
>
>  /*
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index ebb557abb2..65a7a79dc1 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -902,9 +902,42 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>                                                      void *driver_image_handle,
>                                                      void *child_handle)
>  {
> +       return EFI_INVALID_PARAMETER;
> +}
> +
> +static efi_status_t EFIAPI efi_disconnect_controller_ext(
> +                                               void *controller_handle,
> +                                               void *driver_image_handle,
> +                                               void *child_handle)
> +{
>         EFI_ENTRY("%p, %p, %p", controller_handle, driver_image_handle,
>                   child_handle);
> -       return EFI_EXIT(EFI_INVALID_PARAMETER);
> +       return EFI_EXIT(efi_disconnect_controller(controller_handle,
> +                                                 driver_image_handle,
> +                                                 child_handle));
> +}

Now that we have EFI_CALL(), we don't *really* need to have separate
efi_disconnect_controller() and efi_disconnect_controller_ext()..

(Keeping the two versions seems easier for things like
efi_allocate_pool() which are used in a number of places internally,
but that doesn't seem to be the case here..)

> +
> +static efi_status_t efi_close_protocol_int(struct efi_handler *protocol,
> +                                          void *agent_handle,
> +                                          void *controller_handle)
> +{
> +       size_t i;
> +
> +       for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +               struct efi_open_protocol_info_entry *open_info =
> +                       &protocol->open_info[i];
> +
> +               if (!open_info->open_count)
> +                       continue;
> +
> +               if (open_info->agent_handle == agent_handle &&
> +                   open_info->controller_handle ==
> +                   controller_handle) {
> +                       open_info->open_count--;
> +                       return EFI_SUCCESS;
> +               }
> +       }
> +       return EFI_NOT_FOUND;
>  }
>
>  static efi_status_t EFIAPI efi_close_protocol(void *handle,
> @@ -912,9 +945,43 @@ static efi_status_t EFIAPI efi_close_protocol(void *handle,
>                                               void *agent_handle,
>                                               void *controller_handle)
>  {
> +       struct efi_object *efiobj;
> +       size_t i;
> +       struct list_head *lhandle;
> +       efi_status_t r = EFI_NOT_FOUND;
> +
>         EFI_ENTRY("%p, %p, %p, %p", handle, protocol, agent_handle,
>                   controller_handle);
> -       return EFI_EXIT(EFI_NOT_FOUND);
> +
> +       if (!handle || !protocol || !agent_handle) {
> +               r = EFI_INVALID_PARAMETER;
> +               goto out;
> +       }
> +
> +       EFI_PRINT_GUID("protocol:", protocol);
> +
> +       list_for_each(lhandle, &efi_obj_list) {

list_for_each_entry?

(I noticed a lot of the existing code uses list_for_each() +
list_entry()..  so this isn't inconsistent with existing code but it
does seem cleaner to use list_for_each_entry())

> +               efiobj = list_entry(lhandle, struct efi_object, link);
> +
> +               if (efiobj->handle != handle)
> +                       continue;
> +
> +               for (i = 0; i < ARRAY_SIZE(efiobj->protocols); i++) {
> +                       struct efi_handler *handler = &efiobj->protocols[i];
> +                       const efi_guid_t *hprotocol = handler->guid;
> +                       if (!hprotocol)
> +                               continue;
> +                       if (!guidcmp(hprotocol, protocol)) {
> +                               r = efi_close_protocol_int(handler,
> +                                                          agent_handle,
> +                                                          controller_handle);
> +                               goto out;
> +                       }
> +               }
> +               goto out;
> +       }
> +out:
> +       return EFI_EXIT(r);
>  }
>
>  static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle,
> @@ -1119,6 +1186,100 @@ static void EFIAPI efi_set_mem(void *buffer, unsigned long size, uint8_t value)
>         memset(buffer, value, size);
>  }
>
> +static efi_status_t efi_open_protocol_int(
> +                       struct efi_handler *protocol,
> +                       void **protocol_interface, void *agent_handle,
> +                       void *controller_handle, uint32_t attributes)
> +{
> +       bool opened_exclusive = false;
> +       bool opened_by_driver = false;
> +       int i;
> +       struct efi_open_protocol_info_entry *match = NULL;
> +
> +       if (attributes !=
> +           EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> +               *protocol_interface = NULL;
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +               struct efi_open_protocol_info_entry *open_info =
> +                       &protocol->open_info[i];
> +
> +               if (!open_info->open_count)
> +                       continue;
> +               if (open_info->agent_handle == agent_handle) {
> +                       if ((attributes & EFI_OPEN_PROTOCOL_BY_DRIVER) &&
> +                           (open_info->attributes == attributes))
> +                               return EFI_ALREADY_STARTED;
> +                       if (open_info->controller_handle == controller_handle)
> +                               match = open_info;
> +               }
> +               if (open_info->attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE)
> +                       opened_exclusive = true;
> +       }
> +
> +       if (attributes &
> +           (EFI_OPEN_PROTOCOL_EXCLUSIVE | EFI_OPEN_PROTOCOL_BY_DRIVER) &&
> +           opened_exclusive)
> +               return EFI_ACCESS_DENIED;
> +
> +       if (attributes & EFI_OPEN_PROTOCOL_EXCLUSIVE) {
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +                       struct efi_open_protocol_info_entry *open_info =
> +                                       &open_info[i];
> +
> +                       if (!open_info->open_count)
> +                               continue;
> +                       if (open_info->attributes ==
> +                               EFI_OPEN_PROTOCOL_BY_DRIVER) {
> +                               efi_disconnect_controller(
> +                                       open_info->controller_handle,
> +                                       open_info->agent_handle,
> +                                       NULL);
> +                       }
> +               }
> +               opened_by_driver = false;
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +                       struct efi_open_protocol_info_entry *open_info =
> +                                       &protocol->open_info[i];
> +
> +                       if (!open_info->open_count)
> +                               continue;
> +                       if (open_info->attributes & EFI_OPEN_PROTOCOL_BY_DRIVER)
> +                               opened_by_driver = true;
> +               }
> +               if (opened_by_driver)
> +                       return EFI_ACCESS_DENIED;
> +               if (match && !match->open_count)
> +                       match = NULL;
> +       }
> +
> +       /*
> +        * Find an empty slot.
> +        */
> +       if (!match) {
> +               for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
> +                       struct efi_open_protocol_info_entry *open_info =
> +                                       &protocol->open_info[i];
> +
> +                       if (!open_info->open_count) {
> +                               match = open_info;
> +                               break;
> +                       }
> +               }
> +       }
> +       if (!match)
> +               return EFI_OUT_OF_RESOURCES;
> +
> +       match->agent_handle = agent_handle;
> +       match->controller_handle = controller_handle;
> +       match->attributes = attributes;
> +       match->open_count++;
> +       *protocol_interface = protocol->protocol_interface;

This will collide slightly with my "efi_loader: add back optional
efi_handler::open()" patch.  Although in the end looking up
device-paths on file handles (which was the biggest reason I wanted
that patch) turned out to be a bug in fallback[1] so I could probably
re-work things to drop that patch in my patchset.  It means always
constructing an simple-filesystem object for each partition "device",
but that isn't such a heavyweight operation (ie. just amounts to an
extra malloc + populate + insert into list).

I haven't looked closely at the UEFI spec about these interfaces, but
apart from those couple minor nits this looks good.

BR,
-R

[1] https://github.com/rhboot/shim/commit/a8f3dc82be8e16d24ceab416c1b0af897c083f56

> +
> +       return EFI_SUCCESS;
> +}
> +
>  static efi_status_t EFIAPI efi_open_protocol(
>                         void *handle, efi_guid_t *protocol,
>                         void **protocol_interface, void *agent_handle,
> @@ -1173,12 +1334,11 @@ static efi_status_t EFIAPI efi_open_protocol(
>                         if (!hprotocol)
>                                 continue;
>                         if (!guidcmp(hprotocol, protocol)) {
> -                               if (attributes !=
> -                                   EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
> -                                       *protocol_interface =
> -                                               handler->protocol_interface;
> -                               }
> -                               r = EFI_SUCCESS;
> +                               r = efi_open_protocol_int(handler,
> +                                                         protocol_interface,
> +                                                         agent_handle,
> +                                                         controller_handle,
> +                                                         attributes);
>                                 goto out;
>                         }
>                 }
> @@ -1234,7 +1394,7 @@ static const struct efi_boot_services efi_boot_services = {
>         .stall = efi_stall,
>         .set_watchdog_timer = efi_set_watchdog_timer,
>         .connect_controller = efi_connect_controller,
> -       .disconnect_controller = efi_disconnect_controller,
> +       .disconnect_controller = efi_disconnect_controller_ext,
>         .open_protocol = efi_open_protocol,
>         .close_protocol = efi_close_protocol,
>         .open_protocol_information = efi_open_protocol_information,
> --
> 2.11.0
>


More information about the U-Boot mailing list