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: Tue, 22 Sep 2015 09:58:46 -0600 Message-ID: <560196D602000078000A491E__3534.92279905385$1442937606$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> <55DF1836020000780009D674@prv-mh.provo.novell.com> <20150922152117.GG3501@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZePy1-0002LN-67 for xen-devel@lists.xenproject.org; Tue, 22 Sep 2015 15:58:53 +0000 In-Reply-To: <20150922152117.GG3501@olila.local.net-space.pl> 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 22.09.15 at 17:21, wrote: > On Thu, Aug 27, 2015 at 06:01:26AM -0600, Jan Beulich wrote: >> >>> On 20.07.15 at 16:29, 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