On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

"devices" command prints all the uefi variables on the system.

=> efi devices Scanning disk ahci_scsi.id0lun0... Scanning disk ahci_scsi.id1lun0... Found 4 disks D e v Device Path ================ ==================== 7ef3bfa0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b) 7ef3cca0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0) 7ef3d070 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0) 7ef3d1b0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(1,MBR,0x086246ba,0x800,0x40000) 7ef3d3e0 /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(1,0)/HD(2,MBR,0x086246ba,0x40800,0x3f800)

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index c7dc2c11f2e7..6b2df1beedb5 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -18,6 +18,8 @@ #include <linux/ctype.h> #include <asm/global_data.h>

+#define BS systab.boottime

static void dump_var_data(char *data, unsigned long len) { char *start, *end, *p; @@ -266,6 +268,96 @@ out: return ret; }

+static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,

• 		    int *num)
  

+{

• efi_handle_t *handles = NULL;
• efi_uintn_t size = 0;
• efi_status_t ret;
• if (guid) {

• ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
  

• 			handles);
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	handles = calloc(1, size);
  

• 	if (!handles)
  

• 		return -1;
  

• 	ret = BS->locate_handle(BY_PROTOCOL, guid, NULL, &size,
  

• 				handles);
  

• }
  

• if (ret != EFI_SUCCESS) {
  

• 	free(handles);
  

• 	return -1;
  

• }
  

• } else {

• ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size, NULL);
  

• if (ret == EFI_BUFFER_TOO_SMALL) {
  

• 	handles = calloc(1, size);
  

• 	if (!handles)
  

• 		return -1;
  

• 	ret = BS->locate_handle(ALL_HANDLES, NULL, NULL, &size,
  

• 				handles);
  

• }
  

• if (ret != EFI_SUCCESS) {
  

• 	free(handles);
  

• 	return -1;
  

• }
  

• }
• *handlesp = handles;
• *num = size / sizeof(efi_handle_t);
• return 0;

+}

+static int efi_get_device_handle_info(efi_handle_t handle, u16 **name) +{

• struct efi_device_path *dp;
• efi_status_t ret;
• ret = BS->open_protocol(handle, &efi_guid_device_path,

• 		(void **)&dp, NULL /* FIXME */, NULL,
  

• 		EFI_OPEN_PROTOCOL_GET_PROTOCOL);
  

• if (ret == EFI_SUCCESS) {

• *name = efi_dp_str(dp);
  

• return 0;
  

• } else {

• return -1;
  

• }

+}

+static int do_efi_show_devices(int argc, char * const argv[]) +{

• efi_handle_t *handles;
• u16 *devname;

This is the device path. So why not call it dev_path?

• int num, i;
• handles = NULL;
• num = 0;
• if (efi_get_handles_by_proto(NULL, &handles, &num))

• return CMD_RET_FAILURE;
  

• if (!num)

• return CMD_RET_SUCCESS;
  

• printf("D\n");
• printf("e\n");
• printf("v Device Path\n");
• printf("================ ====================\n");
• for (i = 0; i < num; i++) {

• if (!efi_get_device_handle_info(handles[i], &devname)) {
  

• 	printf("%16p %ls\n", handles[i], devname);
  

• 	efi_free_pool(devname);
  

• }
  

• }
• free(handles);
• return CMD_RET_SUCCESS;

+}

static int do_efi_boot_add(int argc, char * const argv[]) {

Please, use the standard way of adding sub-commands, cf. doc/README.commands.

Thanks a lot for your contribution.

Regards

Heinrich

int id; @@ -673,6 +765,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, return do_efi_dump_var(argc, argv); else if (!strcmp(command, "setvar")) return do_efi_set_var(argc, argv);

• else if (!strcmp(command, "devices"))

• return do_efi_show_devices(argc, argv);
  

  else return CMD_RET_USAGE;

} @@ -697,7 +791,9 @@ static char efitool_help_text[] = " - show UEFI variable's value\n" "efitool setvar <name> [<value>]\n" " - set/delete uefi variable's value\n"

• " <value> may be "="..."", "=0x...", "=H...", (set) or "=" (delete)\n";

• " <value> may be "="..."", "=0x...", "=H...", (set) or "=" (delete)\n"
• "efitool devices\n"
• " - show uefi devices\n";

#endif

U_BOOT_CMD(

You raised a couple of questions to me.

On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote:

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

...

This new field will be shown as a driver's name in "efitool drivers" command.

We can have drivers supplied by U-Boot

I assume that what you mention here is a UCLASS_EFI driver.

What's the problem is; efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with *efi_driver_binding_extended_protocol* interface, which is NOT compatible with EFI_DRIVER_BINDING_PROTOCOL. On the other hand, for example, in your efi_selftest_controller test a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL with EFI_DRIVER_BINDING_PROTOCOL interface.

So we have no way to distinguish the two cases(handles) and cannot deal with them properly.

and drivers supplied by an EFI binary that we recently installed via the bootefi command.

A driver installed via the bootefi command will not have allocated memory for the extra fields.

There is no good example of driver of such kind. I don't know how we can retrieve a meaningful "driver name."

So you cannot use the name field in your "efitool drivers" command.

Any suggestion?

Thanks, -Takahiro Akashi

Best regards

Heinrich

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_driver.h | 1 + lib/efi_driver/efi_uclass.c | 1 + 2 files changed, 2 insertions(+)

diff --git a/include/efi_driver.h b/include/efi_driver.h index 840483a416a4..ee8867816094 100644 --- a/include/efi_driver.h +++ b/include/efi_driver.h @@ -34,6 +34,7 @@ struct efi_driver_ops {

• This structure adds internal fields to the driver binding protocol.

*/ struct efi_driver_binding_extended_protocol {

• const char *name; struct efi_driver_binding_protocol bp; const struct efi_driver_ops *ops;

}; diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c index bb86ffd399c3..8bbaa02d490e 100644 --- a/lib/efi_driver/efi_uclass.c +++ b/lib/efi_driver/efi_uclass.c @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) bp->bp.stop = efi_uc_stop; bp->bp.version = 0xffffffff; bp->ops = drv->ops;

• bp->name = drv->name;

  ret = efi_create_handle(&bp->bp.driver_binding_handle); if (ret != EFI_SUCCESS) {

On Thu, Jan 17, 2019 at 07:58:17AM +0100, Heinrich Schuchardt wrote:

On 1/17/19 6:33 AM, AKASHI Takahiro wrote:

...

You raised a couple of questions to me.

On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote:

...

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

...

This new field will be shown as a driver's name in "efitool drivers" command.

We can have drivers supplied by U-Boot

I assume that what you mention here is a UCLASS_EFI driver.

What's the problem is; efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with *efi_driver_binding_extended_protocol* interface, which is NOT compatible with EFI_DRIVER_BINDING_PROTOCOL. On the other hand, for example, in your efi_selftest_controller test a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL with EFI_DRIVER_BINDING_PROTOCOL interface.

So we have no way to distinguish the two cases(handles) and cannot deal with them properly.

Correct. It is allowable to add private fields to a protocol. EDK2 does the same. But we cannot make any assumptions about the private fields here.

I misunderstood something here, but anyhow,

...

...

and drivers supplied by an EFI binary that we recently installed via the bootefi command.

A driver installed via the bootefi command will not have allocated memory for the extra fields.

There is no good example of driver of such kind. I don't know how we can retrieve a meaningful "driver name."

The UEFI spec does not foresee any name field.

Edk2's shell does get a driver's name by using COMPONENT_NAME2_PROTOCOL, and we'd better do the same way. So "<NULL>" will be printed with my patch for now.

-Takahiro Akashi

The interesting information is: on which handle did the driver install which protocol.

This information could be gathered by looping over all protocols on all handles and looking for an OpenProtocolInformation where the driver is the controller.

For me it would be fine if we leave that to a later patch.

Best regards

Heinrich

...

...

So you cannot use the name field in your "efitool drivers" command.

Any suggestion?

Thanks, -Takahiro Akashi

...

Best regards

Heinrich

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

include/efi_driver.h | 1 + lib/efi_driver/efi_uclass.c | 1 + 2 files changed, 2 insertions(+)

diff --git a/include/efi_driver.h b/include/efi_driver.h index 840483a416a4..ee8867816094 100644 --- a/include/efi_driver.h +++ b/include/efi_driver.h @@ -34,6 +34,7 @@ struct efi_driver_ops {

• This structure adds internal fields to the driver binding protocol.

*/ struct efi_driver_binding_extended_protocol {

• const char *name; struct efi_driver_binding_protocol bp; const struct efi_driver_ops *ops;

}; diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c index bb86ffd399c3..8bbaa02d490e 100644 --- a/lib/efi_driver/efi_uclass.c +++ b/lib/efi_driver/efi_uclass.c @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) bp->bp.stop = efi_uc_stop; bp->bp.version = 0xffffffff; bp->ops = drv->ops;

• bp->name = drv->name;

  ret = efi_create_handle(&bp->bp.driver_binding_handle); if (ret != EFI_SUCCESS) {

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

"drivers" command prints all the uefi drivers on the system.

=> efi drivers D r v Driver Name Image Path ================ ==================== ========== 7ef31d30 EFI block driver <built-in>

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index 6b2df1beedb5..4d46721fbf91 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -8,6 +8,7 @@ #include <charset.h> #include <common.h> #include <command.h> +#include <efi_driver.h> #include <efi_loader.h> #include <environment.h> #include <errno.h> @@ -16,6 +17,7 @@ #include <malloc.h> #include <search.h> #include <linux/ctype.h> +#include <linux/kernel.h> #include <asm/global_data.h>

#define BS systab.boottime @@ -358,6 +360,82 @@ static int do_efi_show_devices(int argc, char * const argv[]) return CMD_RET_SUCCESS; }

+static int efi_get_driver_handle_info(efi_handle_t handle, u16 **name,

• 		      u16 **devname, u16 **filename)
  

+{

• struct efi_handler *handler;
• struct efi_driver_binding_extended_protocol *bp;

We can have drivers supplied by U-Boot and drivers supplied by an EFI binary that we recently installed via the bootefi command.

A driver installed via the bootefi command will not have allocated memory for the extra fields. So the rest of the code accessing the name field will produce an illegal memory access.

Best regards

Heinrich

• efi_status_t ret;
• ret = efi_search_protocol(handle, &efi_guid_driver_binding_protocol,

• 		  &handler);
  

• if (ret != EFI_SUCCESS)

• return -1;
  

• bp = handler->protocol_interface;
• *name = malloc((strlen(bp->name) + 1) * sizeof(u16));
• if (*name)

• ascii2unicode(*name, bp->name);
  

• /*

• * TODO:
  

• * handle image->device_handle,
  

• * use append_device_path()
  

• */
  

• *devname = NULL;
• *filename = NULL;
• return 0;

+}

+static int do_efi_show_drivers(int argc, char * const argv[]) +{

• efi_handle_t *handles = NULL, *handle;
• efi_uintn_t size = 0;
• u16 *drvname, *devname, *filename;
• efi_status_t ret;
• int i;
• ret = BS->locate_handle(BY_PROTOCOL, &efi_guid_driver_binding_protocol,

• 		NULL, &size, NULL);
  

• if (ret == EFI_BUFFER_TOO_SMALL) {

• handles = calloc(1, size);
  

• if (!handles)
  

• 	return CMD_RET_FAILURE;
  

• ret = BS->locate_handle(BY_PROTOCOL,
  

• 			&efi_guid_driver_binding_protocol,
  

• 			NULL, &size, handles);
  

• }
• if (ret != EFI_SUCCESS) {

• free(handles);
  

• return CMD_RET_FAILURE;
  

• }
• printf("D\n");
• printf("r\n");
• printf("v Driver Name Image Path\n");
• printf("================ ==================== ==========\n");
• handle = handles;
• for (i = 0; i < size / sizeof(*handle); i++) {

• if (!efi_get_driver_handle_info(*handle, &drvname, &devname,
  

• 				&filename)) {
  

• 	if (filename)
  

• 		printf("%16p %-20ls %ls/%ls\n",
  

• 		       *handle, drvname, devname, filename);
  

• 	else
  

• 		printf("%16p %-20ls <built-in>\n",
  

• 		       *handle, drvname);
  

• 	free(drvname);
  

• 	free(devname);
  

• 	free(filename);
  

• }
  

• handle++;
  

• }
• return CMD_RET_SUCCESS;

+}

static int do_efi_boot_add(int argc, char * const argv[]) { int id; @@ -767,6 +845,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, return do_efi_set_var(argc, argv); else if (!strcmp(command, "devices")) return do_efi_show_devices(argc, argv);

• else if (!strcmp(command, "drivers"))

• return do_efi_show_drivers(argc, argv);
  

  else return CMD_RET_USAGE;

} @@ -793,7 +873,9 @@ static char efitool_help_text[] = " - set/delete uefi variable's value\n" " <value> may be "="..."", "=0x...", "=H...", (set) or "=" (delete)\n" "efitool devices\n"

• " - show uefi devices\n";

• " - show uefi devices\n"
• "efitool drivers\n"
• " - show uefi drivers\n";

#endif

U_BOOT_CMD(

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

"dh" command prints all the uefi handles used in the system.

=> efi dh 7ef3bfa0: Device Path, Device Path To Text, Device Path Utilities, Unicode Collation 2 7ef31d30: Driver Binding 7ef31da0: Simple Text Output 7ef31e10: Simple Text Input, Simple Text Input Ex 7ef3cca0: Block IO, Device Path 7ef3d070: Block IO, Device Path 7ef3d1b0: Block IO, Device Path, Simple File System 7ef3d3e0: Block IO, Device Path, Simple File System

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index 4d46721fbf91..4bd6367b4bd9 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -436,6 +436,103 @@ static int do_efi_show_drivers(int argc, char * const argv[]) return CMD_RET_SUCCESS; }

+static struct {

• u16 *name;

Please, reduce the memory size by using char *.

• efi_guid_t guid;

+} guid_list[] = {

• {

• L"Device Path",
  

• DEVICE_PATH_GUID,
  

• },
• {

• L"Device Path To Text",
  

• EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID,
  

• },
• {

• L"Device Path Utilities",
  

• EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID,
  

• },
• {

• L"Unicode Collation 2",
  

• EFI_UNICODE_COLLATION_PROTOCOL2_GUID,
  

• },
• {

• L"Driver Binding",
  

• EFI_DRIVER_BINDING_PROTOCOL_GUID,
  

• },
• {

• L"Simple Text Input",
  

• EFI_SIMPLE_TEXT_INPUT_PROTOCOL_GUID,
  

• },
• {

• L"Simple Text Input Ex",
  

• EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL_GUID,
  

• },
• {

• L"Simple Text Output",
  

• EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL_GUID,
  

• },
• {

• L"Block IO",
  

• BLOCK_IO_GUID,
  

• },
• {

• L"Simple File System",
  

• EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID,
  

• },

We implement a few more protocols, e.g.

include/efi_api.h:283:#define LOADED_IMAGE_PROTOCOL_GUID \ include/efi_api.h:467:#define BLOCK_IO_GUID \ include/efi_api.h:700:#define EFI_GOP_GUID \ include/efi_api.h:977:#define EFI_DRIVER_BINDING_PROTOCOL_GUID \ include/efi_api.h:999:#define EFI_UNICODE_COLLATION_PROTOCOL2_GUID \

• {

• NULL,
  

• {{0,},},
  

• }

You could use the ARRAY_SIZE() macro instead of the NULL entry.

+};

+static int do_efi_show_handles(int argc, char * const argv[]) +{

• efi_handle_t *handles;
• efi_guid_t **guid;
• efi_uintn_t count;
• int num, i, j, k;
• efi_status_t ret;
• handles = NULL;
• num = 0;
• if (efi_get_handles_by_proto(NULL, &handles, &num))

• return CMD_RET_FAILURE;
  

• if (!num)

• return CMD_RET_SUCCESS;
  

• for (i = 0; i < num; i++) {

• printf("%16p:", handles[i]);
  

• ret = BS->protocols_per_handle(handles[i], &guid, &count);
  

• if (ret || !count) {
  

• 	putc('\n');
  

• 	continue;
  

• }
  

• for (j = 0; j < count; j++) {
  

• 	for (k = 0; guid_list[k].name; k++)
  

• 		if (!guidcmp(&guid_list[k].guid, guid[j]))
  

• 			break;
  

Please, put this logic into a separate function returning const char *. Return NULL if not found.

• 	if (j)
  

• 		printf(", ");
  

• 	else
  

• 		putc(' ');
  

• 	if (guid[j])
  

• 		printf("%ls", guid_list[k].name);
  

• 	else
  

• 		printf("%pUl", guid[j]);
  

• }
  

• putc('\n');
  

• }
• free(handles);
• return CMD_RET_SUCCESS;

+}

static int do_efi_boot_add(int argc, char * const argv[]) {

Please, use the standard way of adding sub-commands. See doc/README.commands.

Best regards

Heinrich

int id; @@ -847,6 +944,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, return do_efi_show_devices(argc, argv); else if (!strcmp(command, "drivers")) return do_efi_show_drivers(argc, argv);

• else if (!strcmp(command, "dh"))

• return do_efi_show_handles(argc, argv);
  

  else return CMD_RET_USAGE;

} @@ -875,7 +974,9 @@ static char efitool_help_text[] = "efitool devices\n" " - show uefi devices\n" "efitool drivers\n"

• " - show uefi drivers\n";

• " - show uefi drivers\n"
• "efitool dh\n"
• " - show uefi handles\n";

#endif

U_BOOT_CMD(

On Tue, Jan 15, 2019 at 05:58:57AM +0100, Heinrich Schuchardt wrote:

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

...

"images" command prints loaded images-related information.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index 4bd6367b4bd9..e77273fc6e1e 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -533,6 +533,13 @@ static int do_efi_show_handles(int argc, char * const argv[]) return CMD_RET_SUCCESS; }

+static int do_efi_show_images(int argc, char * const argv[]) +{

• efi_print_image_infos(NULL);
• return CMD_RET_SUCCESS;

+}

static int do_efi_boot_add(int argc, char * const argv[]) {

Please, use the standard way of adding sub-commands. See doc/README.commands.

OK

-Takahiro Akashi

Best regards

Heinrich

...

int id; @@ -946,6 +953,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, return do_efi_show_drivers(argc, argv); else if (!strcmp(command, "dh")) return do_efi_show_handles(argc, argv);

• else if (!strcmp(command, "images"))

• return do_efi_show_images(argc, argv);
  

  else return CMD_RET_USAGE;

} @@ -976,7 +985,9 @@ static char efitool_help_text[] = "efitool drivers\n" " - show uefi drivers\n" "efitool dh\n"

• " - show uefi handles\n";

• " - show uefi handles\n"
• "efitool images\n"
• " - show loaded images\n";

#endif

U_BOOT_CMD(

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

"memmap" command prints uefi-specific memory map information. => efi memmap Type Start End Attributes ================ ================ ================ ========== CONVENTIONAL 0000000040000000-000000007de27000 WB RUNTIME DATA 000000007de27000-000000007de28000 WB|RT RESERVED 000000007de28000-000000007de2a000 WB RUNTIME DATA 000000007de2a000-000000007de2b000 WB|RT RESERVED 000000007de2b000-000000007de2c000 WB RUNTIME DATA 000000007de2c000-000000007de2d000 WB|RT LOADER DATA 000000007de2d000-000000007ff37000 WB RUNTIME CODE 000000007ff37000-000000007ff38000 WB|RT LOADER DATA 000000007ff38000-0000000080000000 WB

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index e77273fc6e1e..f06718ea580d 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -540,6 +540,78 @@ static int do_efi_show_images(int argc, char * const argv[]) return CMD_RET_SUCCESS; }

+static const char * const efi_mem_type_string[] = {

• [EFI_RESERVED_MEMORY_TYPE] = "RESERVED",
• [EFI_LOADER_CODE] = "LOADER CODE",
• [EFI_LOADER_DATA] = "LOADER DATA",
• [EFI_BOOT_SERVICES_CODE] = "BOOT CODE",
• [EFI_BOOT_SERVICES_DATA] = "BOOT DATA",
• [EFI_RUNTIME_SERVICES_CODE] = "RUNTIME CODE",
• [EFI_RUNTIME_SERVICES_DATA] = "RUNTIME DATA",
• [EFI_CONVENTIONAL_MEMORY] = "CONVENTIONAL",
• [EFI_UNUSABLE_MEMORY] = "UNUSABLE MEM",
• [EFI_ACPI_RECLAIM_MEMORY] = "ACPI RECLAIM MEM",
• [EFI_ACPI_MEMORY_NVS] = "ACPI NVS",
• [EFI_MMAP_IO] = "IO",
• [EFI_MMAP_IO_PORT] = "IO PORT",
• [EFI_PAL_CODE] = "PAL",

+};

+#define EFI_MEM_ATTR(attribute, bit, string) \

• if ((attribute) & (bit)) { \

• if (sep)		\
  

• 	putc('|');	\
  

• sep = 1;		\
  

• printf(string);		\
  

• }

+static int do_efi_show_memmap(int argc, char * const argv[]) +{

• struct efi_mem_desc *memmap = NULL, *map;
• efi_uintn_t map_size = 0;
• int i, sep;
• efi_status_t ret;
• ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);

ret = EFI_SUCCCESS will only occur if the memory has not been setup yet.

So shouldn't we error out if ret != EFI_BUFFER_TOO_SMALL ?

• if (ret == EFI_BUFFER_TOO_SMALL) {

• memmap = malloc(map_size);
  

• if (!memmap)
  

• 	return CMD_RET_FAILURE;
  

• ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
  

• }
• if (ret != EFI_SUCCESS) {

• free(memmap);
  

• return CMD_RET_FAILURE;
  

• }
• printf("Type Start End Attributes\n");
• printf("================ ================ ================ ==========\n");
• for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) {

• sep = 0;
  

• printf("%-16s %016llx-%016llx ",
  

•        efi_mem_type_string[map->type],
  

•        map->physical_start,
  

•        map->physical_start + map->num_pages * EFI_PAGE_SIZE);
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UC, "UC");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WC, "WC");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WT, "WT");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WB, "WB");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_UCE, "UCE");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_WP, "WP");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RP, "RP");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_XP, "WP");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_NV, "NV");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_MORE_RELIABLE, "REL");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RO, "RO");
  

• EFI_MEM_ATTR(map->attribute, EFI_MEMORY_RUNTIME, "RT");
  

Please, put that logic into a separate function which returns a const char *. Then we need only a single printf().

I suggest to use an array for the mapping:

struct memory_attribute_name { u64 attribute; const char *short_name; };

struct memory_attribute_name memory_attribute_names[] = { {EFI_MEMORY_UC, "UC"}, ... };

For looping you can use ARRAY_SIZE(memory_attribute_names).

• putc('\n');
  

• }
• free(memmap);
• return CMD_RET_SUCCESS;

+}

static int do_efi_boot_add(int argc, char * const argv[]) {

Please, refer to doc/README.commands for the standard way of adding sub-commands.

Best regards

Heinrich

int id; @@ -955,6 +1027,8 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, return do_efi_show_handles(argc, argv); else if (!strcmp(command, "images")) return do_efi_show_images(argc, argv);

• else if (!strcmp(command, "memmap"))

• return do_efi_show_memmap(argc, argv);
  

  else return CMD_RET_USAGE;

} @@ -987,7 +1061,9 @@ static char efitool_help_text[] = "efitool dh\n" " - show uefi handles\n" "efitool images\n"

• " - show loaded images\n";

• " - show loaded images\n"
• "efitool memmap\n"
• " - show uefi memory map\n";

#endif

U_BOOT_CMD(

On Tue, Jan 15, 2019 at 06:28:38AM +0100, Heinrich Schuchardt wrote:

On 1/15/19 3:55 AM, AKASHI Takahiro wrote:

...

Those function will be used for integration with 'env' command so as to handle uefi variables.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/efitool.c | 38 ++++++++++++++++++++++++++++++++++---- include/command.h | 4 ++++ 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/cmd/efitool.c b/cmd/efitool.c index f06718ea580d..b8fe28c53aaf 100644 --- a/cmd/efitool.c +++ b/cmd/efitool.c @@ -67,7 +67,7 @@ static void dump_var_data(char *data, unsigned long len)

• efi_$guid_$varname = {attributes}(type)value

*/ -static int do_efi_dump_var(int argc, char * const argv[]) +static int _do_efi_dump_var(int argc, char * const argv[])

Please, do not use two function names only distinguished by and underscore. A a reader I will always wonder which does what.

While I believe that "_" and "__" are a very common practice to show an internal version of function,

Please, use names that describe what the difference is.

I will revert _do_efi_dump_var() to do_efi_dump_var() and change do_efi_dump_var() to do_efi_dump_var_ext().

...

{ char regex[256]; char * const regexlist[] = {regex}; @@ -130,6 +130,21 @@ static int do_efi_dump_var(int argc, char * const argv[]) return CMD_RET_SUCCESS; }

+int do_efi_dump_var(int argc, char * const argv[]) +{

• efi_status_t r;
• /* Initialize EFI drivers */
• r = efi_init_obj_list();
• if (r != EFI_SUCCESS) {

• printf("Error: Cannot set up EFI drivers, r = %lu\n",
  

•        r & ~EFI_ERROR_MASK);
  

You duplicate this code below.

Do you want another function for just calling one function? I don't think it's worth doing so.

So, please, put this into a separate function.

That could also help to avoid having separate do_efi_set_var and _do_efi_set_var.

I don't get your point. Do you want to call efi_init_obj_list() at every do_efi_xxx()? It just doesn't make sense.

...

• return CMD_RET_FAILURE;
  

• }
• return _do_efi_dump_var(argc, argv);

+}

static int append_value(char **bufp, unsigned long *sizep, char *data) { char *tmp_buf = NULL, *new_buf = NULL, *value; @@ -227,7 +242,7 @@ out: return 0; }

-static int do_efi_set_var(int argc, char * const argv[]) +static int _do_efi_set_var(int argc, char * const argv[]) { char *var_name, *value = NULL; efi_uintn_t size = 0; @@ -270,6 +285,21 @@ out: return ret; }

+int do_efi_set_var(int argc, char * const argv[]) +{

• efi_status_t r;
• /* Initialize EFI drivers */
• r = efi_init_obj_list();
• if (r != EFI_SUCCESS) {

• printf("Error: Cannot set up EFI drivers, r = %lu\n",
  

It is more then drivers that we setup. So perhaps

"Cannot initialize UEFI sub-system."

I don't mind, but that string directly comes from the original code in bootefi.c.

...

•        r & ~EFI_ERROR_MASK);
  

• return CMD_RET_FAILURE;
  

• }
• return _do_efi_set_var(argc, argv);

+}

static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp, int *num) { @@ -1016,9 +1046,9 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag, if (!strcmp(command, "boot")) return do_efi_boot_opt(argc, argv); else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))

• return do_efi_dump_var(argc, argv);
  

• return _do_efi_dump_var(argc, argv);
  

  else if (!strcmp(command, "setvar"))

• return do_efi_set_var(argc, argv);
  

• return _do_efi_set_var(argc, argv);
  

  else if (!strcmp(command, "devices")) return do_efi_show_devices(argc, argv); else if (!strcmp(command, "drivers"))

diff --git a/include/command.h b/include/command.h index feddef300ccc..315e4b9aabfb 100644 --- a/include/command.h +++ b/include/command.h @@ -51,6 +51,10 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #if defined(CONFIG_CMD_BOOTEFI) extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); #endif +#if defined(CONFIG_CMD_EFITOOL)

The definition has not dependencies. No need for an #if here.

Currently has, but as you and Alex suggested in a separate thread, we may want to enable it (and do_efi_dump/set_var() as well) whether or not CMD_BOOTEFI and CMD_EFIDEBUG, respectively, are defined.

Thanks, -Takahiro Akashi

...

+int do_efi_dump_var(int argc, char * const argv[]); +int do_efi_set_var(int argc, char * const argv[]); +#endi

Shouldn't we put this into some include/efi*?

Best regards

Heinrich

...

/* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int

On 01/15/2019 03:55 AM, AKASHI Takahiro wrote:

"env [print|set] -e" allows for handling uefi variables without knowing details about mapping to corresponding u-boot variables.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/nvedit.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c index ce746bbf1b3e..44f6c3759253 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -27,6 +27,8 @@ #include <cli.h> #include <command.h> #include <console.h> +#include <efi.h> +#include <efi_loader.h> #include <environment.h> #include <search.h> #include <errno.h> @@ -119,6 +121,25 @@ static int do_env_print(cmd_tbl_t *cmdtp, int flag, int argc, int rcode = 0; int env_flag = H_HIDE_DOT;

+#if defined(CONFIG_CMD_EFITOOL)

This needs to be available without your EFITOOL (which really is a DEBUG command). So this needs to be accessible using only CONFIG_EFI_LOADER I think. Maybe we can add a CONFIG_CMD_EFI_SUBCMD or so that these bits are guarded by if you think that someone really wants to live without.

Alex