>>> On 22.09.15 at 17:21, <daniel.ki...@oracle.com> wrote: > On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, <daniel.ki...@oracle.com> 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@lists.xen.org http://lists.xen.org/xen-devel