[PATCH] Map in physical addresses in efi_map_region_fixed

[PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

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.

The added call is essentially the kexec analog of the first __map_region
that Boris put in efi_map_region in commit d2f7cbe7b26a ("x86/efi:
Runtime services virtual mapping").

Signed-off-by: Alex Thorlton <athorlton@sgi.com>
Cc: Russ Anderson <rja@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Mike Travis <travis@sgi.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-efi@vger.kernel.org
---
 arch/x86/platform/efi/efi_64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 459bcbb..b206126 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -363,6 +363,7 @@ void __init efi_map_region(efi_memory_desc_t *md)
  */
 void __init efi_map_region_fixed(efi_memory_desc_t *md)
 {
+	__map_region(md, md->phys_addr);
 	__map_region(md, md->virt_addr);
 }
 
-- 
1.8.5.6

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

On Fri, Aug 05, 2016 at 06:59:35PM -0500, 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.
> 
> The added call is essentially the kexec analog of the first __map_region
> that Boris put in efi_map_region in commit d2f7cbe7b26a ("x86/efi:
> Runtime services virtual mapping").

Just wanted to ping and see if anybody has had a chance to look at this?
Also, linux-kernel looks broken today (latest e-mails on lkml look to be
from last night?) so I wanted to send something to see if it goes
through :)

- Alex

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Matt Fleming]

(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.
> 
> The added call is essentially the kexec analog of the first __map_region
> that Boris put in efi_map_region in commit d2f7cbe7b26a ("x86/efi:
> Runtime services virtual mapping").
> 
> Signed-off-by: Alex Thorlton <athorlton@sgi.com>
> Cc: Russ Anderson <rja@sgi.com>
> Cc: Dimitri Sivanich <sivanich@sgi.com>
> Cc: Mike Travis <travis@sgi.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Cc: linux-efi@vger.kernel.org
> ---
>  arch/x86/platform/efi/efi_64.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 459bcbb..b206126 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -363,6 +363,7 @@ void __init efi_map_region(efi_memory_desc_t *md)
>   */
>  void __init efi_map_region_fixed(efi_memory_desc_t *md)
>  {
> +	__map_region(md, md->phys_addr);
>  	__map_region(md, md->virt_addr);
>  }
>  

This looks fine to me. I'm going to run it through my tests and unless
anyone complains, apply it for v4.9.

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Matt Fleming]

(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.
> 
> The added call is essentially the kexec analog of the first __map_region
> that Boris put in efi_map_region in commit d2f7cbe7b26a ("x86/efi:
> Runtime services virtual mapping").
> 
> Signed-off-by: Alex Thorlton <athorlton-sJ/iWh9BUns@public.gmane.org>
> Cc: Russ Anderson <rja-sJ/iWh9BUns@public.gmane.org>
> Cc: Dimitri Sivanich <sivanich-sJ/iWh9BUns@public.gmane.org>
> Cc: Mike Travis <travis-sJ/iWh9BUns@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> Cc: x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/x86/platform/efi/efi_64.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 459bcbb..b206126 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -363,6 +363,7 @@ void __init efi_map_region(efi_memory_desc_t *md)
>   */
>  void __init efi_map_region_fixed(efi_memory_desc_t *md)
>  {
> +	__map_region(md, md->phys_addr);
>  	__map_region(md, md->virt_addr);
>  }
>  

This looks fine to me. I'm going to run it through my tests and unless
anyone complains, apply it for v4.9.

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

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?!

What if the physical address is occupied by the kexec kernel?

Why do you guys need the physical mapping all of a sudden?

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?

commit d2f7cbe7b26a74dbbbf8f325b2a6fd01bc34032c
Author: Borislav Petkov <bp@suse.de>
Date:   Thu Oct 31 17:25:08 2013 +0100

    x86/efi: Runtime services virtual mapping
    
    We map the EFI regions needed for runtime services non-contiguously,
    with preserved alignment on virtual addresses starting from -4G down
    for a total max space of 64G. This way, we provide for stable runtime
    services addresses across kernels so that a kexec'd kernel can still use
    them.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

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?!

What if the physical address is occupied by the kexec kernel?

Why do you guys need the physical mapping all of a sudden?

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?

commit d2f7cbe7b26a74dbbbf8f325b2a6fd01bc34032c
Author: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
Date:   Thu Oct 31 17:25:08 2013 +0100

    x86/efi: Runtime services virtual mapping
    
    We map the EFI regions needed for runtime services non-contiguously,
    with preserved alignment on virtual addresses starting from -4G down
    for a total max space of 64G. This way, we provide for stable runtime
    services addresses across kernels so that a kexec'd kernel can still use
    them.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

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

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@sgi.com> 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.

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-sJ/iWh9BUns@public.gmane.org> 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.

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

On Mon, Aug 15, 2016 at 02:52:22PM -0700, H. Peter Anvin wrote:
> 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.

You mean SetVirtualAddressMap.

Oh well, it's not like it matters all that much as we have our own
pagetable for EFI so we can go nuts there. Apparently.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

On Mon, Aug 15, 2016 at 02:52:22PM -0700, H. Peter Anvin wrote:
> 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.

You mean SetVirtualAddressMap.

Oh well, it's not like it matters all that much as we have our own
pagetable for EFI so we can go nuts there. Apparently.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

On Mon, Aug 15, 2016 at 01:47:31PM -0500, Alex Thorlton wrote:
> The only thing we're adding here is the physical mappings, to match
> what is availble in the primary kernel.

I can see what it does - I just am questioning the reasoning for as we
did all that effort so that kexec can have stable virtual mappings.

I guess we still need a way to pass the virtual mappings to kexec
as they're immutable as some "smartass" decided to allow to call
SetVirtualAddressMap only once.

> This is sort of a hand-wavey answer - I will investigate the his further...

Yeah, it'll be interesting to know whether that is an issue because if
we do the 1:1 mappings in the kexec kernel too and there's an address
conflict, then we better know upfront.

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

Well, if it starts to cause trouble, you probably will have to revert.

> If there are strong objections to this change, I won't pursue it
> further.

I don't really care all that much as long as it doesn't break the
existing situation. I've long given up on the hope that EFI and all its
incarnations will hold on to some spec... :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

On Mon, Aug 15, 2016 at 01:47:31PM -0500, Alex Thorlton wrote:
> The only thing we're adding here is the physical mappings, to match
> what is availble in the primary kernel.

I can see what it does - I just am questioning the reasoning for as we
did all that effort so that kexec can have stable virtual mappings.

I guess we still need a way to pass the virtual mappings to kexec
as they're immutable as some "smartass" decided to allow to call
SetVirtualAddressMap only once.

> This is sort of a hand-wavey answer - I will investigate the his further...

Yeah, it'll be interesting to know whether that is an issue because if
we do the 1:1 mappings in the kexec kernel too and there's an address
conflict, then we better know upfront.

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

Well, if it starts to cause trouble, you probably will have to revert.

> If there are strong objections to this change, I won't pursue it
> further.

I don't really care all that much as long as it doesn't break the
existing situation. I've long given up on the hope that EFI and all its
incarnations will hold on to some spec... :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Matt Fleming]

On Mon, 15 Aug, at 05:07:09PM, 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?!
> 
> What if the physical address is occupied by the kexec kernel?
 
That's impossible, because that would mean we loaded the kexec kernel
over the top of physical pages of EFI services. We still need to be
able to invoke EFI services from kexec - we just can't change their
virtual mappings.

Unless I've misunderstood your question.

> Why do you guys need the physical mapping all of a sudden?
 
This is because of the way that the SGI firmware works and that the
"new" virtual address mappings are usable on SGI+kexec now.

> 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.
 
No, some Apple platforms will not boot if SetVirtualAddressMap()
passes 1:1 mappings, and we only enter the firmware via the 1:1
mappings for EFI mixed mode calls. We're still using the virtual
runtime mappings in the commit you reference below most of the time.
 
> 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?
> 
> commit d2f7cbe7b26a74dbbbf8f325b2a6fd01bc34032c
> Author: Borislav Petkov <bp@suse.de>
> Date:   Thu Oct 31 17:25:08 2013 +0100
> 
>     x86/efi: Runtime services virtual mapping
>     
>     We map the EFI regions needed for runtime services non-contiguously,
>     with preserved alignment on virtual addresses starting from -4G down
>     for a total max space of 64G. This way, we provide for stable runtime
>     services addresses across kernels so that a kexec'd kernel can still use
>     them.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Borislav Petkov]

On Tue, Aug 16, 2016 at 01:30:28PM +0100, Matt Fleming wrote:
> That's impossible, because that would mean we loaded the kexec kernel
> over the top of physical pages of EFI services. We still need to be
> able to invoke EFI services from kexec - we just can't change their
> virtual mappings.

Which would mean that whatever comes second - kexec reservation or 1:1
mappings - won't work.

> This is because of the way that the SGI firmware works and that the
> "new" virtual address mappings are usable on SGI+kexec now.

My only issue is that we probably should've aimed for avoiding that...

> No, some Apple platforms will not boot if SetVirtualAddressMap()
> passes 1:1 mappings, and we only enter the firmware via the 1:1
> mappings for EFI mixed mode calls. We're still using the virtual
> runtime mappings in the commit you reference below most of the time.

... but reasons like the above are making us run the kexec kernel with
the exact-same mappings as the boot kernel. Probably not really a big
deal since we have our own pagetable...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

On Tue, Aug 16, 2016 at 07:50:10AM +0200, Borislav Petkov wrote:
> On Mon, Aug 15, 2016 at 01:47:31PM -0500, Alex Thorlton wrote:
> > The only thing we're adding here is the physical mappings, to match
> > what is availble in the primary kernel.
> 
> I can see what it does - I just am questioning the reasoning for as we
> did all that effort so that kexec can have stable virtual mappings.
> 
> I guess we still need a way to pass the virtual mappings to kexec
> as they're immutable as some "smartass" decided to allow to call
> SetVirtualAddressMap only once.
> 
> > This is sort of a hand-wavey answer - I will investigate the his further...
> 
> Yeah, it'll be interesting to know whether that is an issue because if
> we do the 1:1 mappings in the kexec kernel too and there's an address
> conflict, then we better know upfront.

I did some investigation/testing here and found, as Matt stated, that
this shouldn't cause any serious problems.  The crashkernel memory
reservation happens a little later than I originally thought, so we sort
of hit a hybrid of the two problems I described.  i.e, the memory isn't
mapped yet, but the kernel does know not to map anything over it.

I purposely set the crashkernel to land right on top of some of our
MMRs:

#define UV2_GLOBAL_MMR32_BASE           0xfc000000UL

This is 4227858432 in base10, or 4128768K in crashkernel parameter
terms, so I used this on the command line:

crashkernel=256M@4128768K

And, not too surprisingly, I hit this while trying to reserve the
crashkernel memory:

[    0.000000] crashkernel reservation failed - memory is in use.

At a glance, I believe that this is because the memory range for these
MMRs is E820_RESERVED:

[    0.000000] BIOS-e820: [mem 0x00000000f8000000-0x00000000fdffffff] reserved

So, it gets skipped over during memblock_x86_fill, which only picks up
E820_RAM, and E820_RESERVED_KERN memory.  At least in this case, the
memory range where our MMRs fall is seen as completely unavailable (or,
more accurately, not seen at all) when we do the memblock_find_in_range
during reserve_crashkernel.

Note that I chose to map over some MMR space instead of EFI runtime
code.  This was an arbitrary choice - they both show up as
E820_RESERVED, so the behavior will be the same.

This is certainly not news to some, but I wasn't positive about how this
worked, and wanted to make sure that I addressed the concern that you
expressed.

Thanks again for your input!

- Alex

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

On Tue, Aug 16, 2016 at 07:50:10AM +0200, Borislav Petkov wrote:
> On Mon, Aug 15, 2016 at 01:47:31PM -0500, Alex Thorlton wrote:
> > The only thing we're adding here is the physical mappings, to match
> > what is availble in the primary kernel.
> 
> I can see what it does - I just am questioning the reasoning for as we
> did all that effort so that kexec can have stable virtual mappings.
> 
> I guess we still need a way to pass the virtual mappings to kexec
> as they're immutable as some "smartass" decided to allow to call
> SetVirtualAddressMap only once.
> 
> > This is sort of a hand-wavey answer - I will investigate the his further...
> 
> Yeah, it'll be interesting to know whether that is an issue because if
> we do the 1:1 mappings in the kexec kernel too and there's an address
> conflict, then we better know upfront.

I did some investigation/testing here and found, as Matt stated, that
this shouldn't cause any serious problems.  The crashkernel memory
reservation happens a little later than I originally thought, so we sort
of hit a hybrid of the two problems I described.  i.e, the memory isn't
mapped yet, but the kernel does know not to map anything over it.

I purposely set the crashkernel to land right on top of some of our
MMRs:

#define UV2_GLOBAL_MMR32_BASE           0xfc000000UL

This is 4227858432 in base10, or 4128768K in crashkernel parameter
terms, so I used this on the command line:

crashkernel=256M@4128768K

And, not too surprisingly, I hit this while trying to reserve the
crashkernel memory:

[    0.000000] crashkernel reservation failed - memory is in use.

At a glance, I believe that this is because the memory range for these
MMRs is E820_RESERVED:

[    0.000000] BIOS-e820: [mem 0x00000000f8000000-0x00000000fdffffff] reserved

So, it gets skipped over during memblock_x86_fill, which only picks up
E820_RAM, and E820_RESERVED_KERN memory.  At least in this case, the
memory range where our MMRs fall is seen as completely unavailable (or,
more accurately, not seen at all) when we do the memblock_find_in_range
during reserve_crashkernel.

Note that I chose to map over some MMR space instead of EFI runtime
code.  This was an arbitrary choice - they both show up as
E820_RESERVED, so the behavior will be the same.

This is certainly not news to some, but I wasn't positive about how this
worked, and wanted to make sure that I addressed the concern that you
expressed.

Thanks again for your input!

- Alex

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Dave Young]

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

In efi_map_region, there is already mapped md->phys_addr for broken
firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
of kexec kernel.

void __init efi_map_region(efi_memory_desc_t *md)
{
        unsigned long size = md->num_pages << PAGE_SHIFT;
        u64 pa = md->phys_addr;

        if (efi_enabled(EFI_OLD_MEMMAP))
                return old_map_region(md);

        /*
         * Make sure the 1:1 mappings are present as a catch-all for
         * b0rked
         * firmware which doesn't update all internal pointers after
         * switching
         * to virtual mode and would otherwise crap on us.
         */
        __map_region(md, md->phys_addr);

[snip]

Thanks
Dave

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Dave Young]

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

In efi_map_region, there is already mapped md->phys_addr for broken
firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
of kexec kernel.

void __init efi_map_region(efi_memory_desc_t *md)
{
        unsigned long size = md->num_pages << PAGE_SHIFT;
        u64 pa = md->phys_addr;

        if (efi_enabled(EFI_OLD_MEMMAP))
                return old_map_region(md);

        /*
         * Make sure the 1:1 mappings are present as a catch-all for
         * b0rked
         * firmware which doesn't update all internal pointers after
         * switching
         * to virtual mode and would otherwise crap on us.
         */
        __map_region(md, md->phys_addr);

[snip]

Thanks
Dave

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

On Wed, Aug 17, 2016 at 03:01:51PM +0800, Dave Young wrote:
> > > 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.
> 
> In efi_map_region, there is already mapped md->phys_addr for broken
> firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
> of kexec kernel.

We're actually in the middle of trying to move *away* from
EFI_OLD_MEMMAP, which is why we're just starting to uncover some of
these things.  efi_map_region covers us on the primary kernel, because
it maps in the physical address of each md (as you note here), but that
little piece is missing in the kexec'd kernel.  So, our primary kernel
works without efi=old_map, but the second kernel won't, without this
change (supplying "noefi" on the kexec command line also works, but then
we don't have any of our runtime stuff available).

As noted in a previous message, we're aware that our code needs a little
more work to be "perfect," but this small change buys us most of (all
of?) the stuff we'd get by implementing the other changes that we're
aware we need to make, i.e. update our runtime function pointer to its
efi_va during SetVirtualAddressMap, at least from a kexec perspective.

- Alex

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Alex Thorlton]

On Wed, Aug 17, 2016 at 03:01:51PM +0800, Dave Young wrote:
> > > 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.
> 
> In efi_map_region, there is already mapped md->phys_addr for broken
> firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
> of kexec kernel.

We're actually in the middle of trying to move *away* from
EFI_OLD_MEMMAP, which is why we're just starting to uncover some of
these things.  efi_map_region covers us on the primary kernel, because
it maps in the physical address of each md (as you note here), but that
little piece is missing in the kexec'd kernel.  So, our primary kernel
works without efi=old_map, but the second kernel won't, without this
change (supplying "noefi" on the kexec command line also works, but then
we don't have any of our runtime stuff available).

As noted in a previous message, we're aware that our code needs a little
more work to be "perfect," but this small change buys us most of (all
of?) the stuff we'd get by implementing the other changes that we're
aware we need to make, i.e. update our runtime function pointer to its
efi_va during SetVirtualAddressMap, at least from a kexec perspective.

- Alex

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Dave Young]

On 08/17/16 at 11:00am, Alex Thorlton wrote:
> On Wed, Aug 17, 2016 at 03:01:51PM +0800, Dave Young wrote:
> > > > 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.
> > 
> > In efi_map_region, there is already mapped md->phys_addr for broken
> > firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
> > of kexec kernel.
> 
> We're actually in the middle of trying to move *away* from
> EFI_OLD_MEMMAP, which is why we're just starting to uncover some of
> these things.  efi_map_region covers us on the primary kernel, because
> it maps in the physical address of each md (as you note here), but that
> little piece is missing in the kexec'd kernel.  So, our primary kernel
> works without efi=old_map, but the second kernel won't, without this
> change (supplying "noefi" on the kexec command line also works, but then
> we don't have any of our runtime stuff available).
> 
> As noted in a previous message, we're aware that our code needs a little
> more work to be "perfect," but this small change buys us most of (all
> of?) the stuff we'd get by implementing the other changes that we're
> aware we need to make, i.e. update our runtime function pointer to its
> efi_va during SetVirtualAddressMap, at least from a kexec perspective.

Thanks for explanation, I still do not get why the original ioremap way
works if SetVirtualAddressMapdo does not update runtime function
pointer.

But if it fixes the problem in primary kernel, it should be fine to do
same in kexec kernel. 

Thanks
Dave

Re: [PATCH] Map in physical addresses in efi_map_region_fixed

[Dave Young]

On 08/17/16 at 11:00am, Alex Thorlton wrote:
> On Wed, Aug 17, 2016 at 03:01:51PM +0800, Dave Young wrote:
> > > > 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.
> > 
> > In efi_map_region, there is already mapped md->phys_addr for broken
> > firmware. SGI still need EFI_OLD_MEMMAP? I means in 1st kernel instead
> > of kexec kernel.
> 
> We're actually in the middle of trying to move *away* from
> EFI_OLD_MEMMAP, which is why we're just starting to uncover some of
> these things.  efi_map_region covers us on the primary kernel, because
> it maps in the physical address of each md (as you note here), but that
> little piece is missing in the kexec'd kernel.  So, our primary kernel
> works without efi=old_map, but the second kernel won't, without this
> change (supplying "noefi" on the kexec command line also works, but then
> we don't have any of our runtime stuff available).
> 
> As noted in a previous message, we're aware that our code needs a little
> more work to be "perfect," but this small change buys us most of (all
> of?) the stuff we'd get by implementing the other changes that we're
> aware we need to make, i.e. update our runtime function pointer to its
> efi_va during SetVirtualAddressMap, at least from a kexec perspective.

Thanks for explanation, I still do not get why the original ioremap way
works if SetVirtualAddressMapdo does not update runtime function
pointer.

But if it fixes the problem in primary kernel, it should be fine to do
same in kexec kernel. 

Thanks
Dave