| Message ID | 1458055562-24950-4-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <daniel.kiper@oracle.com> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > 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 >
On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote: > 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? AIUI, spec does not require that anything must be provided. Of course on every platform GRUB2 should provide minimal set of system information to properly boot loaded image. However, docs does not say which set make sense or not. This is role of image to know what it requires to properly run on a given platform. And I think that it make sense. > 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? IIRC, GRUB2 does some allocations after getting memory map and it is quite complicated to change that. > Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'.. Why provide map which is invalid and must be verified with something else? Let's ignore (do not provide) invalid data and use correct one without any additional (unneeded) checks. Daniel
> On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com> wrote: > > On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote: >> 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? > > AIUI, spec does not require that anything must be provided. Of course > on every platform GRUB2 should provide minimal set of system information > to properly boot loaded image. However, docs does not say which set make > sense or not. This is role of image to know what it requires to properly > run on a given platform. And I think that it make sense. > >> 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? > > IIRC, GRUB2 does some allocations after getting memory map and it is > quite complicated to change that. > >> Wish there was some multboot memory map flag indicating 'STALE-CHECK-EFI'.. > > Why provide map which is invalid and must be verified with something else? > Let's ignore (do not provide) invalid data and use correct one without > any additional (unneeded) checks. > If you are *not* calling exit efi boot services from grub, there is no sense to provide EFI memory map at all, because to exit boot services [from OS], you *have to* fetch current memory map to get the map key to exit boot services. basically it means, if BS is not disabled and grub is still providing EFI memory map, the OS has to assume this map is not valid - which is bad and better not to provide (potentially) invalid data in first place. rgds, toomas
On Wednesday, March 16, 2016, Toomas Soome <tsoome@me.com> wrote: > > > On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com > <javascript:;>> wrote: > > > > On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote: > >> 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? > > > > AIUI, spec does not require that anything must be provided. Of course > > on every platform GRUB2 should provide minimal set of system information > > to properly boot loaded image. However, docs does not say which set make > > sense or not. This is role of image to know what it requires to properly > > run on a given platform. And I think that it make sense. > > > >> 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? > > > > IIRC, GRUB2 does some allocations after getting memory map and it is > > quite complicated to change that. > > > >> Wish there was some multboot memory map flag indicating > 'STALE-CHECK-EFI'.. > > > > Why provide map which is invalid and must be verified with something > else? > > Let's ignore (do not provide) invalid data and use correct one without > > any additional (unneeded) checks. > > > > If you are *not* calling exit efi boot services from grub, there is no > sense to provide EFI memory map at all, because to exit boot services [from > OS], you *have to* fetch current memory map to get the map key to exit boot > services. > But this doesn't apply for e820-like and simple maps which are unaffected by bootloader allocations > > basically it means, if BS is not disabled and grub is still providing EFI > memory map, the OS has to assume this map is not valid - which is bad and > better not to provide (potentially) invalid data in first place. > > rgds, > toomas
On Wed, Mar 16, 2016 at 11:39:55AM +0100, Vladimir 'phcoder' Serbinenko wrote: > On Wednesday, March 16, 2016, Toomas Soome <tsoome@me.com> wrote: > > > > > > On 16. märts 2016, at 12:02, Daniel Kiper <daniel.kiper@oracle.com > > <javascript:;>> wrote: > > > > > > On Tue, Mar 15, 2016 at 07:46:46PM -0400, Konrad Rzeszutek Wilk wrote: > > >> 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? > > > > > > AIUI, spec does not require that anything must be provided. Of course > > > on every platform GRUB2 should provide minimal set of system information > > > to properly boot loaded image. However, docs does not say which set make > > > sense or not. This is role of image to know what it requires to properly > > > run on a given platform. And I think that it make sense. > > > > > >> 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? > > > > > > IIRC, GRUB2 does some allocations after getting memory map and it is > > > quite complicated to change that. > > > > > >> Wish there was some multboot memory map flag indicating > > 'STALE-CHECK-EFI'.. > > > > > > Why provide map which is invalid and must be verified with something > > else? > > > Let's ignore (do not provide) invalid data and use correct one without > > > any additional (unneeded) checks. > > > > > > > If you are *not* calling exit efi boot services from grub, there is no > > sense to provide EFI memory map at all, because to exit boot services [from > > OS], you *have to* fetch current memory map to get the map key to exit boot > > services. > > > But this doesn't apply for e820-like and simple maps which are unaffected > by bootloader allocations Right. I think we are all on the same page then. If EFI and payload (OS) has requested to be the one executing ExitBootServices then don't provide the memory map? But if the payload hasn't specified what to do with ExitBootServices, GRUB is free to do so and provide the map. > > > > > basically it means, if BS is not disabled and grub is still providing EFI > > memory map, the OS has to assume this map is not valid - which is bad and > > better not to provide (potentially) invalid data in first place. Right. And the description of the patch (to my reading) was not exactly clear on this. Perhaps just updating the description of the patch and repositing as reply here would suffice? > > > > rgds, > > toomas > > > > -- > Regards > Vladimir 'phcoder' Serbinenko
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) {