Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms

[Konrad Rzeszutek Wilk]

> +run_bs:
> +        push    %rax
> +        push    %rdi
> +
> +        /* Initialize BSS (no nasty surprises!). */
> +        lea     __bss_start(%rip),%rdi
> +        lea     __bss_end(%rip),%rcx
> +        sub     %rdi,%rcx
> +        shr     $3,%rcx
> +        xor     %eax,%eax
> +        rep     stosq


This means we are using the %es segment. The multiboot1 is pretty clear
that it should cover up to 4GB.

Would it be worth adding a comment about that?

> +
> +        pop     %rdi
> +
> +        /*
> +         * IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable.
> +         * OUT: %rax - multiboot2.mem_lower. Do not get this value from
> +         * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag. It could be bogus on
> +         * EFI platforms.
> +         */
> +        call    efi_multiboot2

However the function prototype for efi_multiboot2 is 'void', not 'int' - so
it would not return anything!


> +
> +        /* Convert multiboot2.mem_lower to bytes/16. */

Should we check to make sure it is valid? With those weird machines you seem to 
have
run into I am not actually sure what a valid value is :-(

.. snip..

> diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
> index c5ae369..d30fe89 100644
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -13,6 +13,11 @@ struct efi __read_mostly efi = {
>       .smbios3 = EFI_INVALID_TABLE_ADDR
>  };
>  
> +void __init efi_multiboot2(void)

unsigned int __init ..
> +{
> +    /* TODO: Fail if entered! */

And maybe just 'return 0'; ?

> +}