Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1462730 > unrolled thread

Re: [PATCH 0/6] Apple device properties

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/6] Apple device properties Matt Fleming <matt@codeblueprint.co.uk> - 2016-08-15 14:00 +0200
    Re: [PATCH 0/6] Apple device properties Lukas Wunner <lukas@wunner.de> - 2016-08-15 18:20 +0200
      Re: [PATCH 0/6] Apple device properties Matt Fleming <matt@codeblueprint.co.uk> - 2016-08-19 03:00 +0200
        Re: [PATCH 0/6] Apple device properties Lukas Wunner <lukas@wunner.de> - 2016-08-22 12:00 +0200
          Re: [PATCH 0/6] Apple device properties Matt Fleming <matt@codeblueprint.co.uk> - 2016-08-24 22:00 +0200

#1462730 — Re: [PATCH 0/6] Apple device properties

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.

[toc] | [next] | [standalone]

#1462971

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

[toc] | [prev] | [next] | [standalone]

#1465683

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.

[toc] | [prev] | [next] | [standalone]

#1467514

On Thu, Aug 18, 2016 at 09:34:33PM +0100, Matt Fleming wrote:
> 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 : \
>    ^

Right, sorry, I didn't compile-test that version on x86_32.

I'm sending out a new version now which compiles cleanly in all three
cases (x86_32, x86_64 with and without mixed-mode), works fine on my
64-bit EFI and the 32-bit code at least *looks* okay when disassembled.

By the way, arch/x86/Kconfig says that "it is not possible to boot a
mixed-mode enabled kernel via the EFI boot stub - a bootloader that
supports the EFI handover protocol must be used".

Is this still correct? With all the mixed-mode support in head_64.S
and eboot.c, I'm wondering what's missing?

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1469673

On Mon, 22 Aug, at 11:58:50AM, Lukas Wunner wrote:
> By the way, arch/x86/Kconfig says that "it is not possible to boot a
> mixed-mode enabled kernel via the EFI boot stub - a bootloader that
> supports the EFI handover protocol must be used".
> 
> Is this still correct? With all the mixed-mode support in head_64.S
> and eboot.c, I'm wondering what's missing?

Yes, that's still correct.

The EFI boot stub technically refers to the feature of having the
firmware load the kernel directly, without the use of a boot loader.

In that scenario you need the kernel and firmware to agree on a CPU
mode since the kernel has no way to figure out if it needs to switch
or not. Nor does it have a direct way to figure out what bitness the
firmware is.

[ Yes, technically we could add code to the EFI stub to detect the
  current mode and perform the switch, we just don't have anything
  like that right now ]

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web