From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 20/23] x86: add multiboot2 protocol support for EFI platforms Date: Thu, 27 Aug 2015 06:01:26 -0600 Message-ID: <55DF1836020000780009D674__462.912492412393$1440677010$gmane$org@prv-mh.provo.novell.com> References: <1437402558-7313-1-git-send-email-daniel.kiper@oracle.com> <1437402558-7313-21-git-send-email-daniel.kiper@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZUvsC-0000Qo-GR for xen-devel@lists.xenproject.org; Thu, 27 Aug 2015 12:01:40 +0000 In-Reply-To: <1437402558-7313-21-git-send-email-daniel.kiper@oracle.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel Kiper Cc: Juergen Gross , grub-devel@gnu.org, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, keir@xen.org, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org List-Id: xen-devel@lists.xenproject.org >>> On 20.07.15 at 16:29, wrote: > Signed-off-by: Daniel Kiper 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? > @@ -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 > +run_bs: Again. > +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.) > @@ -182,7 +318,7 @@ multiboot2_proto: > and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx > jmp 0b > > -trampoline_setup: > +trampoline_bios_setup: Another .L candidate. > /* Set up trampoline segment 64k below EBDA */ > movzwl 0x40e,%ecx /* EBDA segment */ > cmp $0xa000,%ecx /* sanity check (high) */ > @@ -198,12 +334,13 @@ trampoline_setup: > * multiboot structure (if available) and use the smallest. > */ > cmp $0x100,%edx /* is the multiboot value too small? */ > - jb 2f /* if so, do not use it */ > + jb trampoline_setup /* if so, do not use it */ > shl $10-4,%edx > cmp %ecx,%edx /* compare with BDA value */ > cmovb %edx,%ecx /* and use the smaller */ > > -2: /* Reserve 64kb for the trampoline */ > +trampoline_setup: > + /* Reserve 64kb for the trampoline. */ And one more. > @@ -220,6 +357,13 @@ trampoline_setup: > add $12,%esp /* Remove reloc() args from stack. */ > mov %eax,sym_phys(multiboot_ptr) > > + /* > + * Do not zero BSS on EFI platform here. > + * It was initialized earlier. > + */ > + cmpb $1,sym_phys(skip_realmode) > + je 1f So why can't this be done uniformly here? I didn't see you write any variable in .bss by now. And if there was any, moving it to .data would avoid zeroing .bss in two different places. Or wait - I see, it's the C code in efi_multiboot2() that you need to take care of. Well, probably okay then this way, but please extend the comment at the new zeroing site to say _why_ it needs to be done differently/earlier. > +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > +{ > + EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; > + UINTN cols, gop_mode = ~0, rows; > + > + set_bit(EFI_PLATFORM, &efi.flags); > + > + efi_init(ImageHandle, SystemTable); > + > + efi_console_set_mode(); > + > + if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, > + &cols, &rows) == EFI_SUCCESS ) > + efi_arch_console_init(cols, rows); > + > + gop = efi_get_gop(); > + gop_mode = efi_find_gop_mode(gop, 0, 0, 0); > + efi_arch_edd(); > + efi_tables(); > + setup_efi_pci(); > + efi_variables(); > + efi_set_gop_mode(gop, gop_mode); > + efi_exit_boot(ImageHandle, SystemTable); > + > + return cfg.addr; With all the refactoring in the earlier patches it is now almost impossible to know what cfg.addr holds at this point - it looks to me as if this is entirely unrelated to the mem_lower value the caller wants to derive from this. Please at least add a brief comment. > --- 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! */ No "TODO" or alike please in non-RFC patch submissions (unless there are exceptional circumstances). Here - BUG() (or panic() if there was a remote chance of this being reached). > @@ -708,7 +708,11 @@ void __init noreturn __start_xen(unsigned long mbi_p) > l3_bootmap[l3_table_offset(BOOTSTRAP_MAP_BASE)] = > l3e_from_paddr(__pa(l2_bootmap), __PAGE_HYPERVISOR); > > - memmap_type = loader; > + memmap_type = "EFI"; What's wrong with keeping this line as is? > + } > + else if ( efi_enabled(EFI_PLATFORM) ) > + { > + memmap_type = "EFI"; > } The braces are now pointless here. > --- 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. Jan