From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1afyg8-0002zz-12 for mharc-grub-devel@gnu.org; Tue, 15 Mar 2016 19:47:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afyg5-0002zZ-U0 for grub-devel@gnu.org; Tue, 15 Mar 2016 19:47:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afyg0-0008LB-Sr for grub-devel@gnu.org; Tue, 15 Mar 2016 19:47:05 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:30136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afyg0-0008Ke-LK for grub-devel@gnu.org; Tue, 15 Mar 2016 19:47:00 -0400 Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u2FNko2N000557 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 15 Mar 2016 23:46:50 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0021.oracle.com (8.13.8/8.13.8) with ESMTP id u2FNknK9017921 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL); Tue, 15 Mar 2016 23:46:49 GMT Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by aserv0122.oracle.com (8.13.8/8.13.8) with ESMTP id u2FNkm78028307; Tue, 15 Mar 2016 23:46:49 GMT Received: from char.us.oracle.com (/10.137.176.158) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 15 Mar 2016 16:46:48 -0700 Received: by char.us.oracle.com (Postfix, from userid 1000) id CCD586A00B7; Tue, 15 Mar 2016 19:46:46 -0400 (EDT) Date: Tue, 15 Mar 2016 19:46:46 -0400 From: Konrad Rzeszutek Wilk To: Daniel Kiper Subject: Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Message-ID: <20160315234646.GE29495@char.us.oracle.com> References: <1458055562-24950-1-git-send-email-daniel.kiper@oracle.com> <1458055562-24950-4-git-send-email-daniel.kiper@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458055562-24950-4-git-send-email-daniel.kiper@oracle.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Source-IP: userv0021.oracle.com [156.151.31.71] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] X-Received-From: 156.151.31.81 Cc: jgross@suse.com, grub-devel@gnu.org, eric.snowberg@oracle.com, arvidjaar@gmail.com, andrew.cooper3@citrix.com, stefano.stabellini@eu.citrix.com, cardoe@cardoe.com, pgnet.dev@gmail.com, roy.franz@linaro.org, ning.sun@intel.com, david.vrabel@citrix.com, jbeulich@suse.com, phcoder@gmail.com, xen-devel@lists.xenproject.org, qiaowei.ren@intel.com, richard.l.maliszewski@intel.com, gang.wei@intel.com, fu.wei@linaro.org, seth.goldberg@oracle.com 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: Tue, 15 Mar 2016 23:47:07 -0000 On Tue, Mar 15, 2016 at 04:26:00PM +0100, Daniel Kiper wrote: > Do not pass memory maps to image if it asked for EFI boot services. .. I would change this sentence a bit. If image requested EFI boot services then skip multiboot2 memory maps. > Main reason for not providing maps is because they will likely be > invalid. We do a few allocations after filling them, e.g. for relocator > needs. Usually we do not care as we would already finish boot services. s/would already finish/would have finished/ ? > If we keep boot services then it is easier to not provide maps. However, s/easier/safer?/ > if image needs memory maps and they are not provided by bootloader then > it should get them itself just before ExitBootServices() call. s/them// ? That is making an assumption that the user of Multiboot2 + EFI will do this. Which is OK since only Xen is using it.. but is this inline with the spec? Can you ignore not providing this information? That aside - why not sync the multiboot memory map with what the EFI one will be? Or is it too to complex to move the memory map creation to a later phase of the bootup? Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'.. > > Signed-off-by: Daniel Kiper > Reviewed-by: Konrad Rzeszutek Wilk > --- > v3 - suggestions/fixes: > - improve commit message > (suggested by Konrad Rzeszutek Wilk and Vladimir 'phcoder' Serbinenko). > --- > grub-core/loader/multiboot_mbi2.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c > index 6c04402..ad1553b 100644 > --- a/grub-core/loader/multiboot_mbi2.c > +++ b/grub-core/loader/multiboot_mbi2.c > @@ -390,7 +390,7 @@ static grub_size_t > grub_multiboot_get_mbi_size (void) > { > #ifdef GRUB_MACHINE_EFI > - if (!efi_mmap_size) > + if (!keep_bs && !efi_mmap_size) > find_efi_mmap_size (); > #endif > return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag) > @@ -755,6 +755,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > } > } > > + if (!keep_bs) > { > struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) ptrorig; > grub_fill_multiboot_mmap (tag); > @@ -776,6 +777,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > / sizeof (grub_properly_aligned_t); > } > > + if (!keep_bs) > { > struct multiboot_tag_basic_meminfo *tag > = (struct multiboot_tag_basic_meminfo *) ptrorig; > @@ -886,21 +888,17 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > grub_efi_uintn_t efi_desc_size; > grub_efi_uint32_t efi_desc_version; > > + if (!keep_bs) > + { > tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP; > tag->size = sizeof (*tag) + efi_mmap_size; > > - if (!keep_bs) > err = grub_efi_finish_boot_services (&efi_mmap_size, tag->efi_mmap, NULL, > &efi_desc_size, &efi_desc_version); > - else > - { > - if (grub_efi_get_memory_map (&efi_mmap_size, (void *) tag->efi_mmap, > - NULL, > - &efi_desc_size, &efi_desc_version) <= 0) > - err = grub_error (GRUB_ERR_IO, "couldn't retrieve memory map"); > - } > + > if (err) > return err; > + > tag->descr_size = efi_desc_size; > tag->descr_vers = efi_desc_version; > tag->size = sizeof (*tag) + efi_mmap_size; > @@ -908,6 +906,7 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > / sizeof (grub_properly_aligned_t); > } > + } > > if (keep_bs) > { > -- > 1.7.10.4 >