Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms

From: Doug Goldstein <cardoe@cardoe.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: jgross@suse.com, sstabellini@kernel.org,
	andrew.cooper3@citrix.com, pgnet.dev@gmail.com,
	ning.sun@intel.com, julien.grall@arm.com, jbeulich@suse.com,
	xen-devel@lists.xenproject.org, qiaowei.ren@intel.com,
	gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms
Date: Thu, 12 Jan 2017 13:46:41 -0600	[thread overview]
Message-ID: <adb29af4-c381-9c47-0476-9cd6212299a0@cardoe.com> (raw)
In-Reply-To: <20170112193037.GI32675@olila.local.net-space.pl>

[-- Attachment #1.1.1: Type: text/plain, Size: 2897 bytes --]

On 1/12/17 1:30 PM, Daniel Kiper wrote:
> On Thu, Jan 12, 2017 at 09:44:59AM -0600, Doug Goldstein wrote:

> 
>> view there's no reason for adding MB2 support for BIOS since it provides
>> no advantage over MB1 when booting from the BIOS. Now MB2 solves a
> 
> From your point of view maybe it does not. However, from user point of view it may.
> If you have support for MB2 on legacy BIOS and EFI platforms then you can boot Xen
> on both platforms without changing anything in boot config files. Otherwise you have
> to prepare separate configuration for different platforms.

Neither Grub nor iPXE require different configs for MB1 vs MB2 so I'm
not seeing the validity of this logic.

> 
>> problem with booting over EFI vs MB1 so they'll be willing to take a
>> change there. I'll also disagree that BIOS is easier than EFI since with
>> EFI its just load the ELF into memory and set a few pointers in tags.
>> With BIOS it requires me to build up the memory map into a MB2 structure.
> 
> Xen uses only these tags on legacy BIOS platforms: MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
> (well, nice to have but it can be also not provided), MULTIBOOT2_TAG_TYPE_MMAP (same
> as MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO), MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME
> (same as MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO) ,MULTIBOOT2_TAG_TYPE_CMDLINE,
> MULTIBOOT2_TAG_TYPE_MODULE. I do not mention MULTIBOOT2_TAG_TYPE_END which
> is obvious. So, if you are real hardcore minimalist then you have to provide
> MULTIBOOT2_TAG_TYPE_CMDLINE and MULTIBOOT2_TAG_TYPE_MODULE. All of them
> are provided also on EFI. So, I do not see any reason to not provide MB2
> for legacy BIOS. And I do not think that it is very difficult to provide
> all optional tags mentioned above.

I don't understand what you're attempting to convey here. You've listed
out a number of tags that I mentioned in my message that I don't have to
implement for EFI. You've basically reinforced my point that its easier
to implement this for EFI than BIOS. MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO
and MULTIBOOT2_TAG_TYPE_MMAP are unused by Xen on EFI. It gets this info
from a call to GetMemoryMap(). You actually reminded me of another bug.
Calling ExitBootServices() on Grub and letting it pass the memory info
causes Xen to fail to load.

Andrew helped me troubleshoot this and he discovered the fix. You've got
code:

/* Store Xen image load base address in place accessible for 32-bit code. */
mov %r15d,%esi

But if any of the checks under the run_bs: label specifically:
- /* Are EFI boot services available? */
- /* Is EFI SystemTable address provided by boot loader? */
- /* Is EFI ImageHandle address provided by boot loader? */

Will not run the mov instruction and then fail to boot. Its only if any
of these are false will it attempt to use the tags mentioned above as well.

-- 
Doug Goldstein

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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