On Fri, Aug 30, 2013 at 5:53 AM, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> On Fri, 9 Aug 2013 16:26:03 -0700, Roy Franz <roy.franz@xxxxxxxxxx> wrote:
>> Signed-off-by: Roy Franz <roy.franz@xxxxxxxxxx>
>
> The entire commit message is contained in the subject line! The first
> line of a commit message needs to be a short summary, followed by a
> blank line, followed by the full description. Commit text body should be
> wrapped at column 72 so it looks nice in 'git log'
Commit formatting fixed, benefits of no GOT relocation expanded upon.
>
> From your commit message:
>> Subject: Re: [PATCH 02/16] Add system pointer argument to shared EFI
>> stub related functions so they no longer use global system table
>> pointer as they did when part of eboot.c. This code is now shared, so
>> using a global variable as part of the interface is not that nice.
>> Also, by avoiding any global variables in the ARM EFI stub, this
>> allows the code to be position independent without requiring GOT
>> fixups.
>
> The argument about avoiding GOT fixups is stronger than the global
> variables comment. I don't see a problem at all with using a global
> context pointer since this code will only ever run within the context of
> the EFI environment where there must always be a system table pointer.
> It isn't as if there will ever be multiple system tables.
>
> How much of a concern are GOT fixups at this stage? I thought GOT fixups
> were a solved problem for the EFI stub.
I can do the GOT fixups if required, but it simplifies the code a fair
bit if we don't have to do them.
When relocating the zImage to a location it can boot from, right now I
can just copy it since the GOT
hasn't been fixed up. If I fixup the GOT for the location that the
EFI firmware loads it, after I copy it
to the bootable location, then un-fixup the GOT so that the GOT fixup
code that the decompressor code
does will work. I get to avoid all this by not having any global variables.
>
> g.
>
>> ---
>> arch/x86/boot/compressed/eboot.c | 38 +++++++------
>> drivers/firmware/efi/efi-stub-helper.c | 96 +++++++++++++++++---------------
>> 2 files changed, 72 insertions(+), 62 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>> index ab0eefc..65b6a34 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -453,13 +453,13 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
>> status = efi_call_phys3(sys_table->boottime->handle_protocol,
>> handle, &proto, (void *)&image);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
>> + efi_printk(sys_table, "Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
>> return NULL;
>> }
>>
>> - status = low_alloc(0x4000, 1, (unsigned long *)&boot_params);
>> + status = low_alloc(sys_table, 0x4000, 1, (unsigned long *)&boot_params);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc lowmem for boot params\n");
>> + efi_printk(sys_table, "Failed to alloc lowmem for boot params\n");
>> return NULL;
>> }
>>
>> @@ -503,9 +503,10 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
>>
>> options_size++; /* NUL termination */
>>
>> - status = low_alloc(options_size, 1, &cmdline);
>> + status = low_alloc(sys_table, options_size, 1,
>> + &cmdline);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for cmdline\n");
>> + efi_printk(sys_table, "Failed to alloc mem for cmdline\n");
>> goto fail;
>> }
>>
>> @@ -529,16 +530,16 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
>>
>> memset(sdt, 0, sizeof(*sdt));
>>
>> - status = handle_ramdisks(image, hdr);
>> + status = handle_ramdisks(sys_table, image, hdr);
>> if (status != EFI_SUCCESS)
>> goto fail2;
>>
>> return boot_params;
>> fail2:
>> if (options_size)
>> - low_free(options_size, hdr->cmd_line_ptr);
>> + low_free(sys_table, options_size, hdr->cmd_line_ptr);
>> fail:
>> - low_free(0x4000, (unsigned long)boot_params);
>> + low_free(sys_table, 0x4000, (unsigned long)boot_params);
>> return NULL;
>> }
>>
>> @@ -561,7 +562,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
>> again:
>> size += sizeof(*mem_map) * 2;
>> _size = size;
>> - status = low_alloc(size, 1, (unsigned long *)&mem_map);
>> + status = low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> @@ -569,7 +570,7 @@ get_map:
>> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
>> mem_map, &key, &desc_size, &desc_version);
>> if (status == EFI_BUFFER_TOO_SMALL) {
>> - low_free(_size, (unsigned long)mem_map);
>> + low_free(sys_table, _size, (unsigned long)mem_map);
>> goto again;
>> }
>>
>> @@ -671,7 +672,7 @@ get_map:
>> return EFI_SUCCESS;
>>
>> free_mem_map:
>> - low_free(_size, (unsigned long)mem_map);
>> + low_free(sys_table, _size, (unsigned long)mem_map);
>> return status;
>> }
>>
>> @@ -694,10 +695,10 @@ static efi_status_t relocate_kernel(struct setup_header *hdr)
>> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
>> nr_pages, &start);
>> if (status != EFI_SUCCESS) {
>> - status = low_alloc(hdr->init_size, hdr->kernel_alignment,
>> - &start);
>> + status = low_alloc(sys_table, hdr->init_size,
>> + hdr->kernel_alignment, &start);
>> if (status != EFI_SUCCESS)
>> - efi_printk("Failed to alloc mem for kernel\n");
>> + efi_printk(sys_table, "Failed to alloc mem for kernel\n");
>> }
>>
>> if (status == EFI_SUCCESS)
>> @@ -737,14 +738,15 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
>> EFI_LOADER_DATA, sizeof(*gdt),
>> (void **)&gdt);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for gdt structure\n");
>> + efi_printk(sys_table, "Failed to alloc mem for gdt structure\n");
>> goto fail;
>> }
>>
>> gdt->size = 0x800;
>> - status = low_alloc(gdt->size, 8, (unsigned long *)&gdt->address);
>> + status = low_alloc(sys_table, gdt->size, 8,
>> + (unsigned long *)&gdt->address);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for gdt\n");
>> + efi_printk(sys_table, "Failed to alloc mem for gdt\n");
>> goto fail;
>> }
>>
>> @@ -752,7 +754,7 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
>> EFI_LOADER_DATA, sizeof(*idt),
>> (void **)&idt);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for idt structure\n");
>> + efi_printk(sys_table, "Failed to alloc mem for idt structure\n");
>> goto fail;
>> }
>>
>> diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c
>> index 47891bd..bd6c1a2 100644
>> --- a/drivers/firmware/efi/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/efi-stub-helper.c
>> @@ -19,15 +19,16 @@ struct initrd {
>>
>>
>>
>> -static void efi_char16_printk(efi_char16_t *str)
>> +static void efi_char16_printk(efi_system_table_t *sys_table_arg,
>> + efi_char16_t *str)
>> {
>> struct efi_simple_text_output_protocol *out;
>>
>> - out = (struct efi_simple_text_output_protocol *)sys_table->con_out;
>> + out = (struct efi_simple_text_output_protocol *)sys_table_arg->con_out;
>> efi_call_phys2(out->output_string, out, str);
>> }
>>
>> -static void efi_printk(char *str)
>> +static void efi_printk(efi_system_table_t *sys_table_arg, char *str)
>> {
>> char *s8;
>>
>> @@ -37,15 +38,17 @@ static void efi_printk(char *str)
>> ch[0] = *s8;
>> if (*s8 == '\n') {
>> efi_char16_t nl[2] = { '\r', 0 };
>> - efi_char16_printk(nl);
>> + efi_char16_printk(sys_table_arg, nl);
>> }
>>
>> - efi_char16_printk(ch);
>> + efi_char16_printk(sys_table_arg, ch);
>> }
>> }
>>
>>
>> -static efi_status_t __get_map(efi_memory_desc_t **map, unsigned long *map_size,
>> +static efi_status_t __get_map(efi_system_table_t *sys_table_arg,
>> + efi_memory_desc_t **map,
>> + unsigned long *map_size,
>> unsigned long *desc_size)
>> {
>> efi_memory_desc_t *m = NULL;
>> @@ -60,20 +63,20 @@ again:
>> * allocation which may be in a new descriptor region.
>> */
>> *map_size += sizeof(*m);
>> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
>> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
>> EFI_LOADER_DATA, *map_size, (void **)&m);
>> if (status != EFI_SUCCESS)
>> goto fail;
>>
>> - status = efi_call_phys5(sys_table->boottime->get_memory_map, map_size,
>> - m, &key, desc_size, &desc_version);
>> + status = efi_call_phys5(sys_table_arg->boottime->get_memory_map,
>> + map_size, m, &key, desc_size, &desc_version);
>> if (status == EFI_BUFFER_TOO_SMALL) {
>> - efi_call_phys1(sys_table->boottime->free_pool, m);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, m);
>> goto again;
>> }
>>
>> if (status != EFI_SUCCESS)
>> - efi_call_phys1(sys_table->boottime->free_pool, m);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, m);
>>
>> fail:
>> *map = m;
>> @@ -83,8 +86,9 @@ fail:
>> /*
>> * Allocate at the highest possible address that is not above 'max'.
>> */
>> -static efi_status_t high_alloc(unsigned long size, unsigned long align,
>> - unsigned long *addr, unsigned long max)
>> +static efi_status_t high_alloc(efi_system_table_t *sys_table_arg,
>> + unsigned long size, unsigned long align,
>> + unsigned long *addr, unsigned long max)
>> {
>> unsigned long map_size, desc_size;
>> efi_memory_desc_t *map;
>> @@ -93,7 +97,7 @@ static efi_status_t high_alloc(unsigned long size, unsigned long align,
>> u64 max_addr = 0;
>> int i;
>>
>> - status = __get_map(&map, &map_size, &desc_size);
>> + status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
>> if (status != EFI_SUCCESS)
>> goto fail;
>>
>> @@ -139,7 +143,7 @@ again:
>> if (!max_addr)
>> status = EFI_NOT_FOUND;
>> else {
>> - status = efi_call_phys4(sys_table->boottime->allocate_pages,
>> + status = efi_call_phys4(sys_table_arg->boottime->allocate_pages,
>> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
>> nr_pages, &max_addr);
>> if (status != EFI_SUCCESS) {
>> @@ -152,7 +156,7 @@ again:
>> }
>>
>> free_pool:
>> - efi_call_phys1(sys_table->boottime->free_pool, map);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, map);
>>
>> fail:
>> return status;
>> @@ -161,7 +165,8 @@ fail:
>> /*
>> * Allocate at the lowest possible address.
>> */
>> -static efi_status_t low_alloc(unsigned long size, unsigned long align,
>> +static efi_status_t low_alloc(efi_system_table_t *sys_table_arg,
>> + unsigned long size, unsigned long align,
>> unsigned long *addr)
>> {
>> unsigned long map_size, desc_size;
>> @@ -170,7 +175,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
>> unsigned long nr_pages;
>> int i;
>>
>> - status = __get_map(&map, &map_size, &desc_size);
>> + status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
>> if (status != EFI_SUCCESS)
>> goto fail;
>>
>> @@ -203,7 +208,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
>> if ((start + size) > end)
>> continue;
>>
>> - status = efi_call_phys4(sys_table->boottime->allocate_pages,
>> + status = efi_call_phys4(sys_table_arg->boottime->allocate_pages,
>> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
>> nr_pages, &start);
>> if (status == EFI_SUCCESS) {
>> @@ -216,17 +221,18 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
>> status = EFI_NOT_FOUND;
>>
>> free_pool:
>> - efi_call_phys1(sys_table->boottime->free_pool, map);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, map);
>> fail:
>> return status;
>> }
>>
>> -static void low_free(unsigned long size, unsigned long addr)
>> +static void low_free(efi_system_table_t *sys_table_arg, unsigned long size,
>> + unsigned long addr)
>> {
>> unsigned long nr_pages;
>>
>> nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
>> - efi_call_phys2(sys_table->boottime->free_pages, addr, nr_pages);
>> + efi_call_phys2(sys_table_arg->boottime->free_pages, addr, nr_pages);
>> }
>>
>>
>> @@ -236,7 +242,8 @@ static void low_free(unsigned long size, unsigned long addr)
>> * We only support loading an initrd from the same filesystem as the
>> * kernel image.
>> */
>> -static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
>> +static efi_status_t handle_ramdisks(efi_system_table_t *sys_table_arg,
>> + efi_loaded_image_t *image,
>> struct setup_header *hdr)
>> {
>> struct initrd *initrds;
>> @@ -278,12 +285,12 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
>> if (!nr_initrds)
>> return EFI_SUCCESS;
>>
>> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
>> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
>> EFI_LOADER_DATA,
>> nr_initrds * sizeof(*initrds),
>> &initrds);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for initrds\n");
>> + efi_printk(sys_table_arg, "Failed to alloc mem for initrds\n");
>> goto fail;
>> }
>>
>> @@ -329,18 +336,18 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
>> if (!i) {
>> efi_boot_services_t *boottime;
>>
>> - boottime = sys_table->boottime;
>> + boottime = sys_table_arg->boottime;
>>
>> status = efi_call_phys3(boottime->handle_protocol,
>> image->device_handle, &fs_proto, &io);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to handle fs_proto\n");
>> + efi_printk(sys_table_arg, "Failed to handle fs_proto\n");
>> goto free_initrds;
>> }
>>
>> status = efi_call_phys2(io->open_volume, io, &fh);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to open volume\n");
>> + efi_printk(sys_table_arg, "Failed to open volume\n");
>> goto free_initrds;
>> }
>> }
>> @@ -348,9 +355,9 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
>> status = efi_call_phys5(fh->open, fh, &h, filename_16,
>> EFI_FILE_MODE_READ, (u64)0);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to open initrd file: ");
>> - efi_char16_printk(filename_16);
>> - efi_printk("\n");
>> + efi_printk(sys_table_arg, "Failed to open initrd file: ");
>> + efi_char16_printk(sys_table_arg, filename_16);
>> + efi_printk(sys_table_arg, "\n");
>> goto close_handles;
>> }
>>
>> @@ -360,30 +367,31 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
>> status = efi_call_phys4(h->get_info, h, &info_guid,
>> &info_sz, NULL);
>> if (status != EFI_BUFFER_TOO_SMALL) {
>> - efi_printk("Failed to get initrd info size\n");
>> + efi_printk(sys_table_arg, "Failed to get initrd info size\n");
>> goto close_handles;
>> }
>>
>> grow:
>> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
>> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
>> EFI_LOADER_DATA, info_sz, &info);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc mem for initrd info\n");
>> + efi_printk(sys_table_arg, "Failed to alloc mem for initrd info\n");
>> goto close_handles;
>> }
>>
>> status = efi_call_phys4(h->get_info, h, &info_guid,
>> &info_sz, info);
>> if (status == EFI_BUFFER_TOO_SMALL) {
>> - efi_call_phys1(sys_table->boottime->free_pool, info);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool,
>> + info);
>> goto grow;
>> }
>>
>> file_sz = info->file_size;
>> - efi_call_phys1(sys_table->boottime->free_pool, info);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, info);
>>
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to get initrd info\n");
>> + efi_printk(sys_table_arg, "Failed to get initrd info\n");
>> goto close_handles;
>> }
>>
>> @@ -399,16 +407,16 @@ grow:
>> * addresses in memory, so allocate enough memory for
>> * all the initrd's.
>> */
>> - status = high_alloc(initrd_total, 0x1000,
>> + status = high_alloc(sys_table_arg, initrd_total, 0x1000,
>> &initrd_addr, hdr->initrd_addr_max);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to alloc highmem for initrds\n");
>> + efi_printk(sys_table_arg, "Failed to alloc highmem for initrds\n");
>> goto close_handles;
>> }
>>
>> /* We've run out of free low memory. */
>> if (initrd_addr > hdr->initrd_addr_max) {
>> - efi_printk("We've run out of free low memory\n");
>> + efi_printk(sys_table_arg, "We've run out of free low memory\n");
>> status = EFI_INVALID_PARAMETER;
>> goto free_initrd_total;
>> }
>> @@ -428,7 +436,7 @@ grow:
>> initrds[j].handle,
>> &chunksize, addr);
>> if (status != EFI_SUCCESS) {
>> - efi_printk("Failed to read initrd\n");
>> + efi_printk(sys_table_arg, "Failed to read initrd\n");
>> goto free_initrd_total;
>> }
>> addr += chunksize;
>> @@ -440,7 +448,7 @@ grow:
>>
>> }
>>
>> - efi_call_phys1(sys_table->boottime->free_pool, initrds);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, initrds);
>>
>> hdr->ramdisk_image = initrd_addr;
>> hdr->ramdisk_size = initrd_total;
>> @@ -448,13 +456,13 @@ grow:
>> return status;
>>
>> free_initrd_total:
>> - low_free(initrd_total, initrd_addr);
>> + low_free(sys_table_arg, initrd_total, initrd_addr);
>>
>> close_handles:
>> for (k = j; k < i; k++)
>> efi_call_phys1(fh->close, initrds[k].handle);
>> free_initrds:
>> - efi_call_phys1(sys_table->boottime->free_pool, initrds);
>> + efi_call_phys1(sys_table_arg->boottime->free_pool, initrds);
>> fail:
>> hdr->ramdisk_image = 0;
>> hdr->ramdisk_size = 0;
>> --
>> 1.7.10.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
