[U-Boot] [PATCH v3 0/3] efi_loader: OpenProtocol, CloseProtocol
The implementation of OpenProtocol was incomplete up to now. CloseProtocol and OpenProtocolInformation were not implemented.
The first patch prints a debug message with the protocol GUID in OpenProtocol.
The second patch adds a table of efi_open_protocol_info_entry to keep track of locks to each protocol entry of each handle. This table is maintained in OpenProtocol and CloseProtocol.
The third patch implements OpenProtocolInformation which returns said table. --- v3 Use list_for_each_entry Fix indention Use EFI_CALL to avoid wrapper for efi_disconnect_controller v2 Fixed typo in 1st patch which is resubmitted. Added 2nd and 3rd patch. --- Heinrich Schuchardt (3): efi_loader: write protocol GUID in OpenProtocol efi_loader: open_info in OpenProtocol, CloseProtocol efi_loader: implement OpenProtocolInformation
include/efi_loader.h | 15 +++ lib/efi_loader/efi_boottime.c | 240 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 244 insertions(+), 11 deletions(-)
To understand what happens in OpenProtocol it is necessary to know the protocol interface GUID. Let's write a debug message.
Using uuid_guid_get_str would be quite clumsy for this purpose. This would involve evaluating _DEBUG which probably should not be used outside common.h.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v3: reference uuid_guid_get_str v2: fix typo in commit message --- include/efi_loader.h | 14 ++++++++++++++ lib/efi_loader/efi_boottime.c | 2 ++ 2 files changed, 16 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 037cc7c543..553c615f11 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -51,6 +51,20 @@ const char *__efi_nesting_dec(void); debug("%sEFI: Return From: %s\n", __efi_nesting_dec(), #exp); \ } while(0)
+/* + * Write GUID + */ +#define EFI_PRINT_GUID(txt, guid) ({ \ + debug("EFI: %s %02x%02x%02x%02x-%02x%02x-%02x%02x-" \ + "%02x%02x%02x%02x%02x%02x%02x%02x\n", \ + txt, ((u8 *)guid)[3], \ + ((u8 *)guid)[2], ((u8 *)guid)[1], ((u8 *)guid)[0], \ + ((u8 *)guid)[5], ((u8 *)guid)[4], ((u8 *)guid)[7], \ + ((u8 *)guid)[6], ((u8 *)guid)[8], ((u8 *)guid)[9], \ + ((u8 *)guid)[10], ((u8 *)guid)[11], ((u8 *)guid)[12], \ + ((u8 *)guid)[13], ((u8 *)guid)[14], ((u8 *)guid)[15]); \ + }) + extern struct efi_runtime_services efi_runtime_services; extern struct efi_system_table systab;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index fb05c9e093..ebb557abb2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1138,6 +1138,8 @@ static efi_status_t EFIAPI efi_open_protocol( goto out; }
+ EFI_PRINT_GUID("protocol:", protocol); + switch (attributes) { case EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL: case EFI_OPEN_PROTOCOL_GET_PROTOCOL:
On 05.08.17 21:32, Heinrich Schuchardt wrote:
To understand what happens in OpenProtocol it is necessary to know the protocol interface GUID. Let's write a debug message.
Using uuid_guid_get_str would be quite clumsy for this purpose. This would involve evaluating _DEBUG which probably should not be used outside common.h.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
I agree with Rob that a printf extension would be the nicest way to go here. We could then just use that instead of the %p in EFI_ENTRY() that we have today.
Alex
On 08/11/2017 12:11 PM, Alexander Graf wrote:
On 05.08.17 21:32, Heinrich Schuchardt wrote:
To understand what happens in OpenProtocol it is necessary to know the protocol interface GUID. Let's write a debug message.
Using uuid_guid_get_str would be quite clumsy for this purpose. This would involve evaluating _DEBUG which probably should not be used outside common.h.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
I agree with Rob that a printf extension would be the nicest way to go here. We could then just use that instead of the %p in EFI_ENTRY() that we have today.
Alex
Hello Alex,
I am aware of [PATCH 4/5] vsprintf.c: add GUID printing,
I just wonder if you wnat to take this patch series as is and we modify EFI_PRINT_GUID afterwards
or
we I shall remove GUID printing and resend the patch series without it.
Best regards
Heinrich
On 11.08.17 18:00, Heinrich Schuchardt wrote:
On 08/11/2017 12:11 PM, Alexander Graf wrote:
On 05.08.17 21:32, Heinrich Schuchardt wrote:
To understand what happens in OpenProtocol it is necessary to know the protocol interface GUID. Let's write a debug message.
Using uuid_guid_get_str would be quite clumsy for this purpose. This would involve evaluating _DEBUG which probably should not be used outside common.h.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
I agree with Rob that a printf extension would be the nicest way to go here. We could then just use that instead of the %p in EFI_ENTRY() that we have today.
Alex
Hello Alex,
I am aware of [PATCH 4/5] vsprintf.c: add GUID printing,
I just wonder if you wnat to take this patch series as is and we modify EFI_PRINT_GUID afterwards
or
we I shall remove GUID printing and resend the patch series without it.
I think it's best to keep changes isolated to not stall anyone. Since this is really just a debug enhancement, I think we're best off to drop the GUID printing in this patch set.
Alex
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.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v3: use EFI_CALL to avoid wrapper for efi_disconnect_controller use list_for_each_entry move variable declarations out of loops v2: new patch --- include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 10 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..e969814476 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller( return EFI_EXIT(EFI_NOT_FOUND); }
-static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle, - void *driver_image_handle, - void *child_handle) +static efi_status_t EFIAPI efi_disconnect_controller( + 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); }
+static efi_status_t efi_close_protocol_int(struct efi_handler *protocol, + void *agent_handle, + void *controller_handle) +{ + size_t i; + struct efi_open_protocol_info_entry *open_info; + + for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) { + 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, efi_guid_t *protocol, void *agent_handle, void *controller_handle) { + struct efi_object *efiobj; + size_t i; + 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_entry(efiobj, &efi_obj_list, 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 +1174,96 @@ 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 *open_info; + 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) { + 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) { + open_info = &protocol->open_info[i]; + + if (!open_info->open_count) + continue; + if (open_info->attributes == + EFI_OPEN_PROTOCOL_BY_DRIVER) + EFI_CALL(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) { + 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) { + 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; + + 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 +1318,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; } }
On 05.08.17 22:32, Heinrich Schuchardt 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.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v3: use EFI_CALL to avoid wrapper for efi_disconnect_controller use list_for_each_entry move variable declarations out of loops v2: new patch
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 10 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..e969814476 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller( return EFI_EXIT(EFI_NOT_FOUND); }
-static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
void *driver_image_handle,void *child_handle)+static efi_status_t EFIAPI efi_disconnect_controller(
void *controller_handle,void *driver_image_handle,void *child_handle)+static efi_status_t efi_close_protocol_int(struct efi_handler *protocol,
Please either wrap _ext or use EFI_CALL :).
void *agent_handle,void *controller_handle)+{
- size_t i;
- struct efi_open_protocol_info_entry *open_info;
- for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
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, efi_guid_t *protocol, void *agent_handle, void *controller_handle) {
- struct efi_object *efiobj;
- size_t i;
- 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_entry(efiobj, &efi_obj_list, 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 +1174,96 @@ 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(
Same here.
Alex
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 *open_info;
- 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) {
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) {open_info = &protocol->open_info[i];if (!open_info->open_count)continue;if (open_info->attributes ==EFI_OPEN_PROTOCOL_BY_DRIVER)EFI_CALL(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) {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) {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;
- 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 +1318,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; }
On 08/12/2017 03:37 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt 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.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v3: use EFI_CALL to avoid wrapper for efi_disconnect_controller use list_for_each_entry move variable declarations out of loops v2: new patch
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 10 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..e969814476 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller( return EFI_EXIT(EFI_NOT_FOUND); } -static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
void *driver_image_handle,void *child_handle)+static efi_status_t EFIAPI efi_disconnect_controller(
void *controller_handle,void *driver_image_handle,void *child_handle)*protocol,
Please either wrap _ext or use EFI_CALL :).
Why?
Function efi_close_protocol_int is only used to avoid lines over 80 characters in efi_disconnect_controller. It is not called from anywhere else.
Should I add some comment in the code or in the commit message?
void *agent_handle,void *controller_handle)+{
- size_t i;
- struct efi_open_protocol_info_entry *open_info;
- for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
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, efi_guid_t *protocol, void *agent_handle, void *controller_handle) {
- struct efi_object *efiobj;
- size_t i;
- 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_entry(efiobj, &efi_obj_list, 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 +1174,96 @@ 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(
Same here.
Alex
See above.
Was the rest of the patch ok for you?
Regards
Heinrich
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 *open_info;
- 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) {
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) {open_info = &protocol->open_info[i];if (!open_info->open_count)continue;if (open_info->attributes ==EFI_OPEN_PROTOCOL_BY_DRIVER)EFI_CALL(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) {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) {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;
- 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 +1318,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; } }
On 13.08.17 13:09, Heinrich Schuchardt wrote:
On 08/12/2017 03:37 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt 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.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
v3: use EFI_CALL to avoid wrapper for efi_disconnect_controller use list_for_each_entry move variable declarations out of loops v2: new patch
include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 164 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 155 insertions(+), 10 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..e969814476 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -898,23 +898,78 @@ static efi_status_t EFIAPI efi_connect_controller( return EFI_EXIT(EFI_NOT_FOUND); } -static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
void *driver_image_handle,void *child_handle)+static efi_status_t EFIAPI efi_disconnect_controller(
void *controller_handle,void *driver_image_handle,void *child_handle)*protocol,
Please either wrap _ext or use EFI_CALL :).
Why?
Function efi_close_protocol_int is only used to avoid lines over 80 characters in efi_disconnect_controller. It is not called from anywhere else.
Should I add some comment in the code or in the commit message?
Ah, now I see. No, I think the function name is misleading, so that needs change :). How about efi_close_one_protocol()?
void *agent_handle,void *controller_handle)+{
- size_t i;
- struct efi_open_protocol_info_entry *open_info;
- for (i = 0; i < ARRAY_SIZE(protocol->open_info); ++i) {
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, efi_guid_t *protocol, void *agent_handle, void *controller_handle) {
- struct efi_object *efiobj;
- size_t i;
- 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_entry(efiobj, &efi_obj_list, 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 +1174,96 @@ 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(
Same here.
Alex
See above.
Was the rest of the patch ok for you?
I didn't spot anything obviously bad, but that doesn't usually mean much. My review foo isn't quite as good as others'. When applying I would push the patches through some more detailed testing though.
Alex
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- v3: use list_for_each_entry correct indention v2: new patch --- lib/efi_loader/efi_boottime.c | 74 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 65a7a79dc1..fa9b74d465 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -984,14 +984,86 @@ out: return EFI_EXIT(r); }
+static efi_status_t efi_copy_open_protocol_information( + struct efi_handler *protocol, + struct efi_open_protocol_info_entry **entry_buffer, + unsigned long *entry_count) +{ + unsigned long buffer_size; + unsigned long count = 0; + size_t i; + efi_status_t r; + + *entry_buffer = NULL; + + /* Count entries */ + 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) + ++count; + } + *entry_count = count; + if (!count) + return EFI_SUCCESS; + + /* Copy entries */ + buffer_size = count * sizeof(struct efi_open_protocol_info_entry); + r = efi_allocate_pool(EFI_ALLOCATE_ANY_PAGES, buffer_size, + (void **)entry_buffer); + if (r != EFI_SUCCESS) + return r; + count = 0; + 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; + (*entry_buffer)[count] = *open_info; + ++count; + } + + return EFI_SUCCESS; +} + static efi_status_t EFIAPI efi_open_protocol_information(efi_handle_t handle, efi_guid_t *protocol, struct efi_open_protocol_info_entry **entry_buffer, unsigned long *entry_count) { + struct efi_object *efiobj; + size_t i; + efi_status_t r = EFI_NOT_FOUND; + EFI_ENTRY("%p, %p, %p, %p", handle, protocol, entry_buffer, entry_count); - return EFI_EXIT(EFI_NOT_FOUND); + + if (!handle || !protocol || !entry_buffer) + EFI_EXIT(EFI_INVALID_PARAMETER); + + EFI_PRINT_GUID("protocol:", protocol); + + list_for_each_entry(efiobj, &efi_obj_list, 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_copy_open_protocol_information( + handler, entry_buffer, entry_count); + goto out; + } + } + goto out; + } +out: + return EFI_EXIT(r); }
static efi_status_t EFIAPI efi_protocols_per_handle(void *handle,
On 05.08.17 22:32, Heinrich Schuchardt wrote:
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Do you have an application that leverages these interfaces? Would it be possible to add that application to our travis tests?
Alex
On 08/12/2017 03:38 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt wrote:
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Do you have an application that leverages these interfaces? Would it be possible to add that application to our travis tests?
Alex
iPXE (snp.efi) uses the functions but there are other things missing to make it really working.
Putting new tests into the executable created by CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates multiple executables for the different EFI API functions we have to test?
Each test then should consist of: - start QEMU system - download dtb and test executable from tftp server - start the test executable - evaluate console output - shutdown QEMU system
Regards
Heinrich
On 13.08.17 13:17, Heinrich Schuchardt wrote:
On 08/12/2017 03:38 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt wrote:
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Do you have an application that leverages these interfaces? Would it be possible to add that application to our travis tests?
Alex
iPXE (snp.efi) uses the functions but there are other things missing to make it really working.
Ah, I see. How much is missing to make that one work for real?
Putting new tests into the executable created by CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates multiple executables for the different EFI API functions we have to test?
Each test then should consist of:
- start QEMU system
- download dtb and test executable from tftp server
- start the test executable
- evaluate console output
- shutdown QEMU system
We have most of the above already in travis today. All we would need is to extend it to download a built iPXE binaries and add test snippets to tests/py for iPXE functionality.
Alex
On 08/13/2017 09:24 PM, Alexander Graf wrote:
On 13.08.17 13:17, Heinrich Schuchardt wrote:
On 08/12/2017 03:38 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt wrote:
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Do you have an application that leverages these interfaces? Would it be possible to add that application to our travis tests?
Alex
iPXE (snp.efi) uses the functions but there are other things missing to make it really working.
Ah, I see. How much is missing to make that one work for real?
Before reaching the input prompt ConnectController and DisconnectController fail for the Simple Network Protocol.
So I am not able to say if the network connection will work.
For testing we will need an iSCSI target.
Putting new tests into the executable created by CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates multiple executables for the different EFI API functions we have to test?
Each test then should consist of:
- start QEMU system
- download dtb and test executable from tftp server
- start the test executable
- evaluate console output
- shutdown QEMU system
We have most of the above already in travis today. All we would need is to extend it to download a built iPXE binaries and add test snippets to tests/py for iPXE functionality.
Proving that some binary runs is not enough to test the different corner cases of this complex API.
I would prefer to have a bunch of test binaries compiled from the U-Boot git tree.
Best regards
Heinrich
On 13.08.17 21:32, Heinrich Schuchardt wrote:
On 08/13/2017 09:24 PM, Alexander Graf wrote:
On 13.08.17 13:17, Heinrich Schuchardt wrote:
On 08/12/2017 03:38 PM, Alexander Graf wrote:
On 05.08.17 22:32, Heinrich Schuchardt wrote:
efi_open_protocol_information provides the agent and controller handles as well as the attributes and open count of an protocol on a handle.
Cc: Rob Clark robdclark@gmail.com Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
Do you have an application that leverages these interfaces? Would it be possible to add that application to our travis tests?
Alex
iPXE (snp.efi) uses the functions but there are other things missing to make it really working.
Ah, I see. How much is missing to make that one work for real?
Before reaching the input prompt ConnectController and DisconnectController fail for the Simple Network Protocol.
So I am not able to say if the network connection will work.
For testing we will need an iSCSI target.
Putting new tests into the executable created by CMD_BOOTEFI_HELLO_COMPILE would be possible but that seems messy to me.
Should we replace CMD_BOOTEFI_HELLO_COMPILE by an option that creates multiple executables for the different EFI API functions we have to test?
Each test then should consist of:
- start QEMU system
- download dtb and test executable from tftp server
- start the test executable
- evaluate console output
- shutdown QEMU system
We have most of the above already in travis today. All we would need is to extend it to download a built iPXE binaries and add test snippets to tests/py for iPXE functionality.
Proving that some binary runs is not enough to test the different corner cases of this complex API.
I would prefer to have a bunch of test binaries compiled from the U-Boot git tree.
Sure, even better in my book ;). Ideally we would have both. Unit tests to check individual interfaces and integration tests to make sure we don't regress real world use cases.
Alex
participants (3)
-
Alexander Graf -
Heinrich Schuchardt -
Heinrich Schuchardt