|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 05/10] x86: add multiboot2 protocol support for EFI platforms
On Fri, Jan 20, 2017 at 02:46:47AM -0700, Jan Beulich wrote:
> >>> On 20.01.17 at 02:34, <daniel.kiper@xxxxxxxxxx> wrote:
> > @@ -100,20 +107,48 @@ multiboot2_header_start:
> > gdt_boot_descr:
> > .word 6*8-1
> > .long sym_phys(trampoline_gdt)
> > + .long 0 /* Needed for 64-bit lgdt */
> > +
> > + .align 4
> > +vga_text_buffer:
> > + .long 0xb8000
> > +
> > +efi_platform:
> > + .byte 0
>
> You mean to modify these fields, but you add them to a r/o section.
Yep. So, I think that we should move them to .init.data. Is it OK for you?
> > +.Lefi_multiboot2_proto:
> > + /* Zero EFI SystemTable and EFI ImageHandle addresses. */
> > + xor %esi,%esi
> > + xor %edi,%edi
> > +
> > + /* Skip Multiboot2 information fixed part. */
> > + lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%ecx
>
> In an earlier reply to Andrew's inquiry regarding the possible
> truncation here you said that grub can be made obey to such a
> load restriction. None of the tags added here or in patch 2
> appear to have such an effect, so would you please clarify how
> the address restriction is being enforced?
GRUB2 itself does allocations for all multiboot2 stuff always below 4 GiB.
So, there is no need for extra tags.
Additionally, multiboot2 spec says this:
The bootloader must not load any part of the kernel, the modules, the Multiboot2
information structure, etc. higher than 4 GiB - 1. This requirement is put in
force because most of currently specified tags supports 32-bit addresses only.
Additionally, some kernels, even if they run on EFI 64-bit platform, still
execute some parts of its initialization code in 32-bit mode.
[...]
> > +.Lefi_mb2_next_tag:
> > + /* Go to next Multiboot2 information tag. */
> > + add MB2_tag_size(%rcx),%ecx
> > + add $(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > + and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
> > + jmp .Lefi_mb2_tsize
> > +
> > +run_bs:
>
> .Lrun_bs:
OK.
> > + /* Are EFI boot services available? */
> > + cmpb $0,efi_platform(%rip)
> > + jnz 0f
> > +
> > + /* Jump to .Lmb2_no_bs after switching CPU to x86_32 mode. */
> > + lea .Lmb2_no_bs(%rip),%edi
> > + jmp x86_32_switch
> > +0:
>
> I realize you need to avoid clobbering %rdi in the non-error case,
> but the register choice seems sub-optimal: If you used a register
> which you can clobber here (e.g. %edx as it seems, using %edi in
> its place at x86_32_switch and cs32_switch), you could simplify
> this to
>
> cmpb ...
> lea ...
> je x86_32_switch
>
> at once avoiding all the numeric labels here.
If you do not care that it will be always loaded then OK. However, I think
that %r15d is a bit better here because if we need to add another argument
to efi_multiboot2() and we use %edx then we must change code in more places.
Of course we should do "mov %r15d,%edi" after x86_32_switch label then.
> > +2:
> > + push %rax
> > + push %rdi
> > +
> > + /*
> > + * Initialize BSS (no nasty surprises!).
> > + * It must be done earlier than in BIOS case
> > + * because efi_multiboot2() touches it.
> > + */
> > + lea __bss_start(%rip),%edi
> > + lea __bss_end(%rip),%ecx
> > + sub %edi,%ecx
> > + shr $3,%ecx
> > + xor %eax,%eax
> > + rep stosq
> > +
> > + pop %rdi
> > +
> > + /*
> > + * efi_multiboot2() is called according to System V AMD64 ABI:
> > + * - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> > + * - OUT: %rax - highest allocated memory address below 1 MiB;
> > + * memory below this address is used for trampoline
> > + * stack, trampoline itself and as a storage for
> > + * mbi struct created in reloc().
> > + *
> > + * MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag is not provided
> > + * on EFI platforms. Hence, it could not be used like
> > + * on legacy BIOS platforms.
> > + */
> > + call efi_multiboot2
>
> Now that we had a need for commit f6b7fedc89 ("x86/EFI: meet
> further spec requirements for runtime calls") I'm worried about stack
> alignment here. Does GrUB call or jmp to our entry point (and is that
> firmly specified to be the case)? Perhaps it would be a good idea to
> align the stack earlier on in any case. Or if not (and if alignment at
> this call is ABI conforming), a comment should be left here to warn
> people of future modifications to the amount of items pushed onto /
> popped off the stack.
Multiboot2 spec requires that stack, among others, is passed to loaded
image according to UEFI spec. Though, IIRC, there are no extra stack checks
there. Maybe we should add something to GRUB2 if it does not exists.
> > +trampoline_setup:
> > + /* Save trampoline address for later use. */
> > shl $4, %ecx
> > mov %ecx,sym_phys(trampoline_phys)
>
> I don't think this comment is very useful.
I will drop it if you wish.
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -100,6 +100,9 @@ static void __init relocate_trampoline(unsigned long
> > phys)
> > {
> > const s32 *trampoline_ptr;
> >
> > + if ( !efi_enabled(EFI_LOADER) || trampoline_phys )
> > + return;
> > +
> > trampoline_phys = phys;
> > /* Apply relocations to trampoline. */
> > for ( trampoline_ptr = __trampoline_rel_start;
> > @@ -210,12 +213,10 @@ static void *__init
> > efi_arch_allocate_mmap_buffer(UINTN map_size)
> >
> > static void __init efi_arch_pre_exit_boot(void)
> > {
> > - if ( !trampoline_phys )
> > - {
> > - if ( !cfg.addr )
> > - blexit(L"No memory for trampoline");
> > - relocate_trampoline(cfg.addr);
> > - }
> > + if ( !cfg.addr )
> > + blexit(L"No memory for trampoline");
> > +
> > + relocate_trampoline(cfg.addr);
> > }
>
> Why can't this function be left untouched, and the change to
> relocate_trampoline() above also be reduced or even be removed
> altogether?
There is pretty good chance that efi_arch_pre_exit_boot() can be left
untouched though relocate_trampoline() needs at least
if ( !efi_enabled(EFI_LOADER) )
return;
> > --- a/xen/arch/x86/efi/stub.c
> > +++ b/xen/arch/x86/efi/stub.c
> > @@ -3,6 +3,44 @@
> > #include <xen/init.h>
> > #include <xen/lib.h>
> > #include <asm/page.h>
> > +#include <asm/efibind.h>
> > +#include <efi/efidef.h>
> > +#include <efi/eficapsule.h>
> > +#include <efi/eficon.h>
> > +#include <efi/efidevp.h>
> > +#include <efi/efiapi.h>
> > +
> > +/*
> > + * Here we are in EFI stub. EFI calls are not supported due to lack
> > + * of relevant functionality in compiler and/or linker.
> > + *
> > + * efi_multiboot2() is an exception. Please look below for more details.
> > + */
> > +
> > +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
> > + EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > + CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem
> > halted!!!\r\n";
>
> static const CHAR16 __initconst err[]
>
> And please reduce the number of exclamation marks.
OK.
> > + SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
> > +
> > + StdErr = SystemTable->StdErr ? SystemTable->StdErr :
> > SystemTable->ConOut;
> > +
> > + /*
> > + * Print error message and halt the system.
> > + *
> > + * We have to open code MS x64 calling convention
> > + * in assembly because here this convention may
> > + * not be directly supported by C compiler.
> > + */
> > + asm volatile(
> > + " call *%2 \n"
> > + "0: hlt \n"
> > + " jmp 0b \n"
> > + : "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
>
> The "g" really wants to be "rm": While the kind of expression doesn't
> really allow for an immediate, you still shouldn't give wrong examples
> of constraints (with the * now properly added to the call operand, it
> doesn't allow for an immediate anymore).
I was considering that change once but I was not sure. Though if you
think that it make sense I will do that.
> > --- a/xen/include/asm-x86/config.h
> > +++ b/xen/include/asm-x86/config.h
> > @@ -73,6 +73,11 @@
> > #define STACK_ORDER 3
> > #define STACK_SIZE (PAGE_SIZE << STACK_ORDER)
> >
> > +#define MB_TRAMPOLINE_STACK_SIZE PAGE_SIZE
> > +#define MB_TRAMPOLINE_SIZE (KB(64) - MB_TRAMPOLINE_STACK_SIZE)
>
> What's the reason for the MB_ prefixes here? The trampoline is
> always the same size, isn't it? Nor am I convinced we really need
Please take a look at efi_arch_memory_setup(). Amount of memory allocated
for trampoline and other stuff depends on boot loader type. So, when I use
"MB_" I would like underline that this is relevant for multiboot protocols.
Though I think that we can use the same size for all protocols. However,
I would not like to touch native EFI loader case.
> two defines - except in the link time assertion you always use
> the sum of the two.
>
> > +#define MBI_SIZE (2 * PAGE_SIZE)
>
> On top of Doug's question - if it is needed at all, what does this
Please look around reloc() call in xen/arch/x86/boot/head.S. You quickly
realize that there is following memory layout from top to bottom:
+------------------+
| TRAMPOLINE_STACK |
+------------------+
| TRAMPOLINE |
+------------------+
| mbi struct |
+------------------+
> match up with, i.e. how come this is 2 pages (and not 1 or 3)?
Just thought that 8 KiB will be sufficient for Xen/modules arguments,
memory map and other minor things.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |