On Tue, Jan 15, 2019 at 05:26:42AM +0100, Heinrich Schuchardt wrote:
> 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.aka...@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.
I don't think so.
> 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 don't think it's worth generating a one-time string only for this
specific use.
If 'printf' is a matter, I will simply replace it to puts() here.
> 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.
OK
Thanks,
-Takahiro Akashi
> 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(
> >
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
