Re: [PATCH v13 4/9] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@suse.com>]

>>> On 31.01.17 at 15:23, <daniel.kiper@oracle.com> wrote:
> On Tue, Jan 31, 2017 at 05:33:12AM -0700, Jan Beulich wrote:
>> >>> On 25.01.17 at 23:49, <daniel.kiper@oracle.com> wrote:
>> > On Wed, Jan 25, 2017 at 04:20:30PM -0600, Doug Goldstein wrote:
>> > @@ -123,6 +116,15 @@ efi_platform:
>> >  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>> >  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>> >
>> > +        .section .init.data, "a", @progbits
>>
>> This needs to be a writable section.
> 
> AIUI, it is by default but if you wish I can replace "a" with "aw" here.

To me it would smell like a binutils bug if any flag was added despite
the explicit setting here. Things would be different if you omitted
those arguments.

>> > +        /* Get topmost low-memory stack address. */
>> > +        add     $TRAMPOLINE_SPACE,%ecx
>>
>> The top-most address of the stack is
>> %ecx + TRAMPOLINE_SPACE + TRAMPOLINE_STACK_SPACE - 1.
>> Please don't add misleading comments.
> 
> Right, it is misleading. Do you think that:
> 
> Get the lowest low-memory stack address.
> 
> ...is better?

That or "bottom-most" (to avoid the double "low").

>> >          /*
>> > +         * Now trampoline_phys points to the following structure (lowest
>> > +         * address is at the top):
>> > +         *
>> > +         * +------------------------+
>> > +         * |    TRAMPOLINE_SPACE    |
>> > +         * +- - - - - - - - - - - - +
>> > +         * |       mbi struct       |
>> > +         * +------------------------+
>> > +         * | TRAMPOLINE_STACK_SPACE |
>> > +         * +------------------------+
>> > +         *
>> > +         * mbi struct lives at the end of TRAMPOLINE_SPACE. The rest of
>> > +         * TRAMPOLINE_SPACE is reserved for trampoline code and data.
>> > +         */
>>
>> Please can you clarify here that the MBI data grows downwards
>> from the beginning of the stack to the end of the trampoline?
> 
> OK, but I think that "beginning of the stack" should be replaced
> with "the end of TRAMPOLINE_SPACE" here.

That would be in lines with the #define-s, but not with the drawing
above (which btw also applies to the comment that's there above).

>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -20,7 +20,8 @@
>> >  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[] =
>> > +        L"(XEN) Xen does not have EFI code build in!\r\n(XEN) System halted!\r\n";
>>
>> Why did you add these (XEN) prefixes?
> 
> To align message format with messages printed from most places. And I realized
> that it will be nice to do the same thing in head.S and the rest of EFI code.
> This way it is much easier to differentiate between a bootloader and Xen messages.

I disagree. Those prefixes should be used only for messages ending
up in Xen's log.

>> > --- a/xen/arch/x86/xen.lds.S
>> > +++ b/xen/arch/x86/xen.lds.S
>> > @@ -334,5 +334,5 @@ ASSERT(IS_ALIGNED(trampoline_end,   4), "trampoline_end
>> > misaligned")
>> >  ASSERT(IS_ALIGNED(__bss_start,      8), "__bss_start misaligned")
>> >  ASSERT(IS_ALIGNED(__bss_end,        8), "__bss_end misaligned")
>> >
>> > -ASSERT((trampoline_end - trampoline_start) < MB_TRAMPOLINE_SIZE,
>> > +ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE,
>> >      "not enough room for trampoline")
>>
>> Didn't you mean to make sure there are at least two pages for
>> MBI data?
> 
> Do you wish plain number here or constant like MBI_SIZE defined somewhere.
> I think that constant is better thing.

I'd prefer MBI_SPACE_MIN (or whatever else making clear that this
is a bound, not an exact size).

Jan


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