|
[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 05:40:55AM -0700, Jan Beulich wrote:
> >>> On 20.01.17 at 12:43, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Fri, Jan 20, 2017 at 02:46:47AM -0700, Jan Beulich wrote:
> >> >>> On 20.01.17 at 02:34, <daniel.kiper@xxxxxxxxxx> wrote:
[...]
> >> > +.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.
>
> Okay, that's good enough for now, even if it escapes me how that's
> supposed to work on systems without any memory below 4Gb.
If you wish I can add relevant comment somewhere. Anyway, if there is no
mem below 4 GiB at least, IIRC, GRUB2 will fail during image load. So,
no worry. At least right now.
[...]
> >> > + /*
> >> > + * 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.
>
> That would imply they do a call (and not a jmp), which would make
> the present code correct afaict. As said, imo there should still be a
> warning added here, for anyone wanting to modify the stack layout.
Will do.
[...]
> >> > --- 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.
>
> You already don't touch it, and I see no reason why this should
> change. Yet this is orthogonal to the naming of the constants here.
OK.
> As said, the trampoline itself is always the same, and the stack
Yep.
> portion of it is simply unused in the native EFI loader case. Plus
Hmmm... That is interesting why? I must take a look at trampoline code.
> MB_TRAMPOLINE_SIZE is in no way the size of the trampoline in the
> first place. Perhaps TRAMPOLINE_SPACE (and then covering both
> parts)?
So, I suppose that TRAMPOLINE_SPACE and TRAMPOLINE_STACK_SPACE is
OK for you? And I can agree that this is better naming.
> >> 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.
>
> Even with a couple of dozen modules passed? But the primary
Why not? The biggest thing is memory map. The rest are mostly
command line strings and a few pointers. Modules itself live
in different memory regions.
> question was left unanswered anyway: Does this need placing in
> the low megabyte at all? And even if it does, especially if you
I do not think so. I do not expect that anything in trampoline code
touches it. So, we can put it anywhere below 4 GiB. However, then
we need an allocator to properly allocate mbi struct region. And
at this boot stage this is not an easy thing.
> limit it to 8k, I don't see why it wouldn't fit inside the 64k
> trampoline area.
If you wish why not. Though I think that we should use set of constants
here to avoid currently existing plain numbers mess in assembly. And
I have a feeling that this cleanup/fix should land into separate patch.
Daniel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |