Re: [Xen-devel] [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Fri, May 27, 2016 at 03:02:25AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 23:02, <daniel.kiper@xxxxxxxxxx> wrote:
> > On Wed, May 25, 2016 at 03:32:37AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33, <daniel.kiper@xxxxxxxxxx> wrote:
> >> >  bad_cpu:
> >> >          mov     $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> >> > -        jmp     print_err
> >> > +        mov     $0xB8000,%edi                   # VGA framebuffer
> >> > +        jmp     1f
> >> >  not_multiboot:
> >> >          mov     $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> >> > -print_err:
> >> > -        mov     $0xB8000,%edi  # VGA framebuffer
> >> > +        mov     $0xB8000,%edi                   # VGA framebuffer
> >> > +        jmp     1f
> >> > +mb2_too_old:
> >> > +        mov     $(sym_phys(.Lbad_ldr_mb2)),%esi # Error message
> >> > +        xor     %edi,%edi                       # No VGA framebuffer
> >>
> >> Leaving aside that "framebuffer" really is a bad term here (we're
> >> talking of text mode output after all), limiting the output to serial
> >
> > Yep, but then we should change this in other places too. Maybe in separate
> > patch.
>
> Since you touch (move) it, replacing the word "framebuffer" wouldn't
> seem like something making the patch any more difficult to understand.
>
> >> isn't going to be very helpful in the field, I'm afraid. Even more so
> >> that there's no guarantee for a UART to be at port 0x3f8. That's
> >
> > Right but we do not have big choice here at very early boot stage... :-(((
>
> Excuse me? You're running on EFI, so you have full infrastructure
> available to you. As much as in a normal BIOS scenario (and on a
> half way normal system) we can assume a text mode screen with
> video memory mapped at B8000, under EFI we can assume output
> capabilities (whichever the system owner set up in the firmware
> setup).

Potentially we can do that for bad_cpu only. However, does it pays?
I suppose that most, if not all, platforms with UEFI have CPUs with
X86_FEATURE_LM.

Sadly, It it not feasible for not_multiboot and mb2_too_old. Former
means that we were loaded by unknown boot loader and interface is
not defined. Hence, we are not able to get anything from EFI. Latter
means that we were booted via older multiboot2 version which shutdown
boot services.

> >> not much of a problem for the other two messages as people are
> >> unlikely to try to boot Xen on an unsuitable system, but I view it
> >> as quite possible for Xen to be tried to get booted with an
> >> unsuitable grub2.
> >>
> >> IOW - this needs a better solution, presumably using EFI boot
> >> service output functions.
> >
> > No way, here boot services are dead. GRUB2 (or other loader)
> > shutdown them... :-(((
>
> Wasn't one of your grub2 changes being precisely the deferral of
> that to Xen?

Sure, but if we are here then it means that we were loaded by earlier multiboot2
protocol version which does not understand features added by me.

> >> > +        cld
> >> > +
> >> > +        /* Check for Multiboot2 bootloader. */
> >> > +        cmp     $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> >> > +        je      efi_multiboot2_proto
> >> > +
> >> > +        /* Jump to not_multiboot after switching CPU to x86_32 mode. */
> >> > +        lea     not_multiboot(%rip),%rdi
> >> > +        jmp     x86_32_switch
> >>
> >> What I've said above would also eliminate the need to switch to
> >> 32-bit mode just for emitting an error message and halting the
> >> system.
> >
> > It is not possible. We just know that we run on EFI platform here.
> > However, we are not able to get EFI SystemTable pointer.
>
> How are we not able? Later C code does it afair, so why would it not
> be possible here?

Ditto.

> >> > +efi_multiboot2_proto:
> >>
> >> .Lefi_multiboot2_proto
> >
> > OK if you insist. However, I think that we are loosing helpful
> > debug information this way.
>
> I don't see why or how. Labels persisting in the final symbol table
> are useful only for generating stack traces, yet if you crash this
> early there won't be any stack trace anyway - you don't even
> have an IDT set up yet.

OK, but it is much easier to identify addresses for breakpoints if you
have proper labels. And this one looks quite useful.

> >> > +        /*
> >> > +         * Multiboot2 information address is 32-bit,
> >> > +         * so, zero higher half of %rbx.
> >> > +         */
> >> > +        mov     %ebx,%ebx
> >>
> >> Wait, no - that's a protocol bug then. We're being entered in 64-bit
> >> mode here, so registers should be in 64-bit clean state.
> >
> > You mean higher half cleared. Right? This is not guaranteed.
>
> Hence me saying "that's a protocol bug then".

Why? Protocol strictly says that "this is not guaranteed".
What is the problem with that? Potentially loader can set
%rbx higher half to e.g. 0 but I do not think it is needed.

> > Please check this:
> > http://lists.gnu.org/archive/html/grub-devel/2016-03/msg00304.html
>
> Other than the description of the patch I can't see anything like that,
> in particular
> - no occurrence of "ebx" in any of the added or changed code
> - MULTIBOOT_EFI_MBI_REGISTER getting #define-d as rbx

Please check multiboot2 spec, section 3.2, Machine state. It is not
freshest one (I am working on EFI updates right now) but I hope that it,
together with patch comment, will shed some light.

> >> > +        /* Skip Multiboot2 information fixed part. */
> >> > +        lea     (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%rcx
> >> > +        and     $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
> >>
> >> Or if there really was a reason to do the above, and if there is a
> >> reason not to assume this data is located below 4Gb, then
> >> calculations like this could avoid the REX64 prefix by using %ecx.
> >
> > Here you said something different:
> >   http://lists.xen.org/archives/html/xen-devel/2015-03/msg03557.html
> >
> > So?
>
> So? You simply went too far: There talk was of memory accesses,
> i.e. the (%rbx) part of the instruction above. Using (%ebx) there
> would cause a pointless address size override prefix emitted. Now
> in the new form above you force a pointless REX prefix to be
> emitted. The whole point of both of my requests is - avoid prefixes
> if you don't really need them.

Wilco! Thanks for explanation.

> >> > +0:
> >> > +        /* Get EFI SystemTable address from Multiboot2 information. */
> >> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> >> > +        jne     1f
> >> > +
> >> > +        mov     MB2_efi64_st(%rcx),%rsi
> >> > +
> >> > +        /* Do not clear BSS twice and do not go into real mode. */
> >> > +        movb    $1,skip_realmode(%rip)
> >>
> >> How is the setting of skip_realmode related to the clearing of BSS?
> >> Oh, I've found the connection below (albeit see there for its
> >> validity), but I think mentioning this here is more confusing than
> >> clarifying.
> >
> > In general I have a problem skip_realmode. The name itself is confusing
> > here and there. Probably it should be changed. However, I do not have
> > idea for good compact name describing all skip_realmode usages. And I do
> > not think that we should add another variable which will be used in similar
> > way but with different name to clarify situation.
>
> Just clarify things by suitable (brief) comments?

If it is acceptable by you guys then OK.

> >> > @@ -170,12 +313,19 @@ multiboot2_proto:
> >> >          jne     1f
> >> >
> >> >          mov     MB2_mem_lower(%ecx),%edx
> >> > -        jmp     trampoline_setup
> >> > +        jmp     trampoline_bios_setup
> >> >
> >> >  1:
> >> > +        /* EFI mode is not supported via legacy BIOS path. */
> >> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI32,MB2_tag_type(%ecx)
> >> > +        je      mb2_too_old
> >> > +
> >> > +        cmpl    $MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%ecx)
> >> > +        je      mb2_too_old
> >>
> >> According to the comment we're on the legacy BIOS boot path
> >> here, yet at mb2_too_old you assume no text mode VGA. I.e. I'm
> >> now even confused about the output handling there.
> >
> > It is correct. Old GRUB2 (or other legacy multiboot2 compatible boot
> > loader) which runs on EFI platform and does not have my features will
> > jump into that path. And we do not support that, so, we should fail
> > in the best possible way here.
> >
> > Your comment suggest that code comment should be improved and
> > phrased precisely. I will do that.
>
> Not necessarily: First of all you didn't clarify what video mode we're
> in in that old-grub2 case. Do we have text mode? If so, output should
> not be avoided. And if we're in a graphical mode without any vga=
> option that grub2 may have chosen to interpret, that would smell like
> a bug in grub2.

Here boot services are dead. They were shutdown by GRUB2 (or other
legacy boot loader). So, we do not have simple access to GOP or
anything like that. Am I missing something?

> >> > +    efi_tables();
> >> > +    setup_efi_pci();
> >> > +    efi_variables();
> >> > +
> >> > +    if ( gop )
> >> > +        efi_set_gop_mode(gop, gop_mode);
> >> > +
> >> > +    efi_exit_boot(ImageHandle, SystemTable);
>
> (Btw, regarding your earlier statement of boot services being
> unavailable in the early assembly entry code: Here is the
> exit-boot-services place, i.e. up to here boot services can be used,
> which therefore includes the assembly part of the boot path.)

This is reachable if everything went OK. If it does not very early
then UEFI stuff is unavailable. Please look above for more details.

> >> > --- a/xen/arch/x86/efi/stub.c
> >> > +++ b/xen/arch/x86/efi/stub.c
> >> > @@ -13,6 +13,11 @@ struct efi __read_mostly efi = {
> >> >          .smbios3 = EFI_INVALID_TABLE_ADDR
> >> >  };
> >> >
> >> > +void __init efi_multiboot2(void)
> >> > +{
> >> > +    /* TODO: Fail if entered! */
> >> > +}
> >>
> >> Why not just BUG()? What exactly you do here doesn't seem to
> >> matter, as the symbol is unreachable in this case anyway (you
> >> only need it to please the linker).
> >
> > We should print meaningful message here using boot services.
>
> Which boot services? We're not running on EFI if we get here. And
> as said, this function is unreachable on non-EFI afaict, so ...

It is. Assembly code in head.S is build unconditionally.

> > To do that we need a few line of assembly probably. And BUG()
> > is not solution here.
>
> ... I don't follow you here.
>
> >> > --- a/xen/arch/x86/xen.lds.S
> >> > +++ b/xen/arch/x86/xen.lds.S
> >> > @@ -192,7 +192,7 @@ SECTIONS
> >> >    } :text
> >> >
> >> >    /* Align BSS to speedup its initialization. */
> >> > -  . = ALIGN(4);
> >> > +  . = ALIGN(8);
> >> >    .bss : {                     /* BSS */
> >> >         . = ALIGN(STACK_SIZE);
> >> >         __bss_start = .;
> >> > @@ -207,7 +207,7 @@ SECTIONS
> >> >         *(.bss.percpu.read_mostly)
> >> >         . = ALIGN(SMP_CACHE_BYTES);
> >> >         __per_cpu_data_end = .;
> >> > -       . = ALIGN(4);
> >> > +       . = ALIGN(8);
> >> >         __bss_end = .;
> >>
> >> Is that really worth it? I.e. is going from STOSD to STOSQ really a
> >> meaningful win?
> >
> > Probably yes but I do not think that anybody will be see boot time
> > difference. On the other hand why not do that? It does not cost a lot.
>
> But it grows an already largish patch.

If Andrew do not object I can leave/use stosd/stosl as is.

Daniel

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