Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 08, 2019 at 10:40:36AM +0100, Andrew Cooper wrote:
> On 08/08/2019 01:31, Marek Marczykowski-Górecki wrote:
> > When booting Xen via xen.efi, there is /mapbs option to workaround
> > certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> > map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> > cmdline for the same effect and parse it very early in the
> > multiboot2+EFI boot path.
> >
> > Normally cmdline is parsed after relocating MB2 structure, which happens
> > too late. To have efi= parsed early enough, save cmdline pointer in
> > head.S and pass it as yet another argument to efi_multiboot2(). This
> > way we avoid introducing yet another MB2 structure parser.
> >
> > To keep consistency, handle efi= parameter early in xen.efi too, both in
> > xen.efi command line and cfg file.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> I'm very sorry to do this to you, but I'm going to object to the patch
> (in principle.  I think the command line option itself is fine but...)
> 
> What does Linux do differently here?

I've found this comment:
/*
 * The UEFI specification makes it clear that the operating system is
 * free to do whatever it wants with boot services code after
 * ExitBootServices() has been called. Ignoring this recommendation a
 * significant bunch of EFI implementations continue calling into boot
 * services code (SetVirtualAddressMap). In order to work around such
 * buggy implementations we reserve boot services region during EFI
 * init and make sure it stays executable. Then, after
 * SetVirtualAddressMap(), it is discarded.
 *
 * However, some boot services regions contain data that is required
 * by drivers, so we need to track which memory ranges can never be
 * freed. This is done by tagging those regions with the
 * EFI_MEMORY_RUNTIME attribute.
 *
 * Any driver that wants to mark a region as reserved must use
 * efi_mem_reserve() which will insert a new EFI memory descriptor
 * into efi.memmap (splitting existing regions if necessary) and tag
 * it with EFI_MEMORY_RUNTIME.
 */

So, for start, Linux has "/mapbs" enabled by default. But as you see in
the other thread, it isn't enough. Given the above comment, I more and
more think it is also about SetVirtualAddressMap() call. According to a
git log, SetVirtualAddressMap is not called in Xen, as it's incompatible
with kexec (can be called only once). Looking at Linux code, this is
worked around by passing efi memory regions from the old map to the new
one - at least this is my understanding of kexec_enter_virtual_mode() in
arch/x86/platform/efi/efi.c. For example this comment:

    /*
    * Map efi regions which were passed via setup_data. The virt_addr is a
    * fixed addr which was used in first kernel of a kexec boot.
    */

For my use case, I don't care about kexec, so I'd happily enable
SetVirtualAddressMap() call (it's under #ifdef), but according to
comments and that it wasn't touched since 2011, I don't expect it work
anymore.


> It is actively damaging to the Xen community to users to force users
> tweak command lines in order to boot/recover their system, and it looks
> especially embarrassing when other OSes cope automatically.  We have
> compatibility for all kinds of other firmware screw-ups, except EFI it
> seems, and this needs to change.

I _guess_ calling SetVirtualAddressMap() would help here, but it is much
more complex change than I'd like to do now.
What do you think about adding this option _and_ a heuristic when to
enable it automatically? In this case I'd say to start with
vendor=Lenovo based on my experience...

> So while I have no objection to the option per say, I don't think this
> patch is reasonable as a "fix" to the problem as far as end users are
> concerned.
> 
> ~Andrew

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel