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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
Cc: "jgross@suse.com" <jgross@suse.com>,
	"eric.snowberg@oracle.com" <eric.snowberg@oracle.com>,
	"grub-devel@gnu.org" <grub-devel@gnu.org>,
	Andrei Borzenkov <arvidjaar@gmail.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"cardoe@cardoe.com" <cardoe@cardoe.com>,
	"pgnet.dev@gmail.com" <pgnet.dev@gmail.com>,
	"roy.franz@linaro.org" <roy.franz@linaro.org>,
	"ning.sun@intel.com" <ning.sun@intel.com>,
	"david.vrabel@citrix.com" <david.vrabel@citrix.com>,
	"jbeulich@suse.com" <jbeulich@suse.com>,
	"stefano.stabellini@eu.citrix.com"
	<stefano.stabellini@eu.citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"qiaowei.ren@intel.com" <qiaowei.ren@intel.com>,
	"richard.l.maliszewski@intel.com"
	<richard.l.maliszewski@intel.com>,
	"gang.wei@intel.com" <gang.wei@intel.com>,
	"fu.wei@linaro.org" <fu.wei@linaro.org>,
	"seth.goldberg@oracle.com" <seth.goldberg@oracle.com>
Subject: Re: [GRUB2 PATCH v4 3/4 - FOR REVIEW ONLY] multiboot2: Do not pass memory maps to image if EFI boot services are enabled
Date: Tue, 15 Mar 2016 21:59:08 +0100	[thread overview]
Message-ID: <20160315205908.GD31771@olila.local.net-space.pl> (raw)
In-Reply-To: <CAEaD8JMgHphk31781sFpZVcT5+Q+Sj1LGpNfvhr3cbT=+sU1Lg@mail.gmail.com>

On Tue, Mar 15, 2016 at 06:10:48PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> Le mar. 15 mars 2016 19:06, Andrei Borzenkov <arvidjaar@gmail.com> 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@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.
> > >>
> >
> > 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?

Do you think about MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST tag? If yes it
looks that multiboot2 doc is a bit imprecise and/or GRUB2 code is wrong.

multiboot2 doc says:

[...]

If this tag is present and ‘optional’ is set to ‘0’ information conveyed by
requested tag types must be present. If bootloader is unable to supply this
information it must fail with an error
Note: it doesn’t garantee that any tags of type ‘mbi_tag_types’ will actually
be present. E.g. on a videoless system even if you requested tag ‘8’ no tags
of type ‘8’ will be present in mbi.

[...]

However, grub_multiboot_load() just looks for MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST
tag. If it exists and if ‘optional’ is set to ‘0’ then grub_multiboot_load() verify
list of requested tags. If unknown value is found then multiboot2 command fails
with "grub_error (GRUB_ERR_UNKNOWN_OS, "unsupported information tag: 0x%x", ...".
I cannot find additional checks anywhere...

So, AIUI, "If bootloader is unable to supply..." should be read as "If bootloader
does not support..." or even "If bootloader do not understand..." or something
like that. This does not mean that bootloader must have requested data in hand
and provide it to image. It means that bootloader must understand what image ask for.
And in this term my patch does not violate spec implied by implementation (well,
multiboot2 doc makes some confusion; see below). Image may ask/request for any
memory map. However, if a given map (or set of maps) is not available or it could
be provided but could make confusion in some cases then it can/should not be provided.

Anyway, I think that maybe multiboot2 doc should be fixed and more precise
MULTIBOOT_HEADER_TAG_INFORMATION_REQUEST tag description should be provided.
Or GRUB2 implementation is wrong then...

> Don't you just ignore the memory map in this case?

Yes to some extent. However, I think that loader should not provide
data which is known as (potentially) incorrect/faulty.

Daniel