From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1YHME6-0008KQ-Ne for mharc-grub-devel@gnu.org; Fri, 30 Jan 2015 19:47:54 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHME4-0008KI-ER for grub-devel@gnu.org; Fri, 30 Jan 2015 19:47:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YHMDz-0003Ob-Ds for grub-devel@gnu.org; Fri, 30 Jan 2015 19:47:52 -0500 Received: from ppsw-50.csi.cam.ac.uk ([131.111.8.150]:55066) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YHMDz-0003O0-5h for grub-devel@gnu.org; Fri, 30 Jan 2015 19:47:47 -0500 X-Cam-AntiVirus: no malware found X-Cam-ScannerInfo: http://www.cam.ac.uk/cs/email/scanner/ Received: from client-82-26-162-3.pete.adsl.virginm.net ([82.26.162.3]:50091 helo=[192.168.1.193]) by ppsw-50.csi.cam.ac.uk (smtp.hermes.cam.ac.uk [131.111.8.158]:587) with esmtpsa (PLAIN:amc96) (TLSv1.2:DHE-RSA-AES128-SHA:128) id 1YHMDq-0002CO-qD (Exim 4.82_3-c0e5623) (return-path ); Sat, 31 Jan 2015 00:47:38 +0000 Message-ID: <54CC2630.3050806@citrix.com> Date: Sat, 31 Jan 2015 00:47:44 +0000 From: Andrew Cooper User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Daniel Kiper Subject: Re: [PATCH 18/18] x86: add multiboot2 protocol support for EFI platforms References: <1422640462-28103-1-git-send-email-daniel.kiper@oracle.com> <1422640462-28103-19-git-send-email-daniel.kiper@oracle.com> <54CBD64D.60605@citrix.com> <20150130234346.GG29167@olila.local.net-space.pl> In-Reply-To: <20150130234346.GG29167@olila.local.net-space.pl> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: Andrew Cooper X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 131.111.8.150 Cc: jgross@suse.com, grub-devel@gnu.org, keir@xen.org, ian.campbell@citrix.com, phcoder@gmail.com, stefano.stabellini@eu.citrix.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, jbeulich@suse.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 31 Jan 2015 00:47:53 -0000 On 30/01/2015 23:43, Daniel Kiper wrote: > On Fri, Jan 30, 2015 at 07:06:53PM +0000, Andrew Cooper wrote: >> On 30/01/15 17:54, Daniel Kiper wrote: >>> + >>> +efi_multiboot2_proto: >>> + /* Skip Multiboot2 information fixed part */ >>> + lea MB2_fixed_sizeof(%ebx),%ecx >>> + >>> +0: >>> + /* Get mem_lower from Multiboot2 information */ >>> + cmpl $MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO,(%ecx) >>> + jne 1f >>> + >>> + mov MB2_mem_lower(%ecx),%edx >>> + jmp 4f >>> + >>> +1: >>> + /* Get EFI SystemTable address from Multiboot2 information */ >>> + cmpl $MULTIBOOT2_TAG_TYPE_EFI64,(%ecx) >>> + jne 2f >>> + >>> + lea MB2_efi64_st(%ecx),%esi >>> + lea efi_st(%rip),%edi >>> + movsq >> This is complete overkill for copying a 64bit variable out of the tag >> and into a local variable. Just use a plain 64bit load and store. > I am not sure what do you mean by "64bit load and store" but I have > just realized that we do not need these variables. They are remnants > from early developments when I thought that we need ImageHandle > and SystemTable here and later somewhere else. mov MB2_efi64_st(%rcx), %rdi mov %rdi, efi_st(%rip) But if they are not needed, drop the code completely. >>> + jmp 4f >>> + >>> +3: >>> + /* Is it the end of Multiboot2 information? */ >>> + cmpl $MULTIBOOT2_TAG_TYPE_END,(%ecx) >>> + je run_bs >>> + >>> +4: >>> + /* Go to next Multiboot2 information tag */ >>> + add MB2_tag_size(%ecx),%ecx >>> + add $(MULTIBOOT2_TAG_ALIGN-1),%ecx >>> + and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx >>> + jmp 0b >>> + >>> +run_bs: >>> + push %rax >>> + push %rdx >> Does the EFI spec guarantee that we have a good stack to use at this point? > Unified Extensible Firmware Interface Specification, Version 2.4 Errata B, > section 2.3.4, x64 Platforms says: During boot services time the processor > is in the following execution mode: ..., 128 KiB, or more, of available > stack space. GRUB2 uses this stack too and do not move it to different > memory region. So, I think that here we are on safe side. Sounds ok then. > >>> + /* Initialize BSS (no nasty surprises!) */ >>> + lea __bss_start(%rip),%rdi >>> + lea _end(%rip),%rcx >>> + sub %rdi,%rcx >>> + xor %rax,%rax >> xor %eax,%eax is shorter. >> >>> + rep stosb >> It would be more efficient to make sure that the linker aligns >> __bss_start and _end on 8 byte boundaries, and use stosq instead. > Right but just for this. Is it pays? We do this only once. The BSS in Xen is 300k. It is absolutely better to clear it 8 bytes at a time rather than 1. > However, if you wish... > >>> + mov efi_ih(%rip),%rdi /* EFI ImageHandle */ >>> + mov efi_st(%rip),%rsi /* EFI SystemTable */ >>> + call efi_multiboot2 >>> + >>> + pop %rcx >>> + pop %rax >>> + >>> + shl $10-4,%rcx /* Convert multiboot2.mem_lower to bytes/16 */ >>> + >>> + cli >> This looks suspiciously out of place. Surely the EFI spec doesn't >> permit entry with interrupts enabled? > Unified Extensible Firmware Interface Specification, Version 2.4 Errata B, > section 2.3.4, x64 Platforms says: During boot services time the processor > is in the following execution mode: ..., Interrupts are enabled–though no > interrupt services are supported other than the UEFI boot services timer > functions (All loaded device drivers are serviced synchronously by “polling.”). > So, I think that we should use BS with interrupts enabled and disable > them after ExitBootServices(). Hmmm... Now I think that we should use > cli immediately after efi_multiboot2() call. I presume then that the firmware has set up a valid idt somewhere and is actually serving any interrupts we get. > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index f8be3dd..c5725ca 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -75,6 +75,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); > + >> If any of these forward declarations are needed, they should be > They are needed because efi-boot.h is included before above > mentioned functions definitions. > >> introduced in the appropriate create efi_$FOO patch. However, I can't > I thought about that during development. However, I stated that if I do what > you suggest then it is not clear who needs/uses these forward declarations. > >> spot a need for any of them. > efi-boot.h:efi_multiboot2() uses them. Ah - I see now. ~Andrew