From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vladimir 'phcoder' Serbinenko" Subject: Re: [GRUB2 PATCH v3 3/4] multiboot2: Do not pass memory maps to image if EFI boot services are enabled Date: Thu, 10 Mar 2016 21:28:25 +0100 Message-ID: References: <1456937500-7855-1-git-send-email-daniel.kiper@oracle.com> <1456937500-7855-4-git-send-email-daniel.kiper@oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5389186207156940673==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.84) (envelope-from ) id 1ae7C7-00088u-JM for xen-devel@lists.xenproject.org; Thu, 10 Mar 2016 20:28:27 +0000 Received: by mail-wm0-f67.google.com with SMTP id p65so359079wmp.1 for ; Thu, 10 Mar 2016 12:28:25 -0800 (PST) In-Reply-To: <1456937500-7855-4-git-send-email-daniel.kiper@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Daniel Kiper Cc: "jgross@suse.com" , "grub-devel@gnu.org" , "eric.snowberg@oracle.com" , "arvidjaar@gmail.com" , "andrew.cooper3@citrix.com" , "cardoe@cardoe.com" , "pgnet.dev@gmail.com" , "roy.franz@linaro.org" , "ning.sun@intel.com" , "david.vrabel@citrix.com" , "jbeulich@suse.com" , "stefano.stabellini@eu.citrix.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" List-Id: xen-devel@lists.xenproject.org --===============5389186207156940673== Content-Type: multipart/alternative; boundary=001a11c287060f77d6052db7a850 --001a11c287060f77d6052db7a850 Content-Type: text/plain; charset=UTF-8 On Wednesday, March 2, 2016, Daniel Kiper wrote: > Do not pass memory maps to image if it asked for EFI boot services. > 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. > If we keep boot services then it is easier to not provide maps. However, > if image needs memory maps and they are not provided by bootloader then > it should get them itself just before ExitBootServices() call. > Patch gets too cluttered. Can you provide 2 variants: normal (for commit) and with ignoring whitespaces (for review) > > 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 | 71 > ++++++++++++++++++------------------- > 1 file changed, 35 insertions(+), 36 deletions(-) > > diff --git a/grub-core/loader/multiboot_mbi2.c > b/grub-core/loader/multiboot_mbi2.c > index 7591edc..ce68f48 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) > @@ -759,12 +759,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > } > } > > - { > - struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) > ptrorig; > - grub_fill_multiboot_mmap (tag); > - ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > - / sizeof (grub_properly_aligned_t); > - } > + if (!keep_bs) > + { > + struct multiboot_tag_mmap *tag = (struct multiboot_tag_mmap *) > ptrorig; > + grub_fill_multiboot_mmap (tag); > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > + / sizeof (grub_properly_aligned_t); > + } > > { > struct multiboot_tag_elf_sections *tag > @@ -780,18 +781,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > / sizeof (grub_properly_aligned_t); > } > > - { > - struct multiboot_tag_basic_meminfo *tag > - = (struct multiboot_tag_basic_meminfo *) ptrorig; > - tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO; > - tag->size = sizeof (struct multiboot_tag_basic_meminfo); > + if (!keep_bs) > + { > + struct multiboot_tag_basic_meminfo *tag > + = (struct multiboot_tag_basic_meminfo *) ptrorig; > + tag->type = MULTIBOOT_TAG_TYPE_BASIC_MEMINFO; > + tag->size = sizeof (struct multiboot_tag_basic_meminfo); > > - /* Convert from bytes to kilobytes. */ > - tag->mem_lower = grub_mmap_get_lower () / 1024; > - tag->mem_upper = grub_mmap_get_upper () / 1024; > - ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > - / sizeof (grub_properly_aligned_t); > - } > + /* Convert from bytes to kilobytes. */ > + tag->mem_lower = grub_mmap_get_lower () / 1024; > + tag->mem_upper = grub_mmap_get_upper () / 1024; > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > + / sizeof (grub_properly_aligned_t); > + } > > { > struct grub_net_network_level_interface *net; > @@ -890,27 +892,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > grub_efi_uintn_t efi_desc_size; > grub_efi_uint32_t efi_desc_version; > > - 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"); > + tag->type = MULTIBOOT_TAG_TYPE_EFI_MMAP; > + tag->size = sizeof (*tag) + efi_mmap_size; > + > + err = grub_efi_finish_boot_services (&efi_mmap_size, > tag->efi_mmap, NULL, > + &efi_desc_size, > &efi_desc_version); > + > + if (err) > + return err; > + > + tag->descr_size = efi_desc_size; > + tag->descr_vers = efi_desc_version; > + tag->size = sizeof (*tag) + efi_mmap_size; > + > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > + / sizeof (grub_properly_aligned_t); > } > - if (err) > - return err; > - tag->descr_size = efi_desc_size; > - tag->descr_vers = efi_desc_version; > - tag->size = sizeof (*tag) + efi_mmap_size; > - > - ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > - / sizeof (grub_properly_aligned_t); > } > > if (keep_bs) > -- > 1.7.10.4 > > -- Regards Vladimir 'phcoder' Serbinenko --001a11c287060f77d6052db7a850 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
Do not pass memory maps to image if it asked for EFI bo= ot services.
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.
If we keep boot services then it is easier to not provide maps. However, if image needs memory maps and they are not provided by bootloader then
it should get them itself just before ExitBootServices() call.
Patch gets too cluttered. Can you provide 2 variants: normal (for = commit) and with ignoring whitespaces (for review)

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.= com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@or= acle.com>
---
v3 - suggestions/fixes:
=C2=A0 =C2=A0- improve commit message
=C2=A0 =C2=A0 =C2=A0(suggested by Konrad Rzeszutek Wilk and Vladimir 'p= hcoder' Serbinenko).
---
=C2=A0grub-core/loader/multiboot_mbi2.c |=C2=A0 =C2=A071 ++++++++++++++++++= -------------------
=C2=A01 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot= _mbi2.c
index 7591edc..ce68f48 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -390,7 +390,7 @@ static grub_size_t
=C2=A0grub_multiboot_get_mbi_size (void)
=C2=A0{
=C2=A0#ifdef GRUB_MACHINE_EFI
-=C2=A0 if (!efi_mmap_size)
+=C2=A0 if (!keep_bs && !efi_mmap_size)
=C2=A0 =C2=A0 =C2=A0find_efi_mmap_size ();
=C2=A0#endif
=C2=A0 =C2=A0return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_t= ag)
@@ -759,12 +759,13 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
=C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0}

-=C2=A0 {
-=C2=A0 =C2=A0 struct multiboot_tag_mmap *tag =3D (struct multiboot_tag_mma= p *) ptrorig;
-=C2=A0 =C2=A0 grub_fill_multiboot_mmap (tag);
-=C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) -=C2=A0 =C2=A0 =C2=A0 / sizeof (grub_properly_aligned_t);
-=C2=A0 }
+=C2=A0 if (!keep_bs)
+=C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 struct multiboot_tag_mmap *tag =3D (struct multiboot_= tag_mmap *) ptrorig;
+=C2=A0 =C2=A0 =C2=A0 grub_fill_multiboot_mmap (tag);
+=C2=A0 =C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_TAG_AL= IGN)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/ sizeof (grub_properly_aligned_t);
+=C2=A0 =C2=A0 }

=C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0struct multiboot_tag_elf_sections *tag
@@ -780,18 +781,19 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
=C2=A0 =C2=A0 =C2=A0 =C2=A0/ sizeof (grub_properly_aligned_t);
=C2=A0 =C2=A0}

-=C2=A0 {
-=C2=A0 =C2=A0 struct multiboot_tag_basic_meminfo *tag
-=C2=A0 =C2=A0 =C2=A0 =3D (struct multiboot_tag_basic_meminfo *) ptrorig; -=C2=A0 =C2=A0 tag->type =3D MULTIBOOT_TAG_TYPE_BASIC_MEMINFO;
-=C2=A0 =C2=A0 tag->size =3D sizeof (struct multiboot_tag_basic_meminfo)= ;
+=C2=A0 if (!keep_bs)
+=C2=A0 =C2=A0 {
+=C2=A0 =C2=A0 =C2=A0 struct multiboot_tag_basic_meminfo *tag
+=C2=A0 =C2=A0 =C2=A0 =C2=A0=3D (struct multiboot_tag_basic_meminfo *) ptro= rig;
+=C2=A0 =C2=A0 =C2=A0 tag->type =3D MULTIBOOT_TAG_TYPE_BASIC_MEMINFO; +=C2=A0 =C2=A0 =C2=A0 tag->size =3D sizeof (struct multiboot_tag_basic_m= eminfo);

-=C2=A0 =C2=A0 /* Convert from bytes to kilobytes.=C2=A0 */
-=C2=A0 =C2=A0 tag->mem_lower =3D grub_mmap_get_lower () / 1024;
-=C2=A0 =C2=A0 tag->mem_upper =3D grub_mmap_get_upper () / 1024;
-=C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) -=C2=A0 =C2=A0 =C2=A0 =C2=A0/ sizeof (grub_properly_aligned_t);
-=C2=A0 }
+=C2=A0 =C2=A0 =C2=A0 /* Convert from bytes to kilobytes.=C2=A0 */
+=C2=A0 =C2=A0 =C2=A0 tag->mem_lower =3D grub_mmap_get_lower () / 1024;<= br> +=C2=A0 =C2=A0 =C2=A0 tag->mem_upper =3D grub_mmap_get_upper () / 1024;<= br> +=C2=A0 =C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_TAG_AL= IGN)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0/ sizeof (grub_properly_aligned_t);
+=C2=A0 =C2=A0 }

=C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0struct grub_net_network_level_interface *net;
@@ -890,27 +892,24 @@ grub_multiboot_make_mbi (grub_uint32_t *target)
=C2=A0 =C2=A0 =C2=A0grub_efi_uintn_t efi_desc_size;
=C2=A0 =C2=A0 =C2=A0grub_efi_uint32_t efi_desc_version;

-=C2=A0 =C2=A0 tag->type =3D MULTIBOOT_TAG_TYPE_EFI_MMAP;
-=C2=A0 =C2=A0 tag->size =3D sizeof (*tag) + efi_mmap_size;
-
=C2=A0 =C2=A0 =C2=A0if (!keep_bs)
-=C2=A0 =C2=A0 =C2=A0 err =3D grub_efi_finish_boot_services (&efi_mmap_= size, tag->efi_mmap, NULL,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &= amp;efi_desc_size, &efi_desc_version);
-=C2=A0 =C2=A0 else
=C2=A0 =C2=A0 =C2=A0 =C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (grub_efi_get_memory_map (&efi_mmap_size= , (void *) tag->efi_mmap,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 NULL,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &efi_desc_size, &a= mp;efi_desc_version) <=3D 0)
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0err =3D grub_error (GRUB_ERR_IO, "c= ouldn't retrieve memory map");
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->type =3D MULTIBOOT_TAG_TYPE_EFI_MMAP; +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->size =3D sizeof (*tag) + efi_mmap_size;=
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0err =3D grub_efi_finish_boot_services (&efi= _mmap_size, tag->efi_mmap, NULL,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 &efi_desc_size, &efi_desc_version);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (err)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return err;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->descr_size =3D efi_desc_size;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->descr_vers =3D efi_desc_version;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->size =3D sizeof (*tag) + efi_mmap_size;=
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_= TAG_ALIGN)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/ sizeof (grub_properly_aligned_t);
=C2=A0 =C2=A0 =C2=A0 =C2=A0}
-=C2=A0 =C2=A0 if (err)
-=C2=A0 =C2=A0 =C2=A0 return err;
-=C2=A0 =C2=A0 tag->descr_size =3D efi_desc_size;
-=C2=A0 =C2=A0 tag->descr_vers =3D efi_desc_version;
-=C2=A0 =C2=A0 tag->size =3D sizeof (*tag) + efi_mmap_size;
-
-=C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) -=C2=A0 =C2=A0 =C2=A0 / sizeof (grub_properly_aligned_t);
=C2=A0 =C2=A0}

=C2=A0 =C2=A0if (keep_bs)
--
1.7.10.4



--
Regards
Vladimir 'phcoder' Serbinenk= o

--001a11c287060f77d6052db7a850-- --===============5389186207156940673== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============5389186207156940673==--