| Message ID | 1456937500-7855-4-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> 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 <daniel.kiper@oracle.com <javascript:;>> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com <javascript:;>> > --- > 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 > >
On Thu, Mar 10, 2016 at 09:28:25PM +0100, Vladimir 'phcoder' Serbinenko wrote: > On Wednesday, March 2, 2016, Daniel Kiper <daniel.kiper@oracle.com> 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) Sure thing! I will prepare whole updated patch series next week. I hope that it is OK for you. Daniel
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)