Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms

[Andrew Cooper <andrew.cooper3@citrix.com>]

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