Re: [Xen-devel] Xen 4.12 panic on Thinkpad W540 with UEFI mutiboot2, efi=no-rs workarounds it

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6813 bytes --]

On Tue, Oct 08, 2019 at 03:08:29PM +0200, Jan Beulich wrote:
> On 08.10.2019 13:50, Marek Marczykowski-Górecki  wrote:
> > On Thu, Aug 08, 2019 at 08:03:49AM +0200, Jan Beulich wrote:
> >> On 08.08.2019 04:53, Marek Marczykowski-Górecki  wrote:
> >>> On Wed, Aug 07, 2019 at 09:26:00PM +0200, Marek Marczykowski-Górecki wrote:
> >>>> Ok, regardless of adding proper option for that, I've hardcoded map_bs=1
> >>>> and it still crashes, just slightly differently:
> >>>>
> >>>>      Xen call trace:
> >>>>         [<0000000000000080>] 0000000000000080
> >>>>         [<8c2b0398e0000daa>] 8c2b0398e0000daa
> >>>>
> >>>>      Pagetable walk from ffffffff858483a1:
> >>>>         L4[0x1ff] = 0000000000000000 ffffffffffffffff
> >>>>
> >>>>      ****************************************
> >>>>      Panic on CPU 0:
> >>>>      FATAL PAGE FAULT
> >>>>      [error_code=0002]
> >>>>      Faulting linear address: ffffffff858483a1
> >>>>      ****************************************
> >>>>
> >>>> Full message attached.
> >>>
> >>> After playing more with it and also know workarounds for various EFI
> >>> issues, I've found a way to boot it: avoid calling Exit BootServices.
> >>> There was a patch from Konrad adding /noexit option, that never get
> >>> committed. Similar to efi=mapbs option, I'd add efi=no-exitboot too
> >>> (once efi=mapbs patch is accepted).
> >>>
> >>> Anyway, I'm curious what exactly is wrong here. Is it that the firmware
> >>> is not happy about lack of SetVirtualAddressMap call? FWIW, the crash is
> >>> during GetVariable RS call. I've verified that the function itself is
> >>> within EfiRuntimeServicesCode, but I don't feel like tracing Lenovo
> >>> UEFI...
> >>
> >> This suggests that the firmware zaps a few too many pointers
> >> during ExitBootServices(). Perhaps internally they check
> >> whether pointers point into BootServices* memory, and hence the
> >> wrong marking in the memory map has consequences beyond the OS
> >> re-using such memory?
> >>
> >> A proper answer to your question can of course only be given
> >> by someone knowing this specific firmware version.
> > 
> > I explored it a bit more and talked with a few people doing firmware
> > development and few conclusions:
> > 1. Not calling SetVirtualAddressMap(), while technically legal, is
> > pretty uncommon and not recommended if you want to avoid less tested
> > (aka buggy) UEFI code paths.
> > 2. Every UEFI call before SetVirtualAddressMap() call should be done
> > with flat physical memory. This include SetVirtualAddressMap() call
> > itself. Implicitly this means such calls can legally access memory areas
> > not marked with EFI_MEMORY_RUNTIME.
> 
> I don't think this is quite right - whether non-runtime memory may
> be touched depends exclusively on ExitBootServices() (not) having
> got called (yet).

That would be logical. In practice however we have evidences firmware
vendors have different opinion... A comment from Linux (already quoted
here 2 months ago):

    /*
     * 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.
     */

Regardless of SetVirtualAddressMap() discussion, I propose to
automatically map boot services code/data, to make Xen work on more
machines (even if _we_ consider those buggy). 

> > Then I've tried a different approach: call SetVirtualAddressMap(), but
> > with an address map that tries to pretend physical addressing (the code
> > under #ifndef USE_SET_VIRTUAL_ADDRESS_MAP). This mostly worked, I needed
> > only few changes:
> >  - set VirtualStart back to PhysicalStart in that memory map (it was set
> >    to directmap)
> >  - map boot services (at least for the SetVirtualAddressMap() call time,
> >    but haven't tried unmapping it later)
> >  - call SetVirtualAddressMap() with that "1:1" map in place, using
> >    efi_rs_enter/efi_rs_leave.
> > 
> > This fixed the issue for me, now runtime services do work even without
> > disabling ExitBootServices() call. And without any extra
> > platform-specific command line arguments. And I think it also shouldn't break
> > kexec, since it uses 1:1-like map, but I haven't tried. One should
> > simply ignore EFI_UNSUPPORTED return code (I don't know how to avoid the
> > call at all after kexec).
> 
> That's the point - it can't be avoided, and hence it failing is not
> an option. Or else there needs to be a protocol telling kexec what
> it is to expect, and that it in particular can't change the virtual
> address map to its liking. Back at the time when I put together the
> EFI booting code, no such protocol existed, and hence calling
> SetVirtualAddressMap() was not an option. I have no idea whether
> things have changed in the meantime.

Hmm, how is it different from the current situation? Not calling
SetVirtualAddressMap() means UEFI will not be notified about new address
map. It does _not_ mean it will use 1:1 map, it will use what was
previously set. What if Xen was kexec'ed from Linux?
In Linux case, it looks like it passes around the EFI memory map using
some Linux-specific mechanism, but I don't find it particularly
appealing option.

What about something in between: make this SetVirtualAddressMap() call
compile-time option (kconfig), depending on !CONFIG_KEXEC ? And when
enabled, properly handle SetVirtualAddressMap() failure. I my case,
where I do care about supporting various UEFI implementations, I don't
need kexec support. And apparently people carrying about kexec don't
have problems with lack of SetVirtualAddressMap(), so that would be
win-win, no?

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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