[v15,05/10] x86: add multiboot2 protocol support for EFI platforms
diff mbox

Message ID 20170214183811.GC10876@olila.local.net-space.pl
State New, archived
Headers show

Commit Message

Daniel Kiper Feb. 14, 2017, 6:38 p.m. UTC
On Tue, Feb 14, 2017 at 07:33:24PM +0100, Daniel Kiper 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 <daniel.kiper@oracle.com>

I am posting diff between v14 and v15 for your convenience.

Comments

Jan Beulich Feb. 15, 2017, 10:22 a.m. UTC | #1
>>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -394,10 +394,18 @@ __start:
> 
>          /* EFI IA-32 platforms are not supported. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> +        /*
> +         * Here we should zap vga_text_buffer. However, we can disable
> +         * VGA updates in simpler and more reliable way later.
> +         */
>          je      .Lmb2_efi_ia_32
> 
>          /* Bootloader shutdown EFI x64 boot services. */
>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> +        /*
> +         * Here we should zap vga_text_buffer. However, we can disable
> +         * VGA updates in simpler and more reliable way later.
> +         */
>          je      .Lmb2_no_bs

I'm afraid I don't view these comments as helpful in understanding
the whole situation. That's partly because I don't follow both the
"simpler" and "more reliable" parts (using just the information here,
i.e. leaving aside what you've given as explanation earlier, albeit I
don't think that was fully clarifying things either), and partly
because I continue to think that the explanation should go where
the labels are (which is what I had meant to suggest with my
comment placement in reply to v14). Nor does the adjustment
above help (me) understand the correctness of the dual use of
.Lmb2_no_bs.

Jan
Daniel Kiper Feb. 15, 2017, 9:53 p.m. UTC | #2
On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
> >>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -394,10 +394,18 @@ __start:
> >
> >          /* EFI IA-32 platforms are not supported. */
> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> > +        /*
> > +         * Here we should zap vga_text_buffer. However, we can disable
> > +         * VGA updates in simpler and more reliable way later.
> > +         */
> >          je      .Lmb2_efi_ia_32
> >
> >          /* Bootloader shutdown EFI x64 boot services. */
> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> > +        /*
> > +         * Here we should zap vga_text_buffer. However, we can disable
> > +         * VGA updates in simpler and more reliable way later.
> > +         */
> >          je      .Lmb2_no_bs
>
> I'm afraid I don't view these comments as helpful in understanding
> the whole situation. That's partly because I don't follow both the
> "simpler" and "more reliable" parts (using just the information here,

OK, I will clarify it.

> i.e. leaving aside what you've given as explanation earlier, albeit I
> don't think that was fully clarifying things either), and partly
> because I continue to think that the explanation should go where
> the labels are (which is what I had meant to suggest with my
> comment placement in reply to v14). Nor does the adjustment

OK.

> above help (me) understand the correctness of the dual use of
> .Lmb2_no_bs.

What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.

By the way, what about second half of the patch series? Does it make sense or
a little or is it total crap? Could you say something? You have not commented
it for a few releases, so, there is a chance that it is OK. However, opposite
is also possible. So, if you wish to fix something please say it now than later.
Then I will have more time to do the work.

Daniel
Jan Beulich Feb. 16, 2017, 9:29 a.m. UTC | #3
>>> On 15.02.17 at 22:53, <daniel.kiper@oracle.com> wrote:
> On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
>> >>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
>> > --- a/xen/arch/x86/boot/head.S
>> > +++ b/xen/arch/x86/boot/head.S
>> > @@ -394,10 +394,18 @@ __start:
>> >
>> >          /* EFI IA-32 platforms are not supported. */
>> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
>> > +        /*
>> > +         * Here we should zap vga_text_buffer. However, we can disable
>> > +         * VGA updates in simpler and more reliable way later.
>> > +         */
>> >          je      .Lmb2_efi_ia_32
>> >
>> >          /* Bootloader shutdown EFI x64 boot services. */
>> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
>> > +        /*
>> > +         * Here we should zap vga_text_buffer. However, we can disable
>> > +         * VGA updates in simpler and more reliable way later.
>> > +         */
>> >          je      .Lmb2_no_bs
>>
>> I'm afraid I don't view these comments as helpful in understanding
>> the whole situation. That's partly because I don't follow both the
>> "simpler" and "more reliable" parts (using just the information here,
> 
> OK, I will clarify it.
> 
>> i.e. leaving aside what you've given as explanation earlier, albeit I
>> don't think that was fully clarifying things either), and partly
>> because I continue to think that the explanation should go where
>> the labels are (which is what I had meant to suggest with my
>> comment placement in reply to v14). Nor does the adjustment
> 
> OK.
> 
>> above help (me) understand the correctness of the dual use of
>> .Lmb2_no_bs.
> 
> What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.

As said in v14 review, it's being jumped to from two rather different
places, and hence the VGA aspect isn't obviously the same for both.

Jan
Daniel Kiper Feb. 16, 2017, 9:49 p.m. UTC | #4
On Thu, Feb 16, 2017 at 02:29:45AM -0700, Jan Beulich wrote:
> >>> On 15.02.17 at 22:53, <daniel.kiper@oracle.com> wrote:
> > On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
> >> >>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
> >> > --- a/xen/arch/x86/boot/head.S
> >> > +++ b/xen/arch/x86/boot/head.S
> >> > @@ -394,10 +394,18 @@ __start:
> >> >
> >> >          /* EFI IA-32 platforms are not supported. */
> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> >> > +        /*
> >> > +         * Here we should zap vga_text_buffer. However, we can disable
> >> > +         * VGA updates in simpler and more reliable way later.
> >> > +         */
> >> >          je      .Lmb2_efi_ia_32
> >> >
> >> >          /* Bootloader shutdown EFI x64 boot services. */
> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> >> > +        /*
> >> > +         * Here we should zap vga_text_buffer. However, we can disable
> >> > +         * VGA updates in simpler and more reliable way later.
> >> > +         */
> >> >          je      .Lmb2_no_bs
> >>
> >> I'm afraid I don't view these comments as helpful in understanding
> >> the whole situation. That's partly because I don't follow both the
> >> "simpler" and "more reliable" parts (using just the information here,
> >
> > OK, I will clarify it.
> >
> >> i.e. leaving aside what you've given as explanation earlier, albeit I
> >> don't think that was fully clarifying things either), and partly
> >> because I continue to think that the explanation should go where
> >> the labels are (which is what I had meant to suggest with my
> >> comment placement in reply to v14). Nor does the adjustment
> >
> > OK.
> >
> >> above help (me) understand the correctness of the dual use of
> >> .Lmb2_no_bs.
> >
> > What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.
>
> As said in v14 review, it's being jumped to from two rather different
> places, and hence the VGA aspect isn't obviously the same for both.

OK, I will try to clarify. If a bootloader called us using __efi64_mb2_start
we are sure that we are running on EFI platform and there is no VGA there.
It means that we can safely zap vga_text_buffer unconditionally in first steps
(we do that in second instruction). Then we do not need to take care about
that in case of error. And one of these errors is lack of MULTIBOOT2_TAG_TYPE_EFI_BS
tag. It means that EFI boot services are shutdown. So, we are in black hole.
We have to inform user about that and halt the system. And that is why we
jump to .Lmb2_no_bs here.

On the other hand if the bootloader called us using start label then in most
cases we are running on legacy BIOS platforms. However, if the bootloader also
provided MULTIBOOT2_TAG_TYPE_EFI64 tag here then we are sure that we are running
on EFI platform and EFI boot services are shutdown. This happens when we are
loaded by old boot loader which does not understand MULTIBOOT2_HEADER_TAG_EFI_BS
and MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 tags. So, as above we can jump
to .Lmb2_no_bs here too.

I hope that helps.

Daniel
Doug Goldstein Feb. 16, 2017, 9:56 p.m. UTC | #5
On 2/16/17 3:49 PM, Daniel Kiper wrote:
> On Thu, Feb 16, 2017 at 02:29:45AM -0700, Jan Beulich wrote:
>>>>> On 15.02.17 at 22:53, <daniel.kiper@oracle.com> wrote:
>>> On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
>>>>>>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
>>>>> --- a/xen/arch/x86/boot/head.S
>>>>> +++ b/xen/arch/x86/boot/head.S
>>>>> @@ -394,10 +394,18 @@ __start:
>>>>>
>>>>>          /* EFI IA-32 platforms are not supported. */
>>>>>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
>>>>> +        /*
>>>>> +         * Here we should zap vga_text_buffer. However, we can disable
>>>>> +         * VGA updates in simpler and more reliable way later.
>>>>> +         */
>>>>>          je      .Lmb2_efi_ia_32
>>>>>
>>>>>          /* Bootloader shutdown EFI x64 boot services. */
>>>>>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
>>>>> +        /*
>>>>> +         * Here we should zap vga_text_buffer. However, we can disable
>>>>> +         * VGA updates in simpler and more reliable way later.
>>>>> +         */
>>>>>          je      .Lmb2_no_bs
>>>>
>>>> I'm afraid I don't view these comments as helpful in understanding
>>>> the whole situation. That's partly because I don't follow both the
>>>> "simpler" and "more reliable" parts (using just the information here,
>>>
>>> OK, I will clarify it.
>>>
>>>> i.e. leaving aside what you've given as explanation earlier, albeit I
>>>> don't think that was fully clarifying things either), and partly
>>>> because I continue to think that the explanation should go where
>>>> the labels are (which is what I had meant to suggest with my
>>>> comment placement in reply to v14). Nor does the adjustment
>>>
>>> OK.
>>>
>>>> above help (me) understand the correctness of the dual use of
>>>> .Lmb2_no_bs.
>>>
>>> What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.
>>
>> As said in v14 review, it's being jumped to from two rather different
>> places, and hence the VGA aspect isn't obviously the same for both.
> 
> OK, I will try to clarify. If a bootloader called us using __efi64_mb2_start
> we are sure that we are running on EFI platform and there is no VGA there.
> It means that we can safely zap vga_text_buffer unconditionally in first steps
> (we do that in second instruction). Then we do not need to take care about
> that in case of error. And one of these errors is lack of MULTIBOOT2_TAG_TYPE_EFI_BS
> tag. It means that EFI boot services are shutdown. So, we are in black hole.
> We have to inform user about that and halt the system. And that is why we

Not looking at the code but the words here. If ExitBootServices() has
been called we should be able to still boot if the memory map was passed
along. Are we deferring that use case to a follow on?
Daniel Kiper Feb. 16, 2017, 10:19 p.m. UTC | #6
On Thu, Feb 16, 2017 at 03:56:21PM -0600, Doug Goldstein wrote:
> On 2/16/17 3:49 PM, Daniel Kiper wrote:
> > On Thu, Feb 16, 2017 at 02:29:45AM -0700, Jan Beulich wrote:
> >>>>> On 15.02.17 at 22:53, <daniel.kiper@oracle.com> wrote:
> >>> On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
> >>>>>>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
> >>>>> --- a/xen/arch/x86/boot/head.S
> >>>>> +++ b/xen/arch/x86/boot/head.S
> >>>>> @@ -394,10 +394,18 @@ __start:
> >>>>>
> >>>>>          /* EFI IA-32 platforms are not supported. */
> >>>>>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> >>>>> +        /*
> >>>>> +         * Here we should zap vga_text_buffer. However, we can disable
> >>>>> +         * VGA updates in simpler and more reliable way later.
> >>>>> +         */
> >>>>>          je      .Lmb2_efi_ia_32
> >>>>>
> >>>>>          /* Bootloader shutdown EFI x64 boot services. */
> >>>>>          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> >>>>> +        /*
> >>>>> +         * Here we should zap vga_text_buffer. However, we can disable
> >>>>> +         * VGA updates in simpler and more reliable way later.
> >>>>> +         */
> >>>>>          je      .Lmb2_no_bs
> >>>>
> >>>> I'm afraid I don't view these comments as helpful in understanding
> >>>> the whole situation. That's partly because I don't follow both the
> >>>> "simpler" and "more reliable" parts (using just the information here,
> >>>
> >>> OK, I will clarify it.
> >>>
> >>>> i.e. leaving aside what you've given as explanation earlier, albeit I
> >>>> don't think that was fully clarifying things either), and partly
> >>>> because I continue to think that the explanation should go where
> >>>> the labels are (which is what I had meant to suggest with my
> >>>> comment placement in reply to v14). Nor does the adjustment
> >>>
> >>> OK.
> >>>
> >>>> above help (me) understand the correctness of the dual use of
> >>>> .Lmb2_no_bs.
> >>>
> >>> What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.
> >>
> >> As said in v14 review, it's being jumped to from two rather different
> >> places, and hence the VGA aspect isn't obviously the same for both.
> >
> > OK, I will try to clarify. If a bootloader called us using __efi64_mb2_start
> > we are sure that we are running on EFI platform and there is no VGA there.
> > It means that we can safely zap vga_text_buffer unconditionally in first steps
> > (we do that in second instruction). Then we do not need to take care about
> > that in case of error. And one of these errors is lack of MULTIBOOT2_TAG_TYPE_EFI_BS
> > tag. It means that EFI boot services are shutdown. So, we are in black hole.
> > We have to inform user about that and halt the system. And that is why we
>
> Not looking at the code but the words here. If ExitBootServices() has
> been called we should be able to still boot if the memory map was passed
> along. Are we deferring that use case to a follow on?

In theory yes but, IIRC, we have to significantly refactor Xen EFI boot code then
due to lack of EFI boot services. I tried to do that once but quickly realized
that it does not pays.

Daniel
Jan Beulich Feb. 17, 2017, 7:58 a.m. UTC | #7
>>> On 16.02.17 at 22:49, <daniel.kiper@oracle.com> wrote:
> On Thu, Feb 16, 2017 at 02:29:45AM -0700, Jan Beulich wrote:
>> >>> On 15.02.17 at 22:53, <daniel.kiper@oracle.com> wrote:
>> > On Wed, Feb 15, 2017 at 03:22:02AM -0700, Jan Beulich wrote:
>> >> >>> On 14.02.17 at 19:38, <daniel.kiper@oracle.com> wrote:
>> >> > --- a/xen/arch/x86/boot/head.S
>> >> > +++ b/xen/arch/x86/boot/head.S
>> >> > @@ -394,10 +394,18 @@ __start:
>> >> >
>> >> >          /* EFI IA-32 platforms are not supported. */
>> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
>> >> > +        /*
>> >> > +         * Here we should zap vga_text_buffer. However, we can disable
>> >> > +         * VGA updates in simpler and more reliable way later.
>> >> > +         */
>> >> >          je      .Lmb2_efi_ia_32
>> >> >
>> >> >          /* Bootloader shutdown EFI x64 boot services. */
>> >> >          cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
>> >> > +        /*
>> >> > +         * Here we should zap vga_text_buffer. However, we can disable
>> >> > +         * VGA updates in simpler and more reliable way later.
>> >> > +         */
>> >> >          je      .Lmb2_no_bs
>> >>
>> >> I'm afraid I don't view these comments as helpful in understanding
>> >> the whole situation. That's partly because I don't follow both the
>> >> "simpler" and "more reliable" parts (using just the information here,
>> >
>> > OK, I will clarify it.
>> >
>> >> i.e. leaving aside what you've given as explanation earlier, albeit I
>> >> don't think that was fully clarifying things either), and partly
>> >> because I continue to think that the explanation should go where
>> >> the labels are (which is what I had meant to suggest with my
>> >> comment placement in reply to v14). Nor does the adjustment
>> >
>> > OK.
>> >
>> >> above help (me) understand the correctness of the dual use of
>> >> .Lmb2_no_bs.
>> >
>> > What do you mean by "dual use of .Lmb2_no_bs."? I would like to be sure.
>>
>> As said in v14 review, it's being jumped to from two rather different
>> places, and hence the VGA aspect isn't obviously the same for both.
> 
> OK, I will try to clarify. If a bootloader called us using __efi64_mb2_start
> we are sure that we are running on EFI platform and there is no VGA there.
> It means that we can safely zap vga_text_buffer unconditionally in first steps
> (we do that in second instruction). Then we do not need to take care about
> that in case of error. And one of these errors is lack of MULTIBOOT2_TAG_TYPE_EFI_BS
> tag. It means that EFI boot services are shutdown. So, we are in black hole.

Well, see - this is one of my problems with the overall approach here.
Running on EFI in no way means there _is_ no VGA there, it only
means there _may not be_ any VGA. With boot services shut down
and without serial console you have no way of informing the user, so
making an attempt at writing something to VGA may still be helpful.
In the worst case the memory writes go no-where, to RAM, or to an
unrelated device (both of the latter rather unlikely on x86). Granted
there's the second problem of you perhaps not knowing the video
mode, and hence having a hard time producing output that's also
readable.

> We have to inform user about that and halt the system. And that is why we
> jump to .Lmb2_no_bs here.
> 
> On the other hand if the bootloader called us using start label then in most
> cases we are running on legacy BIOS platforms. However, if the bootloader also
> provided MULTIBOOT2_TAG_TYPE_EFI64 tag here then we are sure that we are running
> on EFI platform and EFI boot services are shutdown. This happens when we are
> loaded by old boot loader which does not understand MULTIBOOT2_HEADER_TAG_EFI_BS
> and MULTIBOOT2_HEADER_TAG_ENTRY_ADDRESS_EFI64 tags. So, as above we can jump
> to .Lmb2_no_bs here too.

However, when the boot loader invoked our start label, can't we be
sure there is VGA (at least as much or as little as there would be on
non-EFI platforms, where headless systems certainly also exist)? I
don't think the boot loader can reasonably invoke our legacy entry
point with the system in a state that's not legacy compatible (which,
among other things, means if there is VGA, then it would be in
traditional 80x25 text mode). Hence this second use of the label
ought to avoid suppressing the VGA output attempt in any case.

> I hope that helps.

The above aside, yes, it does. An abbreviated variant of this is
what I would hope to have attached as comment to the error
handling labels, to namely help readers understand why some of
them inhibit VGA output while others don't.

Jan

Patch
diff mbox

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 5147204..eb738d4 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -394,10 +394,18 @@  __start:

         /* EFI IA-32 platforms are not supported. */
         cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
+        /*
+         * Here we should zap vga_text_buffer. However, we can disable
+         * VGA updates in simpler and more reliable way later.
+         */
         je      .Lmb2_efi_ia_32

         /* Bootloader shutdown EFI x64 boot services. */
         cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
+        /*
+         * Here we should zap vga_text_buffer. However, we can disable
+         * VGA updates in simpler and more reliable way later.
+         */
         je      .Lmb2_no_bs

         /* Is it the end of Multiboot2 information? */
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 2127cce..17da050 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -34,10 +34,10 @@  void __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
      * not be directly supported by C compiler.
      */
     asm volatile(
-    "    call *%2                     \n"
+    "    call *%3                     \n"
     "0:  hlt                          \n"
     "    jmp  0b                      \n"
-       : "+c" (StdErr) : "d" (err), "rm" (StdErr->OutputString)
+       : "+c" (StdErr), "=d" (StdErr) : "1" (err), "rm" (StdErr->OutputString)
        : "rax", "r8", "r9", "r10", "r11", "memory");

     unreachable();