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

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2016 at 11:39:55AM +0100, Vladimir 'phcoder' Serbinenko wrote:
> On Wednesday, March 16, 2016, Toomas Soome <address@hidden> wrote:
> 
> >
> > > On 16. märts 2016, at 12:02, Daniel Kiper <address@hidden
> > <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