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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

>> 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?

>> > +        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?

>> > +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.

>> > +        /*
>> > +         * 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".

> 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

>> > +        /* 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.

>> > +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?

>> > @@ -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.

>> > +    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.)

>> > --- 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 ...

> 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.

Jan

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