Re: [Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 17:18, <daniel.kiper@xxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 06:29:55AM -0600, Jan Beulich wrote:
>> >>> On 12.09.16 at 22:18, <daniel.kiper@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/efi/stub.c
>> > +++ b/xen/arch/x86/efi/stub.c
>> > @@ -3,6 +3,43 @@
>> >  #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)
>>
>> Long line.
>>
>> > +{
>> > +    CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem 
>> > halted!!!\r\n";
>> > +    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 is not
>> > +     * directly supported by C compiler and/or linker.
>>
>> So how can lack of calling convention support be due to missing
>> functionality in the linker? Please avoid such misleading comments:
>> Linker capabilities get probed solely for PE32+ output format
>> support. With the linker part dropped here,
> 
> The problem here is that we are not able to tell why stub.c is build.
> There is a chance that C compiler does not support MS x64 calling
> convention or linker is not able to emit PE32+ output or both. So,
> I think that we should say so. However, I can agree that comment
> can be improved.

The first comment (higher up, but still visible in context) already
mentions both compiler and linker. That's sufficient for the purpose
that you mention, imo. The second comment should be solely about
why this needs to be an asm(), and that has nothing to do with the
linker.

Jan


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