Message ID | 20160809133816.GA6571@wunner.de (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
On Tue, 09 Aug, at 03:38:16PM, Lukas Wunner wrote: > @@ -208,7 +201,10 @@ struct efi_config { > __pure const struct efi_config *__efi_early(void); > > #define efi_call_early(f, ...) \ > - __efi_early()->call(__efi_early()->f, __VA_ARGS__); > + __efi_early()->call(__efi_early()->is64 ? \ > + ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \ > + ((efi_boot_services_32_t *)__efi_early()->boot_services)->f, \ > + __VA_ARGS__); > You cannot use pointers from the firmware directly in mixed mode because the kernel is compiled for 64-bits but the firmware is using 32-bit addresses, so dereferencing a pointer causes a 64-bit load. That's the reason we deconstruct the tables and copy the addresses from the last level - so we don't have to jump through multiple pointers. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 15, 2016 at 12:54:14PM +0100, Matt Fleming wrote: > On Tue, 09 Aug, at 03:38:16PM, Lukas Wunner wrote: > > @@ -208,7 +201,10 @@ struct efi_config { > > __pure const struct efi_config *__efi_early(void); > > > > #define efi_call_early(f, ...) \ > > - __efi_early()->call(__efi_early()->f, __VA_ARGS__); > > + __efi_early()->call(__efi_early()->is64 ? \ > > + ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \ > > + ((efi_boot_services_32_t *)__efi_early()->boot_services)->f, \ > > + __VA_ARGS__); > > > > You cannot use pointers from the firmware directly in mixed mode > because the kernel is compiled for 64-bits but the firmware is using > 32-bit addresses, so dereferencing a pointer causes a 64-bit load. Please behold the resulting binary code, which uses a 32-bit load, not a 64-bit load (note the "mov edi, dword [ds:rax+0x2c]"). This is a call to AllocatePool *with* my patch: 0x22c1 mov rax, qword [ds:efi_early] 0x22c8 add rdx, 0x10 ; buffer size argument 0x22cc cmp byte [ds:rax+0x28], 0x0 ; !efi_early->is64 ? 0x22d0 mov r8, qword [ds:rax+0x20] ; efi_early->call() 0x22d4 mov rax, qword [ds:rax+0x10] ; efi_early->boot_services 0x22d8 je 0x2410 0x22de mov rdi, qword [ds:rax+0x40] ; allocate_pool (64 bit) 0x22e2 xor eax, eax 0x22e4 mov rcx, r13 ; buffer argument 0x22e7 mov esi, 0x2 ; EfiLoaderData argument 0x22ec call r8 ... 0x2410 mov edi, dword [ds:rax+0x2c] ; allocate_pool (32 bit) 0x2413 jmp 0x22e2 The same *without* my patch: 0x1d41 mov r8, qword [ds:efi_early] 0x1d48 add r15, 0x40 0x1d4c mov rcx, qword [ss:rsp-0x10+arg_20] ; buffer argument 0x1d51 mov rdx, r15 ; buffer size argument 0x1d54 mov esi, 0x2 ; EfiLoaderData argument 0x1d59 mov rdi, qword [ds:r8+0x10] ; allocate_pool 0x1d5d call qword [ds:r8+0x58] ; efi_early->call So it looks to me like my patch should work just fine on 32-bit, even though I cannot verify it through testing. The ARM folks afford invocation of arbitrary boot services, it just seemed natural to me to allow the same for x86. The portion of the stub code which is shared between arches cannot use more than the 8 boot services supported by x86 even though ARM would be capable of using all of them. Of course the binary code with my patch is longer, less readable, and needs to follow multiple indirections and I can understand if you would rather stay with the current approach for these reasons. But I would like to understand the "cannot jump through pointers at runtime" argument because the binary code looks to me like it should work on 32 bit. I guess I must be missing something obvious? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 15 Aug, at 06:13:58PM, Lukas Wunner wrote: > > But I would like to understand the "cannot jump through pointers at > runtime" argument because the binary code looks to me like it should > work on 32 bit. I guess I must be missing something obvious? Ah no, I forgot that efi_boot_services_{32,64}_t doesn't contain pointers - it contains u32/u64 objects. So yeah, your patch looks fine. It does trigger the following warnings when building for i386 though, In file included from /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:14:0: /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c: In function ‘efi_get_memory_map’: /dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:205:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \ ^ /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:85:11: note: in expansion of macro ‘efi_call_early’ status = efi_call_early(allocate_pool, EFI_LOADER_DATA, ^ /dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:206:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] ((efi_boot_services_32_t *)__efi_early()->boot_services)->f, \ ^ /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:85:11: note: in expansion of macro ‘efi_call_early’ status = efi_call_early(allocate_pool, EFI_LOADER_DATA, ^ /dev/shm/mfleming/git/efi/arch/x86/include/asm/efi.h:205:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \ ^ /dev/shm/mfleming/git/efi/drivers/firmware/efi/libstub/efi-stub-helper.c:92:11: note: in expansion of macro ‘efi_call_early’ status = efi_call_early(get_memory_map, map_size, m, ^ etc. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index ec6d2ef..2e0189c 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -29,22 +29,11 @@ __pure const struct efi_config *__efi_early(void) static void setup_boot_services##bits(struct efi_config *c) \ { \ efi_system_table_##bits##_t *table; \ - efi_boot_services_##bits##_t *bt; \ \ table = (typeof(table))sys_table; \ \ + c->boot_services = table->boottime; \ c->text_output = table->con_out; \ - \ - bt = (typeof(bt))(unsigned long)(table->boottime); \ - \ - c->allocate_pool = bt->allocate_pool; \ - c->allocate_pages = bt->allocate_pages; \ - c->get_memory_map = bt->get_memory_map; \ - c->free_pool = bt->free_pool; \ - c->free_pages = bt->free_pages; \ - c->locate_handle = bt->locate_handle; \ - c->handle_protocol = bt->handle_protocol; \ - c->exit_boot_services = bt->exit_boot_services; \ } BOOT_SERVICES(32); BOOT_SERVICES(64); diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S index 1038524..fd0b6a2 100644 --- a/arch/x86/boot/compressed/head_32.S +++ b/arch/x86/boot/compressed/head_32.S @@ -82,7 +82,7 @@ ENTRY(efi_pe_entry) /* Relocate efi_config->call() */ leal efi32_config(%esi), %eax - add %esi, 88(%eax) + add %esi, 32(%eax) pushl %eax call make_boot_params @@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry) /* Relocate efi_config->call() */ leal efi32_config(%esi), %eax - add %esi, 88(%eax) + add %esi, 32(%eax) pushl %eax 2: call efi_main @@ -264,7 +264,7 @@ relocated: #ifdef CONFIG_EFI_STUB .data efi32_config: - .fill 11,8,0 + .fill 4,8,0 .long efi_call_phys .long 0 .byte 0 diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 0d80a7a..efdfba2 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -265,7 +265,7 @@ ENTRY(efi_pe_entry) /* * Relocate efi_config->call(). */ - addq %rbp, efi64_config+88(%rip) + addq %rbp, efi64_config+32(%rip) movq %rax, %rdi call make_boot_params @@ -285,7 +285,7 @@ handover_entry: * Relocate efi_config->call(). */ movq efi_config(%rip), %rax - addq %rbp, 88(%rax) + addq %rbp, 32(%rax) 2: movq efi_config(%rip), %rdi call efi_main @@ -457,14 +457,14 @@ efi_config: #ifdef CONFIG_EFI_MIXED .global efi32_config efi32_config: - .fill 11,8,0 + .fill 4,8,0 .quad efi64_thunk .byte 0 #endif .global efi64_config efi64_config: - .fill 11,8,0 + .fill 4,8,0 .quad efi_call .byte 1 #endif /* CONFIG_EFI_STUB */ diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 6b06939..306203c 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -192,14 +192,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map( struct efi_config { u64 image_handle; u64 table; - u64 allocate_pool; - u64 allocate_pages; - u64 get_memory_map; - u64 free_pool; - u64 free_pages; - u64 locate_handle; - u64 handle_protocol; - u64 exit_boot_services; + u64 boot_services; u64 text_output; efi_status_t (*call)(unsigned long, ...); bool is64; @@ -208,7 +201,10 @@ struct efi_config { __pure const struct efi_config *__efi_early(void); #define efi_call_early(f, ...) \ - __efi_early()->call(__efi_early()->f, __VA_ARGS__); + __efi_early()->call(__efi_early()->is64 ? \ + ((efi_boot_services_64_t *)__efi_early()->boot_services)->f : \ + ((efi_boot_services_32_t *)__efi_early()->boot_services)->f, \ + __VA_ARGS__); #define __efi_call_early(f, ...) \ __efi_early()->call((unsigned long)f, __VA_ARGS__);