Re: [Xen-devel] [PATCH v12 05/10] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.17 at 15:43, <daniel.kiper@xxxxxxxxxx> wrote:
> On Fri, Jan 20, 2017 at 07:10:32AM -0700, Jan Beulich wrote:
>> >>> 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:
>> >> 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.
> 
> If we use TRAMPOLINE_SPACE only for both trampoline and its stack
> then ASSERT() is not very reliable. There is chance that trampoline
> code will fill all space specified in TRAMPOLINE_SPACE and then there
> will be no space for stack at all. However, I can agree that chance
> is small if we assume 64 KiB space for trampoline and trampoline size
> is about 10 KiB right now.

The ASSERT() is the only place you need the stack size, so just
encode (subtract) it there (like iirc Doug had done).

>> >> >> 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.
> 
> Do you think that we should have more than 8 KiB there? Anyway,
> I am more afraid about memory map size here.

The question is how flexible we want to be. If the size is fixed,
you need to make your code stop using space beyond that size.

>> >> 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
> 
> IIRC, it is needed only during init phase. Probably later it can be dropped.

That's way too imprecise.

>> may be places to put it without much risk of overflowing anything.
> 
> I am not sure about which places do you think.

There are a number of relatively large internal buffers which could
be used for this purpose if the uses of the MBI data all live before
the first use of whichever such buffer we would select as victim.
Otherwise, just go down from e.g. trampoline+60k until you reach
trampoline_end.

Jan


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