[U-Boot] [PATCH] efi_loader: add back optional efi_handler::open()
Rob Clark
robdclark at gmail.com
Tue Jul 25 17:20:45 UTC 2017
On Tue, Jul 25, 2017 at 12:39 PM, Heinrich Schuchardt
<xypron.glpk at gmx.de> wrote:
> On 07/25/2017 02:21 PM, Rob Clark wrote:
>> In some cases it is useful to defer creation of the protocol interface
>> object. So add back an optional ->open() hook that is used if
>> protcol_interface is NULL.
>>
>> I've slightly simplified the fxn ptr signature to remove unneeded args,
>> and so compiler will complain if patches that used the "old way" are,
>> and which do not need this extra complexity, are rebased.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>> v2: also handle this properly in locate_protocol as agraf pointed out
>>
>> Note that I might be misunderstanding what the EFI application expects
>> when looking up device-path on a file handle.. if this is only expected
>> to be the device path up to the partition object (and not including the
>> file-path part of the path), then maybe ->open() isn't *as* important
>> as I thought it would be. OTOH I think it would be cleaner to use for
>> loaded_image_info_obj and bootefi_device_obj.. currently I have to
>> patch in the real boot device-path in efi_set_bootdev(), which is
>> messy and fragile.
>>
>> I don't fully understand Heinrich's concerns, especially since the
>> EFI_OPEN_PROTOCOL_INFORMATION_ENTRY stuff looks like it only cares
>> about already opened protocols, which should already have their open()
>> called. (And either way, since open() is optional maybe it is enough
>> to just not use it for any efiobj's where it would be problematic?)
>>
>> So tl;dr, if this is really going to be a problem for some things that
>> Heinrich has got in the pipeline, maybe (pending clarification of a
>> file handle's device-path) we can get by without this. But if it is
>> not a problem, then it certainly seems useful in a few places.
>>
>> include/efi_loader.h | 9 ++++++++-
>> lib/efi_loader/efi_boottime.c | 30 ++++++++++++++++++++++++------
>> 2 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 48eeb4cdd8..a9888afb04 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>> /*
>> * 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 interface */
>> + * protocol GUID to the respective protocol interface.
>> + *
>> + * The optional ->open() fxn can be used for cases where the protocol
>> + * interface is constructed on-demand, and is called if protocol_interface
>> + * is NULL.
>> + */
>> struct efi_handler {
>> const efi_guid_t *guid;
>> void *protocol_interface;
>> + efi_status_t (EFIAPI *open)(void *handle, const efi_guid_t *protocol,
>> + void **protocol_interface);
>> };
>>
>> /*
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 7d45c18ff7..80d9024a53 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -903,6 +903,23 @@ out:
>> return EFI_EXIT(r);
>> }
>>
>> +static efi_status_t open_protocol(struct efi_object *efiobj,
>> + struct efi_handler *handler,
>> + void **protocol_interface)
>> +{
>> + efi_status_t ret = EFI_SUCCESS;
>> +
>> + if (!handler->protocol_interface) {
>> + ret = handler->open(efiobj->handle,
>> + handler->guid,
>> + &handler->protocol_interface);
>> + }
>> + *protocol_interface =
>> + handler->protocol_interface;
>> +
>> + return ret;
>> +}
>> +
>> static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>> void *registration,
>> void **protocol_interface)
>> @@ -925,9 +942,10 @@ static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol,
>> if (!handler->guid)
>> continue;
>> if (!guidcmp(handler->guid, protocol)) {
>> - *protocol_interface =
>> - handler->protocol_interface;
>> - return EFI_EXIT(EFI_SUCCESS);
>> + efi_status_t ret;
>> + ret = open_protocol(efiobj, handler,
>> + protocol_interface);
>
> NAK
>
> With your design a driver could decide that on the first call of
> open_protocol it exposes a protocol and on the second it does not.
Not true, please read the patch again. ->open() is only called the first time.
> Or LocateHandleBuffer return an object meant to support a protocol and
> then opening is refused.
>
> The UEFI standard assumes that the supported protocols can be reliably
> be determined once and for all.
>
> If anything, please, use an init function of type:
> void init(efi_guid guid);
> Simply pass the protocol guid to the init function of the handler.
>
> How many ms and kiB do you think you will save when starting grub with
> deferred initialization?
I did confirm that the device-path for an file object should include
the file path. So this also avoids needing to construct a device path
on each file open, which will most likely never be accessed.
BR,
-R
> Best regards
>
> Heinrich
>
>
>> + return EFI_EXIT(ret);
>> }
>> }
>> }
>> @@ -1061,12 +1079,12 @@ static efi_status_t EFIAPI efi_open_protocol(
>> if (!hprotocol)
>> continue;
>> if (!guidcmp(hprotocol, protocol)) {
>> + r = EFI_SUCCESS;
>> if (attributes !=
>> EFI_OPEN_PROTOCOL_TEST_PROTOCOL) {
>> - *protocol_interface =
>> - handler->protocol_interface;
>> + r = open_protocol(efiobj, handler,
>> + protocol_interface);
>> }
>> - r = EFI_SUCCESS;
>> goto out;
>> }
>> }
>>
>
More information about the U-Boot
mailing list