[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platfo
From: |
Jan Beulich |
Subject: |
Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms |
Date: |
Tue, 22 Sep 2015 09:58:46 -0600 |
>>> On 22.09.15 at 17:21, <address@hidden> wrote:
> On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote:
>> >>> On 20.07.15 at 16:29, <address@hidden> 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