Re: [Xen-devel] [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.15 at 17:21, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <daniel.kiper@xxxxxxxxxx> wrote:
>> > @@ -130,6 +146,119 @@ print_err:
>> >  .Lhalt: hlt
>> >          jmp     .Lhalt
>> >
>> > +        .code64
>> > +
>> > +__efi64_start:
>> > +        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
>> > +
>> > +efi_multiboot2_proto:
>>
>> .L
> 
> Why do you want to hide labels which could be useful during debugging?

With a few exceptions, assembly code should follow C and other
high level languages: Symbol table entries only at function
boundaries (or whatever their suitable counterparts in assembly
are).

>> > +x86_32_switch:
>> > +        cli
>> > +
>> > +        /* Initialise GDT. */
>> > +        lgdt    gdt_boot_descr(%rip)
>> > +
>> > +        /* Reload code selector. */
>> > +        ljmpl   *cs32_switch_addr(%rip)
>> > +
>> > +        .code32
>> > +
>> > +cs32_switch:
>> > +        /* Initialise basic data segments. */
>> > +        mov     $BOOT_DS,%edx
>> > +        mov     %edx,%ds
>> > +        mov     %edx,%es
>> > +        mov     %edx,%fs
>> > +        mov     %edx,%gs
>> > +        mov     %edx,%ss
>>
>> I see no point in loading %fs and %gs with other than nul selectors.
>> I also think %ss should be loaded first. Which reminds me - what
>> about %rsp? Is it guaranteed to have its upper 32 bits clear, so you
>> can re-use the stack in 32-bit mode? (If it is, saying so in a comment
>> would be very desirable.)
> 
> I am not reusing %rsp value. %esp is initialized later in 32-bit code.
> Stack is not used until %esp is not initialized.

If you load %ss without loading the stack pointer, you should imo
at least add a comment saying when/where the other half will be
done.

>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -79,6 +79,17 @@ static size_t wstrlen(const CHAR16 * s);
>> >  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>> >  static bool_t match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
>> >
>> > +static void efi_init(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> > *SystemTable);
>> > +static void efi_console_set_mode(void);
>> > +static EFI_GRAPHICS_OUTPUT_PROTOCOL *efi_get_gop(void);
>> > +static UINTN efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>> > +                               UINTN cols, UINTN rows, UINTN depth);
>> > +static void efi_tables(void);
>> > +static void setup_efi_pci(void);
>> > +static void efi_variables(void);
>> > +static void efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN 
>> > gop_mode);
>> > +static void efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> > *SystemTable);
>>
>> This is ugly; I'm sure there is a way to avoid these declarations.
> 
> This probably requires play with '#include "efi-boot.h"' and move
> somewhere before efi_start(). Maybe something else. If it is not
> a problem for you I can do that.

Indeed moving an #include would seem far better than adding almost
a dozen declarations (any of which will need to get touched if the
respective definition changes, i.e. arranging for future churn).

Jan


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