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

On Tue, Jul 11, 2017 at 4:06 PM, Heinrich Schuchardt xypron.glpk@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@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@lists.denx.de https://lists.denx.de/listinfo/u-boot