Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[H. Peter Anvin]

On August 15, 2016 11:47:31 AM PDT, Alex Thorlton <athorlton@xxxxxxx> wrote:
>On Mon, Aug 15, 2016 at 05:07:09PM +0200, Borislav Petkov wrote:
>> On Mon, Aug 15, 2016 at 01:42:58PM +0100, Matt Fleming wrote:
>> > (Cc'ing Boris and Dave)
>> > 
>> > On Fri, 05 Aug, at 06:59:35PM, Alex Thorlton wrote:
>> > > This is a simple change to add in the physical mappings as well
>as the
>> > > virtual mappings in efi_map_region_fixed.  The motivation here is
>to
>> > > get access to EFI runtime code that is only available via the 1:1
>> > > mappings on a kexec'd kernel.
>> 
>> So I don't understand: the whole jumping through hoops so that we
>have
>> stable virtual mappings just so that the kexec-ed kernel can call EFI
>> runtime, is now useless?!
>
>Well, anyone who is using the mappings in -4G down range still needs
>your code to map their stuff into the appropriate spot in the kexec'd
>kernel, so anybody who is doing things in the most up-to-date manner
>still makes use of your new code, right?
>
>AFAIK, we pass up the efi_runtime_map to the kexec'd kernel, and then
>process the memory descriptors one by one, mapping in their virtual
>addresses during kexec_enter_virtual_mode.  All of this relies on your
>efforts to pass up the virtual mappings.  The only thing we're adding
>here is the physical mappings, to match what is availble in the primary
>kernel.
>
>> What if the physical address is occupied by the kexec kernel?
>
>Ah, that's a good question.  I usually don't force the placement of my
>crashkernel, so I can't exactly comment intelligently on what will
>happen in that situation.  I will look into this, but in our case, I
>believe that the primary kernel would either fail to boot, or fail to
>load the kexec kernel if we placed it over the range where are EFI code
>gets mapped in, which I think is an issue even without this patch.
>
>IIRC, the crashkernel memory gets reserved really early, so I'd imagine
>we'd hit the former problem, since we will try to remap into that same
>space in uv_bios_init.
>
>This is sort of a hand-wavey answer - I will investigate the his
>further to
>make sure that I'm not making any stupid assumptions (there's a good
>chance that I am :)
>
>> Why do you guys need the physical mapping all of a sudden?
>
>It's not that we need it all of the sudden, necessarily, it's just that
>we've had to make other changes to make things work with the new,
>(almost) completely isolated, EFI page tables.  We ended up choosing
>the
>lesser of two evils, and have decided to temporarily rely on the
>physical address of our runtime code, instead of continuing to rely on
>EFI_OLD_MEMMAP.
>
>I guess what I'm saying is, if we hadn't been relying on some
>semi-undefined behavior in the EFI memory mapping scheme, we would've
>been relying on the physical address for quite a while, since nobody
>would have been mapping in the virtual address for us.
>
>It's important to note that we've been dancing back and forth between
>workarounds for some slightly inorrect assumptions, and some actual bug
>fixes.  We're working to get everything 100% compatible with the new
>memory mapping schemes, but there're a few pieces of the puzzle we
>haven't gotten around to yet.
>
>> Your patch is basically rendering all the effort moot and we could've
>> saved ourselves all that trouble of doing all that virtual address
>> mapping and done the 1:1 thing.
>> 
>> Which really is probably simpler since we have an EFI-specific page
>> table and running EFI in the kexec-ed kernel would mean basically
>> recreating it.
>> 
>> What am I missing?
>
>I don't think it renders all of your effort worthless.  It just allows
>those who haven't had a chance to completely update their code to work
>with the new mapping schemes a way to utilize EFI runtime callbacks, in
>a kexec'd kernel.
>
>I do understand that, in a perfect world, we would be able to just hop
>right in and use your memory mapping scheme.  At the same time, I don't
>see how this is that much different from what we already do in the
>primary, non-kexec'd kernel.
>
>If there are strong objections to this change, I won't pursue it
>further.  We will be able to achieve the same effect once we've had a
>chance to update our code to register a callback with
>SetVirtualAddressMap to fix up our function pointer.  This is on my
>upcoming to-do list, but it'll be a little bit before I have a chance
>to
>get it finished.
>
>Thanks for the input, Boris!
>
>- Alex

So to answer the implicit question: we have found UEFI stacks in the field which fail without the physical mappings present, and we have found stacks which fail without a nontrivial SetAddressMapping.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.