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: > > Signed-off-by: Daniel Kiper <daniel.ki...@oracle.com> > > For a patch of this size, no description at all seems rather > problematic. > > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -89,6 +89,13 @@ multiboot1_header_end: > > 0, /* Number of the lines - no preference. */ \ > > 0 /* Number of bits per pixel - no preference. */ > > > > + /* Do not disable EFI boot services. */ > > + mb2ht_init MB2_HT(EFI_BS), MB2_HT(OPTIONAL) > > + > > + /* EFI64 entry point. */ > > + mb2ht_init MB2_HT(ENTRY_ADDRESS_EFI64), MB2_HT(OPTIONAL), \ > > + sym_phys(__efi64_start) > > Iirc at least one of those two is what upstream doesn't have yet, > or not all earlier grub2 versions might have. If so - what happens > if the resulting Xen gets booted on an incapable grub? Silent death > (with black screen)? Or at least some note to the user that the > grub2 version is not suitable?
There is the code below which sends relevant message to COM1 and... ...well, VGA which of course does not make sense and should be fixed. > > @@ -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? > > +run_bs: > > Again. Ditto. > > +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. [...] > > --- 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. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel