From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vladimir 'phcoder' Serbinenko" Subject: Re: [GRUB2 PATCH v3 2/4] multiboot2: Add tags used to pass ImageHandle to loaded image Date: Fri, 11 Mar 2016 16:39:21 +0100 Message-ID: References: <1456937500-7855-1-git-send-email-daniel.kiper@oracle.com> <1456937500-7855-3-git-send-email-daniel.kiper@oracle.com> <20160311132710.GZ3500@olila.local.net-space.pl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8513882594064965715==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.84) (envelope-from ) id 1aeP9v-0000Cu-M6 for xen-devel@lists.xenproject.org; Fri, 11 Mar 2016 15:39:23 +0000 Received: by mail-wm0-f65.google.com with SMTP id n205so3133297wmf.2 for ; Fri, 11 Mar 2016 07:39:21 -0800 (PST) In-Reply-To: <20160311132710.GZ3500@olila.local.net-space.pl> 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 --===============8513882594064965715== Content-Type: multipart/alternative; boundary=001a1147cd9c1da341052dc7bce9 --001a1147cd9c1da341052dc7bce9 Content-Type: text/plain; charset=UTF-8 On Friday, March 11, 2016, Daniel Kiper wrote: > On Thu, Mar 10, 2016 at 09:26:06PM +0100, Vladimir 'phcoder' Serbinenko > wrote: > > On Wednesday, March 2, 2016, Daniel Kiper > wrote: > > > > > Add tags used to pass ImageHandle to loaded image if requested. > > > It is used by at least ExitBootServices() function. > > > > > > Signed-off-by: Daniel Kiper > > > > > --- > > > v3 - suggestions/fixes: > > > - mbi EFI related stuff size calculation > > > should depend on target architecture > > > (suggested by Konrad Rzeszutek Wilk), > > > - use plain type instead of pointer > > > dereference as sizeof() argument > > > (suggested by Konrad Rzeszutek Wilk), > > > - improve commit message > > > (suggested by Konrad Rzeszutek Wilk). > > > --- > > > grub-core/loader/multiboot_mbi2.c | 50 > > > ++++++++++++++++++++++++++++++------- > > > include/multiboot2.h | 16 ++++++++++++ > > > 2 files changed, 57 insertions(+), 9 deletions(-) > > > > > > diff --git a/grub-core/loader/multiboot_mbi2.c > > > b/grub-core/loader/multiboot_mbi2.c > > > index a3dca90..7591edc 100644 > > > --- a/grub-core/loader/multiboot_mbi2.c > > > +++ b/grub-core/loader/multiboot_mbi2.c > > > @@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const char > > > *filename) > > > case MULTIBOOT_TAG_TYPE_NETWORK: > > > case MULTIBOOT_TAG_TYPE_EFI_MMAP: > > > case MULTIBOOT_TAG_TYPE_EFI_BS: > > > + case MULTIBOOT_TAG_TYPE_EFI32_IH: > > > + case MULTIBOOT_TAG_TYPE_EFI64_IH: > > > break; > > > > > > default: > > > @@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void) > > > + grub_get_multiboot_mmap_count () > > > * sizeof (struct multiboot_mmap_entry)), > > > MULTIBOOT_TAG_ALIGN) > > > + ALIGN_UP (sizeof (struct multiboot_tag_framebuffer), > > > MULTIBOOT_TAG_ALIGN) > > > +#ifdef GRUB_MACHINE_EFI > > > +#ifdef __i386__ > > > + ALIGN_UP (sizeof (struct multiboot_tag_efi32), > MULTIBOOT_TAG_ALIGN) > > > + + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih), > > > MULTIBOOT_TAG_ALIGN) > > > +#endif > > > +#ifdef __x86_64__ > > > + ALIGN_UP (sizeof (struct multiboot_tag_efi64), > MULTIBOOT_TAG_ALIGN) > > > + + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih), > > > MULTIBOOT_TAG_ALIGN) > > > +#endif > > > + + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap) > > > + + efi_mmap_size, MULTIBOOT_TAG_ALIGN) > > > +#endif > > > > > It doesn't need to be exact. It's just an upper bound. Feel free to > > simplify knowing this, removing few #ifdef. > > OK. > > > > + ALIGN_UP (sizeof (struct multiboot_tag_old_acpi) > > > + sizeof (struct grub_acpi_rsdp_v10), > MULTIBOOT_TAG_ALIGN) > > > + acpiv2_size () > > > + net_size () > > > -#ifdef GRUB_MACHINE_EFI > > > - + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap) > > > - + efi_mmap_size, MULTIBOOT_TAG_ALIGN) > > > -#endif > > > + sizeof (struct multiboot_tag_vbe) + MULTIBOOT_TAG_ALIGN - 1 > > > + sizeof (struct multiboot_tag_apm) + MULTIBOOT_TAG_ALIGN - 1; > > > } > > > @@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *target) > > > > > > if (keep_bs) > > > { > > > - struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig; > > > - tag->type = MULTIBOOT_TAG_TYPE_EFI_BS; > > > - tag->size = sizeof (struct multiboot_tag); > > > - ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > > > - / sizeof (grub_properly_aligned_t); > > > + { > > > + struct multiboot_tag *tag = (struct multiboot_tag *) ptrorig; > > > + tag->type = MULTIBOOT_TAG_TYPE_EFI_BS; > > > + tag->size = sizeof (struct multiboot_tag); > > > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > > > + / sizeof (grub_properly_aligned_t); > > > + } > > > + > > > +#ifdef __i386__ > > > + { > > > + struct multiboot_tag_efi32_ih *tag = (struct > > > multiboot_tag_efi32_ih *) ptrorig; > > > + tag->type = MULTIBOOT_TAG_TYPE_EFI32_IH; > > > + tag->size = sizeof (struct multiboot_tag_efi32_ih); > > > + tag->pointer = (grub_addr_t) grub_efi_image_handle; > > > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > > > + / sizeof (grub_properly_aligned_t); > > > + } > > > +#endif > > > + > > > +#ifdef __x86_64__ > > > + { > > > + struct multiboot_tag_efi64_ih *tag = (struct > > > multiboot_tag_efi64_ih *) ptrorig; > > > + tag->type = MULTIBOOT_TAG_TYPE_EFI64_IH; > > > + tag->size = sizeof (struct multiboot_tag_efi64_ih); > > > + tag->pointer = (grub_addr_t) grub_efi_image_handle; > > > + ptrorig += ALIGN_UP (tag->size, MULTIBOOT_TAG_ALIGN) > > > + / sizeof (grub_properly_aligned_t); > > > + } > > > +#endif > > > } > > > #endif > > > > > > diff --git a/include/multiboot2.h b/include/multiboot2.h > > > index d96aa40..36a174f 100644 > > > --- a/include/multiboot2.h > > > +++ b/include/multiboot2.h > > > @@ -60,6 +60,8 @@ > > > #define MULTIBOOT_TAG_TYPE_NETWORK 16 > > > #define MULTIBOOT_TAG_TYPE_EFI_MMAP 17 > > > #define MULTIBOOT_TAG_TYPE_EFI_BS 18 > > > +#define MULTIBOOT_TAG_TYPE_EFI32_IH 19 > > > +#define MULTIBOOT_TAG_TYPE_EFI64_IH 20 > > > > > Is there a followup to define those in texi doc as well? > > No problem. Which one should I update? > It's in the branch 'multiboot2' in git: http://git.savannah.gnu.org/cgit/grub.git/tree/doc/multiboot.texi?h=multiboot2 > > Daniel > -- Regards Vladimir 'phcoder' Serbinenko --001a1147cd9c1da341052dc7bce9 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Friday, March 11, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:
On Thu, Mar 10, 2016 at 09:26:06PM +0100, Vladimir 'p= hcoder' Serbinenko wrote:
> On Wednesday, March 2, 2016, Daniel Kiper <d= aniel.kiper@oracle.com> wrote:
>
> > Add tags used to pass ImageHandle to loaded image if requested. > > It is used by at least ExitBootServices() function.
> >
> > Signed-off-by: Daniel Kiper <daniel.ki= per@oracle.com <javascript:;>>
> > ---
> > v3 - suggestions/fixes:
> >=C2=A0 =C2=A0 - mbi EFI related stuff size calculation
> >=C2=A0 =C2=A0 =C2=A0 should depend on target architecture
> >=C2=A0 =C2=A0 =C2=A0 (suggested by Konrad Rzeszutek Wilk),
> >=C2=A0 =C2=A0 - use plain type instead of pointer
> >=C2=A0 =C2=A0 =C2=A0 dereference as sizeof() argument
> >=C2=A0 =C2=A0 =C2=A0 (suggested by Konrad Rzeszutek Wilk),
> >=C2=A0 =C2=A0 - improve commit message
> >=C2=A0 =C2=A0 =C2=A0 (suggested by Konrad Rzeszutek Wilk).
> > ---
> >=C2=A0 grub-core/loader/multiboot_mbi2.c |=C2=A0 =C2=A050
> > ++++++++++++++++++++++++++++++-------
> >=C2=A0 include/multiboot2.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 |=C2=A0 =C2=A016 ++++++++++++
> >=C2=A0 2 files changed, 57 insertions(+), 9 deletions(-)
> >
> > diff --git a/grub-core/loader/multiboot_mbi2.c
> > b/grub-core/loader/multiboot_mbi2.c
> > index a3dca90..7591edc 100644
> > --- a/grub-core/loader/multiboot_mbi2.c
> > +++ b/grub-core/loader/multiboot_mbi2.c
> > @@ -172,6 +172,8 @@ grub_multiboot_load (grub_file_t file, const = char
> > *filename)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case MULTIB= OOT_TAG_TYPE_NETWORK:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case MULTIB= OOT_TAG_TYPE_EFI_MMAP:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case MULTIB= OOT_TAG_TYPE_EFI_BS:
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case MULTIBOOT_T= AG_TYPE_EFI32_IH:
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case MULTIBOOT_T= AG_TYPE_EFI64_IH:
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0brea= k;
> >
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0default: > > @@ -407,16 +409,22 @@ grub_multiboot_get_mbi_size (void)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 + g= rub_get_multiboot_mmap_count ()
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * s= izeof (struct multiboot_mmap_entry)),
> > MULTIBOOT_TAG_ALIGN)
> >=C2=A0 =C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_fram= ebuffer),
> > MULTIBOOT_TAG_ALIGN)
> > +#ifdef GRUB_MACHINE_EFI
> > +#ifdef __i386__
> >=C2=A0 =C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi3= 2), MULTIBOOT_TAG_ALIGN)
> > +=C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi32_ih)= ,
> > MULTIBOOT_TAG_ALIGN)
> > +#endif
> > +#ifdef __x86_64__
> >=C2=A0 =C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi6= 4), MULTIBOOT_TAG_ALIGN)
> > +=C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi64_ih)= ,
> > MULTIBOOT_TAG_ALIGN)
> > +#endif
> > +=C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)=
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+ efi_mma= p_size, MULTIBOOT_TAG_ALIGN)
> > +#endif
> >
> It doesn't need to be exact. It's just an upper bound. Feel fr= ee to
> simplify knowing this, removing few #ifdef.

OK.

> >=C2=A0 =C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_old_= acpi)
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+ si= zeof (struct grub_acpi_rsdp_v10), MULTIBOOT_TAG_ALIGN)
> >=C2=A0 =C2=A0 =C2=A0 + acpiv2_size ()
> >=C2=A0 =C2=A0 =C2=A0 + net_size ()
> > -#ifdef GRUB_MACHINE_EFI
> > -=C2=A0 =C2=A0 + ALIGN_UP (sizeof (struct multiboot_tag_efi_mmap)=
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+ efi_mma= p_size, MULTIBOOT_TAG_ALIGN)
> > -#endif
> >=C2=A0 =C2=A0 =C2=A0 + sizeof (struct multiboot_tag_vbe) + MULTIBO= OT_TAG_ALIGN - 1
> >=C2=A0 =C2=A0 =C2=A0 + sizeof (struct multiboot_tag_apm) + MULTIBO= OT_TAG_ALIGN - 1;
> >=C2=A0 }
> > @@ -907,11 +915,35 @@ grub_multiboot_make_mbi (grub_uint32_t *tar= get)
> >
> >=C2=A0 =C2=A0 if (keep_bs)
> >=C2=A0 =C2=A0 =C2=A0 {
> > -=C2=A0 =C2=A0 =C2=A0 struct multiboot_tag *tag =3D (struct multi= boot_tag *) ptrorig;
> > -=C2=A0 =C2=A0 =C2=A0 tag->type =3D MULTIBOOT_TAG_TYPE_EFI_BS;=
> > -=C2=A0 =C2=A0 =C2=A0 tag->size =3D sizeof (struct multiboot_t= ag);
> > -=C2=A0 =C2=A0 =C2=A0 ptrorig +=3D ALIGN_UP (tag->size, MULTIB= OOT_TAG_ALIGN)
> > -=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 *tag =3D (struct= multiboot_tag *) ptrorig;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->type =3D MULTIBOOT_TAG_TYPE_E= FI_BS;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->size =3D sizeof (struct multi= boot_tag);
> > +=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_aligne= d_t);
> > +=C2=A0 =C2=A0 =C2=A0 }
> > +
> > +#ifdef __i386__
> > +=C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct multiboot_tag_efi32_ih *tag = =3D (struct
> > multiboot_tag_efi32_ih *) ptrorig;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->type =3D MULTIBOOT_TAG_TYPE_E= FI32_IH;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->size =3D sizeof (struct multi= boot_tag_efi32_ih);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->pointer =3D (grub_addr_t) gru= b_efi_image_handle;
> > +=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_aligne= d_t);
> > +=C2=A0 =C2=A0 =C2=A0 }
> > +#endif
> > +
> > +#ifdef __x86_64__
> > +=C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0struct multiboot_tag_efi64_ih *tag = =3D (struct
> > multiboot_tag_efi64_ih *) ptrorig;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->type =3D MULTIBOOT_TAG_TYPE_E= FI64_IH;
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->size =3D sizeof (struct multi= boot_tag_efi64_ih);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0tag->pointer =3D (grub_addr_t) gru= b_efi_image_handle;
> > +=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_aligne= d_t);
> > +=C2=A0 =C2=A0 =C2=A0 }
> > +#endif
> >=C2=A0 =C2=A0 =C2=A0 }
> >=C2=A0 #endif
> >
> > diff --git a/include/multiboot2.h b/include/multiboot2.h
> > index d96aa40..36a174f 100644
> > --- a/include/multiboot2.h
> > +++ b/include/multiboot2.h
> > @@ -60,6 +60,8 @@
> >=C2=A0 #define MULTIBOOT_TAG_TYPE_NETWORK=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A016
> >=C2=A0 #define MULTIBOOT_TAG_TYPE_EFI_MMAP=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 17
> >=C2=A0 #define MULTIBOOT_TAG_TYPE_EFI_BS=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 18
> > +#define MULTIBOOT_TAG_TYPE_EFI32_IH=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 19
> > +#define MULTIBOOT_TAG_TYPE_EFI64_IH=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 20
> >
> Is there a followup to define those in texi doc as well?

No problem. Which one should I update?
It's in the= branch 'multiboot2' in git: http://git.savannah.= gnu.org/cgit/grub.git/tree/doc/multiboot.texi?h=3Dmultiboot2=C2=A0

Daniel


--
Regards
Vladimir 'phcoder' Serbinenk= o

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