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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.06.16 at 21:03, <daniel.kiper@xxxxxxxxxx> wrote:
> 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:
>> >> 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.

I'm not sure about that, keeping various Atoms in mind.

> 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 may at least assume accessing VGA memory won't do
much bad.

> Hence, we are not able to get anything from EFI. Latter
> means that we were booted via older multiboot2 version which shutdown
> boot services.

Hmm, that's certainly one of the possibilities, yes. I wonder then
whether we wouldn't better do away with all of those output
attempts then.

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

I disagree, especially to the "much".

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

A 64-bit interface shouldn't specify values to live only in halves
of registers, in my opinion. Remember that the architecture
guarantees high halves to get zeroed for 32-bit writes to
registers, so I don't even see any complication for the provider
side of the interface (which necessarily runs in 64-bit mode).

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

May I refer you back to you, in another patch, adding just two
architecture defines for MB2: i386 and MIPS? That's a pretty
clear indication that there can't be much consistency to be
expected when talk comes to x86-64 (which most definitely is
not i386).

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

How are boot services coming into the picture here? As the code
comment still visible above says, we're on the legacy boot path
here. And legacy boot, from all I know, implies a text mode on
the primary VGA (if any).

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

Oh, I see - a new-style xen.gz which doesn't have EFI support
enabled due to tool chain issues but gets booted from EFI/grub2.

Jan

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