| Message ID | 20171112140247.26532-6-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | manage protocols in a linked list | expand |
Hi Heinrich, On 12 November 2017 at 07:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > The current implementation of efi_locate_device_path does not match > the UEFI specification. It completely ignores the protocol > parameters. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > v2 > no change > --- > lib/efi_loader/efi_boottime.c | 106 ++++++++++++++++++++++++++++++------------ > 1 file changed, 76 insertions(+), 30 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> Nits below > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 9b5512f086..7457d54739 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1080,36 +1080,6 @@ static efi_status_t EFIAPI efi_locate_handle_ext( > buffer_size, buffer)); > } > > -/* > - * Get the device path and handle of an device implementing a protocol. > - * > - * This function implements the LocateDevicePath service. > - * See the Unified Extensible Firmware Interface (UEFI) specification > - * for details. Oh yes, I will get right on that now :-) > - * > - * @protocol GUID of the protocol > - * @device_path device path > - * @device handle of the device > - * @return status code > - */ > -static efi_status_t EFIAPI efi_locate_device_path( > - const efi_guid_t *protocol, > - struct efi_device_path **device_path, > - efi_handle_t *device) > -{ > - struct efi_object *efiobj; > - > - EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); > - > - efiobj = efi_dp_find_obj(*device_path, device_path); > - if (!efiobj) > - return EFI_EXIT(EFI_NOT_FOUND); > - > - *device = efiobj->handle; > - > - return EFI_EXIT(EFI_SUCCESS); > -} > - > /* Collapses configuration table entries, removing index i */ > static void efi_remove_configuration_table(int i) > { > @@ -1813,6 +1783,82 @@ static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol, > return EFI_EXIT(EFI_NOT_FOUND); > } > > +/* > + * Get the device path and handle of an device implementing a protocol. a device? > + * > + * This function implements the LocateDevicePath service. > + * See the Unified Extensible Firmware Interface (UEFI) specification > + * for details. > + * > + * @protocol GUID of the protocol > + * @device_path device path > + * @device handle of the device > + * @return status code > + */ > +static efi_status_t EFIAPI efi_locate_device_path( > + const efi_guid_t *protocol, > + struct efi_device_path **device_path, > + efi_handle_t *device) > +{ > + struct efi_device_path *dp; > + size_t i; > + struct efi_handler *handler; > + efi_handle_t *handles; > + size_t len, len_dp; > + size_t len_best = 0; > + efi_uintn_t no_handles; Is there none? I think num_handles is better, or handles_count. > + u8 *remainder; > + efi_status_t ret; > + > + EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); > + > + if (!protocol || !device_path || !*device_path || !device) { > + ret = EFI_INVALID_PARAMETER; > + goto out; > + } > + > + /* Find end of device path */ > + len = efi_dp_size(*device_path); > + > + /* Get all handles implementing the protocol */ > + ret = EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL, protocol, NULL, > + &no_handles, &handles)); > + if (ret != EFI_SUCCESS) > + goto out; > + > + for (i = 0; i < no_handles; ++i) { > + /* Find the device path protocol */ > + ret = efi_search_protocol(handles[i], &efi_guid_device_path, > + &handler); > + if (ret != EFI_SUCCESS) > + continue; > + dp = (struct efi_device_path *)handler->protocol_interface; > + len_dp = efi_dp_size(dp); > + /* > + * This handle can only be a better fit > + * if its device path length is longer then the best fit and > + * if its device path length is shorter of equal the searched > + * device path. Format to ~77 cols? > + */ > + if (len_dp <= len_best || len_dp > len) > + continue; > + /* Check if dp is a subpath of device_path. */ Can you drop the period? > + if (memcmp(*device_path, dp, len_dp)) > + continue; > + *device = handles[i]; > + len_best = len_dp; > + } > + if (len_best) { > + remainder = (u8 *)*device_path + len_best; > + *device_path = (struct efi_device_path *)remainder; > + ret = EFI_SUCCESS; > + } else { > + ret = EFI_NOT_FOUND; > + } > +out: > + return EFI_EXIT(ret); > +} > + > /* > * Install multiple protocol interfaces. > * > -- > 2.15.0 > Regards, Simon
On 11/17/2017 03:06 PM, Simon Glass wrote: > Hi Heinrich, > > On 12 November 2017 at 07:02, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> The current implementation of efi_locate_device_path does not match >> the UEFI specification. It completely ignores the protocol >> parameters. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> v2 >> no change >> --- >> lib/efi_loader/efi_boottime.c | 106 ++++++++++++++++++++++++++++++------------ >> 1 file changed, 76 insertions(+), 30 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Nits below > >> >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 9b5512f086..7457d54739 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -1080,36 +1080,6 @@ static efi_status_t EFIAPI efi_locate_handle_ext( >> buffer_size, buffer)); >> } >> >> -/* >> - * Get the device path and handle of an device implementing a protocol. >> - * >> - * This function implements the LocateDevicePath service. >> - * See the Unified Extensible Firmware Interface (UEFI) specification >> - * for details. > > Oh yes, I will get right on that now :-) > >> - * >> - * @protocol GUID of the protocol >> - * @device_path device path >> - * @device handle of the device >> - * @return status code >> - */ >> -static efi_status_t EFIAPI efi_locate_device_path( >> - const efi_guid_t *protocol, >> - struct efi_device_path **device_path, >> - efi_handle_t *device) >> -{ >> - struct efi_object *efiobj; >> - >> - EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); >> - >> - efiobj = efi_dp_find_obj(*device_path, device_path); >> - if (!efiobj) >> - return EFI_EXIT(EFI_NOT_FOUND); >> - >> - *device = efiobj->handle; >> - >> - return EFI_EXIT(EFI_SUCCESS); >> -} >> - >> /* Collapses configuration table entries, removing index i */ >> static void efi_remove_configuration_table(int i) >> { >> @@ -1813,6 +1783,82 @@ static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol, >> return EFI_EXIT(EFI_NOT_FOUND); >> } >> >> +/* >> + * Get the device path and handle of an device implementing a protocol. > > a device? > >> + * >> + * This function implements the LocateDevicePath service. >> + * See the Unified Extensible Firmware Interface (UEFI) specification >> + * for details. >> + * >> + * @protocol GUID of the protocol >> + * @device_path device path >> + * @device handle of the device >> + * @return status code >> + */ >> +static efi_status_t EFIAPI efi_locate_device_path( >> + const efi_guid_t *protocol, >> + struct efi_device_path **device_path, >> + efi_handle_t *device) >> +{ >> + struct efi_device_path *dp; >> + size_t i; >> + struct efi_handler *handler; >> + efi_handle_t *handles; >> + size_t len, len_dp; >> + size_t len_best = 0; >> + efi_uintn_t no_handles; > > Is there none? > > I think num_handles is better, or handles_count. In the UEFI spec this is the name of the parameter of LocateHandleBuffer and it is also the parameter name in efi_locate_handle_buffer. I get confused when using different names for the same thing. > >> + u8 *remainder; >> + efi_status_t ret; >> + >> + EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); >> + >> + if (!protocol || !device_path || !*device_path || !device) { >> + ret = EFI_INVALID_PARAMETER; >> + goto out; >> + } >> + >> + /* Find end of device path */ >> + len = efi_dp_size(*device_path); >> + >> + /* Get all handles implementing the protocol */ >> + ret = EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL, protocol, NULL, >> + &no_handles, &handles)); >> + if (ret != EFI_SUCCESS) >> + goto out; >> + >> + for (i = 0; i < no_handles; ++i) { >> + /* Find the device path protocol */ >> + ret = efi_search_protocol(handles[i], &efi_guid_device_path, >> + &handler); >> + if (ret != EFI_SUCCESS) >> + continue; >> + dp = (struct efi_device_path *)handler->protocol_interface; >> + len_dp = efi_dp_size(dp); >> + /* >> + * This handle can only be a better fit >> + * if its device path length is longer then the best fit and >> + * if its device path length is shorter of equal the searched >> + * device path. > > Format to ~77 cols? I deliberately put the two conditions in separate lines to make the text easier to read. I could have added bullet points but that looked like overkill to me. > >> + */ >> + if (len_dp <= len_best || len_dp > len) >> + continue; >> + /* Check if dp is a subpath of device_path. */ > > Can you drop the period? Sure. Regards Heinrich > >> + if (memcmp(*device_path, dp, len_dp)) >> + continue; >> + *device = handles[i]; >> + len_best = len_dp; >> + } >> + if (len_best) { >> + remainder = (u8 *)*device_path + len_best; >> + *device_path = (struct efi_device_path *)remainder; >> + ret = EFI_SUCCESS; >> + } else { >> + ret = EFI_NOT_FOUND; >> + } >> +out: >> + return EFI_EXIT(ret); >> +} >> + >> /* >> * Install multiple protocol interfaces. >> * >> -- >> 2.15.0 >> > Regards, > Simon >
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 9b5512f086..7457d54739 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1080,36 +1080,6 @@ static efi_status_t EFIAPI efi_locate_handle_ext( buffer_size, buffer)); } -/* - * Get the device path and handle of an device implementing a protocol. - * - * This function implements the LocateDevicePath service. - * See the Unified Extensible Firmware Interface (UEFI) specification - * for details. - * - * @protocol GUID of the protocol - * @device_path device path - * @device handle of the device - * @return status code - */ -static efi_status_t EFIAPI efi_locate_device_path( - const efi_guid_t *protocol, - struct efi_device_path **device_path, - efi_handle_t *device) -{ - struct efi_object *efiobj; - - EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); - - efiobj = efi_dp_find_obj(*device_path, device_path); - if (!efiobj) - return EFI_EXIT(EFI_NOT_FOUND); - - *device = efiobj->handle; - - return EFI_EXIT(EFI_SUCCESS); -} - /* Collapses configuration table entries, removing index i */ static void efi_remove_configuration_table(int i) { @@ -1813,6 +1783,82 @@ static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol, return EFI_EXIT(EFI_NOT_FOUND); } +/* + * Get the device path and handle of an device implementing a protocol. + * + * This function implements the LocateDevicePath service. + * See the Unified Extensible Firmware Interface (UEFI) specification + * for details. + * + * @protocol GUID of the protocol + * @device_path device path + * @device handle of the device + * @return status code + */ +static efi_status_t EFIAPI efi_locate_device_path( + const efi_guid_t *protocol, + struct efi_device_path **device_path, + efi_handle_t *device) +{ + struct efi_device_path *dp; + size_t i; + struct efi_handler *handler; + efi_handle_t *handles; + size_t len, len_dp; + size_t len_best = 0; + efi_uintn_t no_handles; + u8 *remainder; + efi_status_t ret; + + EFI_ENTRY("%pUl, %p, %p", protocol, device_path, device); + + if (!protocol || !device_path || !*device_path || !device) { + ret = EFI_INVALID_PARAMETER; + goto out; + } + + /* Find end of device path */ + len = efi_dp_size(*device_path); + + /* Get all handles implementing the protocol */ + ret = EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL, protocol, NULL, + &no_handles, &handles)); + if (ret != EFI_SUCCESS) + goto out; + + for (i = 0; i < no_handles; ++i) { + /* Find the device path protocol */ + ret = efi_search_protocol(handles[i], &efi_guid_device_path, + &handler); + if (ret != EFI_SUCCESS) + continue; + dp = (struct efi_device_path *)handler->protocol_interface; + len_dp = efi_dp_size(dp); + /* + * This handle can only be a better fit + * if its device path length is longer then the best fit and + * if its device path length is shorter of equal the searched + * device path. + */ + if (len_dp <= len_best || len_dp > len) + continue; + /* Check if dp is a subpath of device_path. */ + if (memcmp(*device_path, dp, len_dp)) + continue; + *device = handles[i]; + len_best = len_dp; + } + if (len_best) { + remainder = (u8 *)*device_path + len_best; + *device_path = (struct efi_device_path *)remainder; + ret = EFI_SUCCESS; + } else { + ret = EFI_NOT_FOUND; + } +out: + return EFI_EXIT(ret); +} + /* * Install multiple protocol interfaces. *
The current implementation of efi_locate_device_path does not match the UEFI specification. It completely ignores the protocol parameters. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- v2 no change --- lib/efi_loader/efi_boottime.c | 106 ++++++++++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 30 deletions(-)