Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 28.09.16 at 11:39,  wrote:
> On Wed, Sep 28, 2016 at 02:57:10AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 20:11,  wrote:
>> > On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
>> >> >>> On 23.09.16 at 23:47,  wrote:
>> >> > +/*
>> >> > + * Initialize BSS (no nasty surprises!).
>> >> > + * It must be done earlier than in BIOS case
>> >> > + * because efi_multiboot2() touches it.
>> >> > + */
>> >> > +lea .startof.(.bss)(%rip),%edi
>> >> > +mov $.sizeof.(.bss),%ecx
>> >> > +shr $3,%ecx
>> >> > +xor %eax,%eax
>> >> > +rep stosq
>> >> > +
>> >> > +pop %rdi
>> >> > +
>> >> > +/*
>> >> > + * efi_multiboot2() is called according to System V AMD64 ABI:
>> >> > + *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
>> >> > + *   - OUT: %rax - highest usable memory address below 1 MiB;
>> >> > + * memory above this address is reserved for 
>> >> > trampoline;
>> >> > + * memory below this address is used for stack 
>> >> > and as
>> >> > + * a storage for boot data.
>> >>
>> >> How can you validly use memory below this address? (And I'd like to
>> >
>> > Right, I should not do that blindly. As quick fix we can check in
>> > efi_arch_process_memory_map()
>> > that chosen memory region has size cfg.size plus let's say 64 KiB. Is it 
>> > sufficient
>> > for you?
>>
>> Depends. Part of my problem here is that you convert, in your answer,
>> my "validly" to "blindly". And then I'd like to see especially "storage for
> 
> I am not sure why the difference, IMO minor, is important for you here.

Because I questioned any access to that area, yet you suggested
accesses are okay with a little more checking.

>> boot data" better qualified: What exactly is it that you mean to store
>> there?
> 
> mbi struct created in reloc.c from original multiboot(2) data passed by boot 
> loader.

Ah, right. Please make the comment less vague then. And this then
also gives you/us a means to determine what size would need to be
available (64k looks to be way too much for mb1, but mb2 of course
is much less bounded).

Jan


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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper]

On Wed, Sep 28, 2016 at 02:57:10AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 20:11,  wrote:
> > On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >> >>> On 23.09.16 at 23:47,  wrote:
> >> > This way Xen can be loaded on EFI platforms using GRUB2 and
> >> > other boot loaders which support multiboot2 protocol.
> >> >
> >> > Signed-off-by: Daniel Kiper 
> >> > Acked-by: Jan Beulich 
> >> > ---
> >> > v7 - suggestions/fixes:
> >> >- do not allocate twice memory for trampoline if we were
> >> >  loaded via multiboot2 protocol on EFI platform,
> >>
> >> If you fix bugs not noticed by a reviewer but by yourself, please
> >> drop all acks/R-b-s covering the code in question. And then I'm
> >
> > OK.
> >
> >> afraid I haven't even been able to locate that change - could you
> >> please point out what you did where?
> >
> > The change is very subtle. I add trampoline_setup label behind
> >
> >  /* From arch/x86/smpboot.c: start_eip had better be page-aligned! 
> > */
> >  xor %cl, %cl
> >
> > instead of
> >
> >  cmp %ecx,%edx   /* compare with BDA value */
> >  cmovb   %edx,%ecx   /* and use the smaller */
>
> So how did you expect anyone having looked at previous version
> to spot that? Please, in your changes section, rather than
> tediously listing who suggested which change, please make that
> section useful for incremental reviews.

I think that in general it is useful, however, I can agree that
this thing could be described better.

> >> > +/*
> >> > + * Initialize BSS (no nasty surprises!).
> >> > + * It must be done earlier than in BIOS case
> >> > + * because efi_multiboot2() touches it.
> >> > + */
> >> > +lea .startof.(.bss)(%rip),%edi
> >> > +mov $.sizeof.(.bss),%ecx
> >> > +shr $3,%ecx
> >> > +xor %eax,%eax
> >> > +rep stosq
> >> > +
> >> > +pop %rdi
> >> > +
> >> > +/*
> >> > + * efi_multiboot2() is called according to System V AMD64 ABI:
> >> > + *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> >> > + *   - OUT: %rax - highest usable memory address below 1 MiB;
> >> > + * memory above this address is reserved for 
> >> > trampoline;
> >> > + * memory below this address is used for stack 
> >> > and as
> >> > + * a storage for boot data.
> >>
> >> How can you validly use memory below this address? (And I'd like to
> >
> > Right, I should not do that blindly. As quick fix we can check in
> > efi_arch_process_memory_map()
> > that chosen memory region has size cfg.size plus let's say 64 KiB. Is it 
> > sufficient
> > for you?
>
> Depends. Part of my problem here is that you convert, in your answer,
> my "validly" to "blindly". And then I'd like to see especially "storage for

I am not sure why the difference, IMO minor, is important for you here.

> boot data" better qualified: What exactly is it that you mean to store
> there?

mbi struct created in reloc.c from original multiboot(2) data passed by boot 
loader.

> I don't recall having noticed either this or that area being used
> as stack anywhere in the series, so I'm afraid I've overlooked

Hmmm... I do not know why I put stack here. It should be dropped from comment.

Daniel

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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 27.09.16 at 20:21,  wrote:
> On Mon, Sep 26, 2016 at 09:12:40AM -0600, Jan Beulich wrote:
>> >>> On 26.09.16 at 16:40,  wrote:
>> > On 26/09/16 15:33, Jan Beulich wrote:
>> > On 26.09.16 at 16:19,  wrote:
>> >>> On 23/09/16 22:47, Daniel Kiper wrote:
>>  +/*
>>  + * Initialize BSS (no nasty surprises!).
>>  + * It must be done earlier than in BIOS case
>>  + * because efi_multiboot2() touches it.
>>  + */
>>  +lea .startof.(.bss)(%rip),%edi
>>  +mov $.sizeof.(.bss),%ecx
>> >>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>> >>> accept Jan's patch making similar changes elsewhere.
>> >>>
>> >>> Amongst other issues, you will break the Clang build with it.
>> >> Did you verify meanwhile that this doesn't work with llvm?
>> >
>> > Yes.
>> >
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
>> > #include 
>> >
>> > static unsigned int x;
>> >
>> > int main(void)
>> > {
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> >  "mov $.sizeof.(.bss), %%rcx;"
>> >  "mov $-1, %%rax;"
>> >  "rep stosb;"
>> >  ::: "memory", "rax", "rcx", "rdi");
>> > printf("x: %#x\n", x);
>> > }
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
>> > x: 0x
>> > andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
>> > foo.c:7:18: error: unexpected token in memory operand
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> >  ^
>> > :1:16: note: instantiated into assembly here
>> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
>> > $-1, %rax;rep stosb;
>> >   ^
>> > foo.c:7:18: error: unexpected token in argument list
>> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>> >  ^
>> > :1:47: note: instantiated into assembly here
>> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
>> > $-1, %rax;rep stosb;
>> >  ^
>> > 2 errors generated.
>>
>> No, that's inline assembly, which does not match Daniel's use
>> case. That's why I referred to llvm, as with assembly sources it
>> should only be the linker which gets to see the symbols generated
>> from those constructs (and if llvm supported them I'd then consider
>> breaking out the assembly file changes from that patch of mine).
> 
> Guys, may I suggest using sym_phys(__bss_start)/sym_phys(__bss_end) as
> it is in head.S right now? If .startof.()/.sizeof.() works as expected
> and both of you accept it then later we can move to new approach. Does
> it make sense?

While I dislike taking this step backwards, I guess that's the only
viable option to move this patch closer to being ready to go in.

Jan


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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 27.09.16 at 20:11,  wrote:
> On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
>> >>> On 23.09.16 at 23:47,  wrote:
>> > This way Xen can be loaded on EFI platforms using GRUB2 and
>> > other boot loaders which support multiboot2 protocol.
>> >
>> > Signed-off-by: Daniel Kiper 
>> > Acked-by: Jan Beulich 
>> > ---
>> > v7 - suggestions/fixes:
>> >- do not allocate twice memory for trampoline if we were
>> >  loaded via multiboot2 protocol on EFI platform,
>>
>> If you fix bugs not noticed by a reviewer but by yourself, please
>> drop all acks/R-b-s covering the code in question. And then I'm
> 
> OK.
> 
>> afraid I haven't even been able to locate that change - could you
>> please point out what you did where?
> 
> The change is very subtle. I add trampoline_setup label behind
> 
>  /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
>  xor %cl, %cl
> 
> instead of
> 
>  cmp %ecx,%edx   /* compare with BDA value */
>  cmovb   %edx,%ecx   /* and use the smaller */

So how did you expect anyone having looked at previous version
to spot that? Please, in your changes section, rather than
tediously listing who suggested which change, please make that
section useful for incremental reviews.

>> > +/*
>> > + * Initialize BSS (no nasty surprises!).
>> > + * It must be done earlier than in BIOS case
>> > + * because efi_multiboot2() touches it.
>> > + */
>> > +lea .startof.(.bss)(%rip),%edi
>> > +mov $.sizeof.(.bss),%ecx
>> > +shr $3,%ecx
>> > +xor %eax,%eax
>> > +rep stosq
>> > +
>> > +pop %rdi
>> > +
>> > +/*
>> > + * efi_multiboot2() is called according to System V AMD64 ABI:
>> > + *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
>> > + *   - OUT: %rax - highest usable memory address below 1 MiB;
>> > + * memory above this address is reserved for 
>> > trampoline;
>> > + * memory below this address is used for stack 
>> > and as
>> > + * a storage for boot data.
>>
>> How can you validly use memory below this address? (And I'd like to
> 
> Right, I should not do that blindly. As quick fix we can check in 
> efi_arch_process_memory_map()
> that chosen memory region has size cfg.size plus let's say 64 KiB. Is it 
> sufficient
> for you?

Depends. Part of my problem here is that you convert, in your answer,
my "validly" to "blindly". And then I'd like to see especially "storage for
boot data" better qualified: What exactly is it that you mean to store
there? I don't recall having noticed either this or that area being used
as stack anywhere in the series, so I'm afraid I've overlooked
something which may invalidate a good part of the review done.

Jan


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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper]

On Mon, Sep 26, 2016 at 09:12:40AM -0600, Jan Beulich wrote:
> >>> On 26.09.16 at 16:40,  wrote:
> > On 26/09/16 15:33, Jan Beulich wrote:
> > On 26.09.16 at 16:19,  wrote:
> >>> On 23/09/16 22:47, Daniel Kiper wrote:
>  +/*
>  + * Initialize BSS (no nasty surprises!).
>  + * It must be done earlier than in BIOS case
>  + * because efi_multiboot2() touches it.
>  + */
>  +lea .startof.(.bss)(%rip),%edi
>  +mov $.sizeof.(.bss),%ecx
> >>> Sorry, but you cannot use this syntax, for the same reasons why I won't
> >>> accept Jan's patch making similar changes elsewhere.
> >>>
> >>> Amongst other issues, you will break the Clang build with it.
> >> Did you verify meanwhile that this doesn't work with llvm?
> >
> > Yes.
> >
> > andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
> > #include 
> >
> > static unsigned int x;
> >
> > int main(void)
> > {
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> >  "mov $.sizeof.(.bss), %%rcx;"
> >  "mov $-1, %%rax;"
> >  "rep stosb;"
> >  ::: "memory", "rax", "rcx", "rdi");
> > printf("x: %#x\n", x);
> > }
> > andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
> > x: 0x
> > andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
> > foo.c:7:18: error: unexpected token in memory operand
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> >  ^
> > :1:16: note: instantiated into assembly here
> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> > $-1, %rax;rep stosb;
> >   ^
> > foo.c:7:18: error: unexpected token in argument list
> > asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
> >  ^
> > :1:47: note: instantiated into assembly here
> > lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> > $-1, %rax;rep stosb;
> >  ^
> > 2 errors generated.
>
> No, that's inline assembly, which does not match Daniel's use
> case. That's why I referred to llvm, as with assembly sources it
> should only be the linker which gets to see the symbols generated
> from those constructs (and if llvm supported them I'd then consider
> breaking out the assembly file changes from that patch of mine).

Guys, may I suggest using sym_phys(__bss_start)/sym_phys(__bss_end) as
it is in head.S right now? If .startof.()/.sizeof.() works as expected
and both of you accept it then later we can move to new approach. Does
it make sense?

Daniel

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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper]

On Mon, Sep 26, 2016 at 07:47:42AM -0600, Jan Beulich wrote:
> >>> On 23.09.16 at 23:47,  wrote:
> > This way Xen can be loaded on EFI platforms using GRUB2 and
> > other boot loaders which support multiboot2 protocol.
> >
> > Signed-off-by: Daniel Kiper 
> > Acked-by: Jan Beulich 
> > ---
> > v7 - suggestions/fixes:
> >- do not allocate twice memory for trampoline if we were
> >  loaded via multiboot2 protocol on EFI platform,
>
> If you fix bugs not noticed by a reviewer but by yourself, please
> drop all acks/R-b-s covering the code in question. And then I'm

OK.

> afraid I haven't even been able to locate that change - could you
> please point out what you did where?

The change is very subtle. I add trampoline_setup label behind

 /* From arch/x86/smpboot.c: start_eip had better be page-aligned! */
 xor %cl, %cl

instead of

 cmp %ecx,%edx   /* compare with BDA value */
 cmovb   %edx,%ecx   /* and use the smaller */

> > +/*
> > + * Initialize BSS (no nasty surprises!).
> > + * It must be done earlier than in BIOS case
> > + * because efi_multiboot2() touches it.
> > + */
> > +lea .startof.(.bss)(%rip),%edi
> > +mov $.sizeof.(.bss),%ecx
> > +shr $3,%ecx
> > +xor %eax,%eax
> > +rep stosq
> > +
> > +pop %rdi
> > +
> > +/*
> > + * efi_multiboot2() is called according to System V AMD64 ABI:
> > + *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> > + *   - OUT: %rax - highest usable memory address below 1 MiB;
> > + * memory above this address is reserved for 
> > trampoline;
> > + * memory below this address is used for stack and 
> > as
> > + * a storage for boot data.
>
> How can you validly use memory below this address? (And I'd like to

Right, I should not do that blindly. As quick fix we can check in 
efi_arch_process_memory_map()
that chosen memory region has size cfg.size plus let's say 64 KiB. Is it 
sufficient
for you? However, I think that later (for 4.9?) we should consider what we 
discussed
here https://lists.xen.org/archives/html/xen-devel/2016-09/msg01424.html

> note that this also changed from v6, and the change to comments
> listed in the v7 section and supposedly suggested by me can't cover
> this, as I don't recall having asked for such an adjustment.)

Ah, sorry about that. I should be more precise.

Daniel

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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 26.09.16 at 16:40,  wrote:
> On 26/09/16 15:33, Jan Beulich wrote:
> On 26.09.16 at 16:19,  wrote:
>>> On 23/09/16 22:47, Daniel Kiper wrote:
 +/*
 + * Initialize BSS (no nasty surprises!).
 + * It must be done earlier than in BIOS case
 + * because efi_multiboot2() touches it.
 + */
 +lea .startof.(.bss)(%rip),%edi
 +mov $.sizeof.(.bss),%ecx
>>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>>> accept Jan's patch making similar changes elsewhere.
>>>
>>> Amongst other issues, you will break the Clang build with it.
>> Did you verify meanwhile that this doesn't work with llvm?
> 
> Yes.
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
> #include 
> 
> static unsigned int x;
> 
> int main(void)
> {
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>  "mov $.sizeof.(.bss), %%rcx;"
>  "mov $-1, %%rax;"
>  "rep stosb;"
>  ::: "memory", "rax", "rcx", "rdi");
> printf("x: %#x\n", x);
> }
> andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
> x: 0x
> andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
> foo.c:7:18: error: unexpected token in memory operand
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>  ^
> :1:16: note: instantiated into assembly here
> lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> $-1, %rax;rep stosb;
>   ^
> foo.c:7:18: error: unexpected token in argument list
> asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
>  ^
> :1:47: note: instantiated into assembly here
> lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
> $-1, %rax;rep stosb;
>  ^
> 2 errors generated.

No, that's inline assembly, which does not match Daniel's use
case. That's why I referred to llvm, as with assembly sources it
should only be the linker which gets to see the symbols generated
from those constructs (and if llvm supported them I'd then consider
breaking out the assembly file changes from that patch of mine).

Jan


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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Andrew Cooper]

On 26/09/16 15:33, Jan Beulich wrote:
 On 26.09.16 at 16:19,  wrote:
>> On 23/09/16 22:47, Daniel Kiper wrote:
>>> +/*
>>> + * Initialize BSS (no nasty surprises!).
>>> + * It must be done earlier than in BIOS case
>>> + * because efi_multiboot2() touches it.
>>> + */
>>> +lea .startof.(.bss)(%rip),%edi
>>> +mov $.sizeof.(.bss),%ecx
>> Sorry, but you cannot use this syntax, for the same reasons why I won't
>> accept Jan's patch making similar changes elsewhere.
>>
>> Amongst other issues, you will break the Clang build with it.
> Did you verify meanwhile that this doesn't work with llvm?

Yes.

andrewcoop@andrewcoop:/local/xen.git/xen$ cat foo.c
#include 

static unsigned int x;

int main(void)
{
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
 "mov $.sizeof.(.bss), %%rcx;"
 "mov $-1, %%rax;"
 "rep stosb;"
 ::: "memory", "rax", "rcx", "rdi");
printf("x: %#x\n", x);
}
andrewcoop@andrewcoop:/local/xen.git/xen$ gcc foo.c -o foo && ./foo
x: 0x
andrewcoop@andrewcoop:/local/xen.git/xen$ clang-3.8 foo.c -o foo && ./foo
foo.c:7:18: error: unexpected token in memory operand
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
 ^
:1:16: note: instantiated into assembly here
lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
$-1, %rax;rep stosb;
  ^
foo.c:7:18: error: unexpected token in argument list
asm volatile("lea .startof.(.bss)(%%rip), %%rdi;"
 ^
:1:47: note: instantiated into assembly here
lea .startof.(.bss)(%rip), %rdi;mov $.sizeof.(.bss), %rcx;mov
$-1, %rax;rep stosb;
 ^
2 errors generated.

~Andrew

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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 26.09.16 at 16:19,  wrote:
> On 23/09/16 22:47, Daniel Kiper wrote:
>> +/*
>> + * Initialize BSS (no nasty surprises!).
>> + * It must be done earlier than in BIOS case
>> + * because efi_multiboot2() touches it.
>> + */
>> +lea .startof.(.bss)(%rip),%edi
>> +mov $.sizeof.(.bss),%ecx
> 
> Sorry, but you cannot use this syntax, for the same reasons why I won't
> accept Jan's patch making similar changes elsewhere.
> 
> Amongst other issues, you will break the Clang build with it.

Did you verify meanwhile that this doesn't work with llvm?

Jan


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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Andrew Cooper]

On 23/09/16 22:47, Daniel Kiper wrote:
> +/*
> + * Initialize BSS (no nasty surprises!).
> + * It must be done earlier than in BIOS case
> + * because efi_multiboot2() touches it.
> + */
> +lea .startof.(.bss)(%rip),%edi
> +mov $.sizeof.(.bss),%ecx

Sorry, but you cannot use this syntax, for the same reasons why I won't
accept Jan's patch making similar changes elsewhere.

Amongst other issues, you will break the Clang build with it.

~Andrew

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

Re: [Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 23.09.16 at 23:47,  wrote:
> This way Xen can be loaded on EFI platforms using GRUB2 and
> other boot loaders which support multiboot2 protocol.
> 
> Signed-off-by: Daniel Kiper 
> Acked-by: Jan Beulich 
> ---
> v7 - suggestions/fixes:
>- do not allocate twice memory for trampoline if we were
>  loaded via multiboot2 protocol on EFI platform,

If you fix bugs not noticed by a reviewer but by yourself, please
drop all acks/R-b-s covering the code in question. And then I'm
afraid I haven't even been able to locate that change - could you
please point out what you did where?

> +/*
> + * Initialize BSS (no nasty surprises!).
> + * It must be done earlier than in BIOS case
> + * because efi_multiboot2() touches it.
> + */
> +lea .startof.(.bss)(%rip),%edi
> +mov $.sizeof.(.bss),%ecx
> +shr $3,%ecx
> +xor %eax,%eax
> +rep stosq
> +
> +pop %rdi
> +
> +/*
> + * efi_multiboot2() is called according to System V AMD64 ABI:
> + *   - IN: %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
> + *   - OUT: %rax - highest usable memory address below 1 MiB;
> + * memory above this address is reserved for 
> trampoline;
> + * memory below this address is used for stack and as
> + * a storage for boot data.

How can you validly use memory below this address? (And I'd like to
note that this also changed from v6, and the change to comments
listed in the v7 section and supposedly suggested by me can't cover
this, as I don't recall having asked for such an adjustment.)

Jan


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

[Xen-devel] [PATCH v7 08/14] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper]

This way Xen can be loaded on EFI platforms using GRUB2 and
other boot loaders which support multiboot2 protocol.

Signed-off-by: Daniel Kiper 
Acked-by: Jan Beulich 
---
v7 - suggestions/fixes:
   - do not allocate twice memory for trampoline if we were
 loaded via multiboot2 protocol on EFI platform,
   - wrap long line
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich).

v6 - suggestions/fixes:
   - improve label names in assembly
 error printing code
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - various minor cleanups and fixes
 (suggested by Jan Beulich).

v4 - suggestions/fixes:
   - remove redundant BSS alignment,
   - update BSS alignment check,
   - use __set_bit() instead of set_bit() if possible
 (suggested by Jan Beulich),
   - call efi_arch_cpu() from efi_multiboot2()
 even if the same work is done later in
 other place right now
 (suggested by Jan Beulich),
   - xen/arch/x86/efi/stub.c:efi_multiboot2()
 fail properly on EFI platforms,
   - do not read data beyond the end of multiboot2
 information in xen/arch/x86/boot/head.S
 (suggested by Jan Beulich),
   - use 32-bit registers in x86_64 code if possible
 (suggested by Jan Beulich),
   - multiboot2 information address is 64-bit
 in x86_64 code, so, treat it is as is
 (suggested by Jan Beulich),
   - use cmovcc if possible,
   - leave only one space between rep and stosq
 (suggested by Jan Beulich),
   - improve error handling,
   - improve early error messages,
 (suggested by Jan Beulich),
   - improve early error messages printing code,
   - improve label names
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - various minor cleanups.

v3 - suggestions/fixes:
   - take into account alignment when skipping multiboot2 fixed part
 (suggested by Konrad Rzeszutek Wilk),
   - improve segment registers initialization
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich and Konrad Rzeszutek Wilk),
   - improve commit message
 (suggested by Jan Beulich).

v2 - suggestions/fixes:
   - generate multiboot2 header using macros
 (suggested by Jan Beulich),
   - switch CPU to x86_32 mode before
 jumping to 32-bit code
 (suggested by Andrew Cooper),
   - reduce code changes to increase patch readability
 (suggested by Jan Beulich),
   - improve comments
 (suggested by Jan Beulich),
   - ignore MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO tag on EFI platform
 and find on my own multiboot2.mem_lower value,
   - stop execution if EFI platform is detected
 in legacy BIOS path.
---
 xen/arch/x86/boot/head.S  |  258 ++---
 xen/arch/x86/efi/efi-boot.h   |   49 ++-
 xen/arch/x86/efi/stub.c   |   38 ++
 xen/arch/x86/x86_64/asm-offsets.c |2 +
 xen/arch/x86/xen.lds.S|4 +-
 xen/common/efi/boot.c |   11 ++
 6 files changed, 340 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 383ff79..a371cde 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -89,6 +89,13 @@ multiboot2_header_start:
0, /* Number of the lines - no preference. */ \
0  /* Number of bits per pixel - no preference. */
 
+/* Inhibit bootloader from calling ExitBootServices(). */
+mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL)
+
+/* EFI64 entry point. */
+mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \
+   sym_phys(__efi64_start)
+
 /* Multiboot2 header end tag. */
 mb2ht_init MB2_HT(END), MB2_HT(REQUIRED)
 .Lmultiboot2_header_end:
@@ -100,20 +107,49 @@ multiboot2_header_start:
 gdt_boot_descr:
 .word   6*8-1
 .long   sym_phys(trampoline_gdt)
+.long   0 /* Needed for 64-bit lgdt */
+
+cs32_switch_addr:
+.long   sym_phys(cs32_switch)
+.word   BOOT_CS32
+
+.align 4
+vga_text_buffer:
+.long   0xb8000
 
 .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
 .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
+.Lbad_ldr_nbs: .asciz "ERR: Bootloader shutdown EFI x64 boot services!"
+.Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
+.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.text, "ax", @progbits
 
 bad_cpu:
 mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
-jmp print_err
+jmp .Lget_vtb
 not_multiboot:
 mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
-print_err:
-mov $0xB8000,%edi  # VGA framebuffer
-1:  mov (%esi),%bl
+jmp .Lget_vtb
+.Lmb2_no_st:
+mov