Re: [Xen-devel] [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled

["Vladimir 'phcoder' Serbinenko" <phcoder@xxxxxxxxx>]

Le mar. 15 mars 2016 19:06, Andrei Borzenkov <arvidjaar@xxxxxxxxx> a écrit :

15.03.2016 19:07, Vladimir 'phcoder' Serbinenko пишет:
> Looks good. Let's give a day for others to comment. Is the second email the
> version for commit?
>
> On Tuesday, March 15, 2016, Daniel Kiper <daniel.kiper@xxxxxxxxxx> 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.
>>

Are there any existing users of it that rely on memory map provided by
bootloader? What if image explicitly requested non-optional memory map?
According to multiboot specification it is valid and bootloader must
either provide requested information or fail load.

I think you have a point. @Daniel, is current behavior a problem for you? Don't you just ignore the memory map in this case?

>> Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx <_javascript_:;>>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx <_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 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,12 +755,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
>> @@ -776,18 +777,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;
>> @@ -886,27 +888,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
>>
>>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel