| Message ID | 20170214183811.GC10876@olila.local.net-space.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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
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?
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
>>> 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
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();