[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms
From: |
Andrew Cooper |
Subject: |
Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms |
Date: |
Sat, 31 Jan 2015 00:47:44 +0000 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 30/01/2015 23:43, Daniel Kiper wrote:
> On Fri, Jan 30, 2015 at 07:06:53PM +0000, Andrew Cooper wrote:
>> On 30/01/15 17:54, Daniel Kiper wrote:
>>> +
>>> +efi_multiboot2_proto:
>>> + /* Skip Multiboot2 information fixed part */
>>> + lea MB2_fixed_sizeof(%ebx),%ecx
>>> +
>>> +0:
>>> + /* Get mem_lower from Multiboot2 information */
>>> + cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx)
>>> + jne 1f
>>> +
>>> + mov MB2_mem_lower(%ecx),%edx
>>> + jmp 4f
>>> +
>>> +1:
>>> + /* Get EFI SystemTable address from Multiboot2 information */
>>> + cmpl $MULTIBOOT2_TAG_TYPE_EFI64,(%ecx)
>>> + jne 2f
>>> +
>>> + lea MB2_efi64_st(%ecx),%esi
>>> + lea efi_st(%rip),%edi
>>> + movsq
>> This is complete overkill for copying a 64bit variable out of the tag
>> and into a local variable. Just use a plain 64bit load and store.
> I am not sure what do you mean by "64bit load and store" but I have
> just realized that we do not need these variables. They are remnants
> from early developments when I thought that we need ImageHandle
> and SystemTable here and later somewhere else.
mov MB2_efi64_st(%rcx), %rdi
mov %rdi, efi_st(%rip)
But if they are not needed, drop the code completely.
>>> + jmp 4f
>>> +
>>> +3:
>>> + /* Is it the end of Multiboot2 information? */
>>> + cmpl $MULTIBOOT2_TAG_TYPE_END,(%ecx)
>>> + je run_bs
>>> +
>>> +4:
>>> + /* Go to next Multiboot2 information tag */
>>> + add MB2_tag_size(%ecx),%ecx
>>> + add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
>>> + and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>>> + jmp 0b
>>> +
>>> +run_bs:
>>> + push %rax
>>> + push %rdx
>> Does the EFI spec guarantee that we have a good stack to use at this point?
> Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
> section 2.3.4, x64 Platforms says: During boot services time the processor
> is in the following execution mode: ..., 128 KiB, or more, of available
> stack space. GRUB2 uses this stack too and do not move it to different
> memory region. So, I think that here we are on safe side.
Sounds ok then.
>
>>> + /* Initialize BSS (no nasty surprises!) */
>>> + lea __bss_start(%rip),%rdi
>>> + lea _end(%rip),%rcx
>>> + sub %rdi,%rcx
>>> + xor %rax,%rax
>> xor %eax,%eax is shorter.
>>
>>> + rep stosb
>> It would be more efficient to make sure that the linker aligns
>> __bss_start and _end on 8 byte boundaries, and use stosq instead.
> Right but just for this. Is it pays? We do this only once.
The BSS in Xen is 300k. It is absolutely better to clear it 8 bytes at
a time rather than 1.
> However, if you wish...
>
>>> + mov efi_ih(%rip),%rdi /* EFI ImageHandle */
>>> + mov efi_st(%rip),%rsi /* EFI SystemTable */
>>> + call efi_multiboot2
>>> +
>>> + pop %rcx
>>> + pop %rax
>>> +
>>> + shl $10-4,%rcx /* Convert multiboot2.mem_lower to
>>> bytes/16 */
>>> +
>>> + cli
>> This looks suspiciously out of place. Surely the EFI spec doesn't
>> permit entry with interrupts enabled?
> Unified Extensible Firmware Interface Specification, Version 2.4 Errata B,
> section 2.3.4, x64 Platforms says: During boot services time the processor
> is in the following execution mode: ..., Interrupts are enabled–though no
> interrupt services are supported other than the UEFI boot services timer
> functions (All loaded device drivers are serviced synchronously by
> “polling.”).
> So, I think that we should use BS with interrupts enabled and disable
> them after ExitBootServices(). Hmmm... Now I think that we should use
> cli immediately after efi_multiboot2() call.
I presume then that the firmware has set up a valid idt somewhere and is
actually serving any interrupts we get.
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index f8be3dd..c5725ca 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -75,6 +75,17 @@ static size_t wstrlen(const CHAR16 * s);
> static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
> static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>
> +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable);
> +static void efi_console_set_mode(void);
> +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
> +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> + UINTN cols, UINTN rows, UINTN depth);
> +static void efi_tables(void);
> +static void setup_efi_pci(void);
> +static void efi_variables(void);
> +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN
> gop_mode);
> +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
> *SystemTable);
> +
>> If any of these forward declarations are needed, they should be
> They are needed because efi-boot.h is included before above
> mentioned functions definitions.
>
>> introduced in the appropriate create efi_$FOO patch. However, I can't
> I thought about that during development. However, I stated that if I do what
> you suggest then it is not clear who needs/uses these forward declarations.
>
>> spot a need for any of them.
> efi-boot.h:efi_multiboot2() uses them.
Ah - I see now.
~Andrew
- [PATCH 10/18] efi: create efi_console_set_mode(), (continued)
- [PATCH 10/18] efi: create efi_console_set_mode(), Daniel Kiper, 2015/01/30
- [PATCH 11/18] efi: create efi_get_gop(), Daniel Kiper, 2015/01/30
- [PATCH 13/18] efi: create efi_tables(), Daniel Kiper, 2015/01/30
- [PATCH 14/18] efi: create efi_variables(), Daniel Kiper, 2015/01/30
- [PATCH 16/18] efi: create efi_exit_boot(), Daniel Kiper, 2015/01/30
- [PATCH 15/18] efi: create efi_set_gop_mode(), Daniel Kiper, 2015/01/30
- [PATCH 17/18] x86/efi: create new early memory allocator, Daniel Kiper, 2015/01/30
- [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms, Daniel Kiper, 2015/01/30
- Re: [PATCH 00/18] x86: multiboot2 protocol support, Daniel Kiper, 2015/01/30
- Re: [PATCH 00/18] x86: multiboot2 protocol support, João Jerónimo, 2015/01/31