[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 20.01.17 at 14:46, <daniel.kiper@xxxxxxxxxx> wrote:
> 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:
>> >> > --- 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.

It's a boot time only thing, and the boot path if different for
native EFI.

>> 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.

Again, I don't see why you would need TRAMPOLINE_STACK_SPACE.

>> >> 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.

Command line string may get lengthy.

>> 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.

Depending for how long you need that data to stay around, there
may be places to put it without much risk of overflowing anything.

>> 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.

Constants - sure. Separate patch - fine with me.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.