[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.
Roy Franz
roy.franz at linaro.org
Fri Aug 30 17:46:07 EDT 2013
On Fri, Aug 30, 2013 at 5:53 AM, Grant Likely <grant.likely at secretlab.ca> wrote:
> On Fri, 9 Aug 2013 16:26:03 -0700, Roy Franz <roy.franz at linaro.org> wrote:
>> Signed-off-by: Roy Franz <roy.franz at linaro.org>
>
> 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
More information about the linux-arm-kernel
mailing list