Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340438 > unrolled thread

[tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings tip-bot for Sai Praneeth <tipbot@zytor.com>@zytor.com - 2016-02-23 10:20 +0100
    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-23 18:50 +0100
      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-23 19:10 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-23 19:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 03:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings "H. Peter Anvin" <hpa@zytor.com> - 2016-02-24 03:20 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Ingo Molnar <mingo@kernel.org> - 2016-02-25 10:00 +0100
      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> - 2016-02-24 02:00 +0100
        Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 03:50 +0100
          Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 15:20 +0100
            Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-24 17:30 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:40 +0100
                Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-24 20:00 +0100
                  Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 20:50 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 21:00 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Sylvain Chouleur <sylvain.chouleur@gmail.com> - 2016-02-29 12:00 +0100
                      Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-03-02 12:30 +0100
                Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-24 20:40 +0100
                  Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 21:00 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Ingo Molnar <mingo@kernel.org> - 2016-02-25 10:10 +0100
                    Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-25 16:30 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:50 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Borislav Petkov <bp@alien8.de> - 2016-02-24 17:50 +0100
            Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Andy Lutomirski <luto@amacapital.net> - 2016-02-24 17:50 +0100
              Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page  table mappings Matt Fleming <matt@codeblueprint.co.uk> - 2016-02-25 17:10 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1343270 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, 24 Feb, at 11:49:23AM, Andy Lutomirski wrote:
> On Wed, Feb 24, 2016 at 11:33 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Wed, 24 Feb, at 08:36:33AM, Andy Lutomirski wrote:
> >> On Wed, Feb 24, 2016 at 8:20 AM, Borislav Petkov <bp@alien8.de> wrote:
> >> > On Wed, Feb 24, 2016 at 02:10:46PM +0000, Matt Fleming wrote:
> >> >> > Normally, the only pages with are _PAGE_GLOBAL are those that are in
> >> >> > the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
> >> >> > By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
> >> >> > that convention, which forces you to use extra-expensive
> >> >> > __flush_tlb_all calls in efi_call_virt.
> >> >
> >> > Hold on, do you mean the __flush_tlb_all() in the CONFIG_EFI_MIXED code?
> >> >
> >> > That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
> >> > That's EFI on 64-bit but that is mandated by the spec, AFAIR.
> >>
> >> I mean the one in efi_call_virt.  Why would the spec mandate a TLB
> >> flush at all?  EFI runtime services have no business touching the
> >> paging structures directly.  Heck, the 32-bit ones don't even know the
> >> *format* of the paging structures.
> >
> > Right, and it would necessitate copying out arguments because the
> > firmware won't understand where/how the kernel has mapped things.
> >
> > No firmware is going to be doing that.
> 
> Just so I understand correctly: could we get away with putting the EFI
> virtual runtime mappings at positive (user) addresses for 64-bit UEFI,
> or is there some reason that we need the high bit set?
 
Good question. There are multiple parts to this answer:

 1) Some firmware is known to break when entered via the identity
    addresses (VA==PA) 

 2) Kexec cares the most about where we map things because the region
    has to be static across Kexec reboots. We do pass the kernel's EFI
    memory map regions between Kexec kernels but that region clearly
    needs to be available across kernel versions

It shouldn't be possible to conflict with userspace mappings or
anything like that because we should never be accessing userspace
addresses during EFI runtime services calls - all relevant data is
copied to a kernel buffer or such. Userspace isn't even mapped now
we've got completely separate EFI page tables.

I don't think there's anything else that would stop us clearing the
high bit and moving the EFI virtual mapping region somewhere else.
Boris?

> If we could use positive addresses, then we could use the existing
> use_mm infrastructure directly with no funny business at all except to
> the extent that we might need to use unusual APIs to set up the VMAs
> (if we use real VMAs) in the first place.  (We could cheat and
> allocate a single monstrous VM_MIXEDMAP or VM_PFNMAP vma with a .fault
> handler that always fails.)  If we have to use negative addresses,
> then we'll always be stuck with a funny pgd, but we could still
> probably use use_mm instead of manually fiddling with cr3.
 
We don't use VMAs at the moment.

Having a custom .fault handler could be a very interesting idea
because we've talked about wanting to do EFI-specific things in the
past if we fault while executing firmware, e.g. printing warnings in
the kernel log indicating the firmware is known to be buggy because it
performed an access not compliant with the spec. See 1) above.

> Some day I want to experiment with calling runtime services at CPL 3,
> too :)  We'd want to add some infrastructure to permit kernel threads
> to run through the entry/exit code as if they were user processes, but
> there's nothing conceptually wrong with that.  We already allow kernel
> threads to call execve and "return" to real user mode, so it's not
> much of a stretch.  The main issue would be dealing with signal
> handling and such -- we'd want to report faults back to the kernel
> thread's CPL3-invocation thunks rather than delivering a signal at CPL
> 3.
 
Right, more isolation is better. I'm not sure we could get all the way
to CPL 3 but I wouldn't begrudge anyone trying.

[toc] | [prev] | [next] | [standalone]

#1342198 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 8:41 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Feb 24, 2016 at 05:20:02PM +0100, Borislav Petkov wrote:
>> That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
>> That's EFI on 64-bit but that is mandated by the spec, AFAIR.
>
> Ok, so mfleming set me straight on IRC - that's tip/master I should be
> staring at.
>
> In any case, I think we should do __flush_tlb_all() in efi_call_cirt()
> just in case, for the simple reason that EFI could be installing some
> funky TLB entries which we don't want. I'm not saying it does and it
> probably won't but what's stopping it?
>
> Or am I being overly paranoid?

I think you may be overly paranoid here.  At least no working 32-bit
EFI does this because we run it in compat mode.  Any paging entries it
inserts would be misinterpreted and likely immediately cause a crash.
Also, the EFI code doesn't know a virtual address through which to
reference the paging structures in the first place -- it could read
CR3, but that gives a physical address, and it's not at all clear to
me what even the crazier firmware authors would do with a physical
address that doesn't live in EFI-defined ranges.

--Andy

[toc] | [prev] | [next] | [standalone]

#1342205 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 05:20:02PM +0100, Borislav Petkov wrote:
> That's mixed mode. I think you mean the FLUSH_TLB_ALL in efi_call.
> That's EFI on 64-bit but that is mandated by the spec, AFAIR.

Ok, so mfleming set me straight on IRC - that's tip/master I should be
staring at.

In any case, I think we should do __flush_tlb_all() in efi_call_cirt()
just in case, for the simple reason that EFI could be installing some
funky TLB entries which we don't want. I'm not saying it does and it
probably won't but what's stopping it?

Or am I being overly paranoid?

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1342202 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, Feb 24, 2016 at 6:10 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Tue, 23 Feb, at 06:43:04PM, Andy Lutomirski wrote:
>> On Tue, Feb 23, 2016 at 4:50 PM, Sai Praneeth Prakhya
>> <sai.praneeth.prakhya@intel.com> wrote:
>> >
>> > As you rightly said the code is about making a page GLOBAL if PRESENT is
>> > set and we do set PRESENT bit before mapping so that page is GLOBAL.
>> > This code was taken from the other parts of pageattr.c. The point is
>> > that we don't want differences between whether things were mapped in the
>> > EFI page tables directly (i.e. using populate_pte()) or later split from
>> > large pages via the split_large_page() code path. If this is still
>> > confusing could you please elaborate on it further.
>>
>> At least the comment should say "Set the GLOBAL flag if and only
>> if...".  But why is this code here in the first place?  What is
>> passing a pgprot with global unset into this code in the first place?
>
> This comes from populate_pgd(),
>
>   static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
>   {
>           pgprot_t pgprot = __pgprot(_KERNPG_TABLE);

which reminds me: aren't we supposed to *not* set GLOBAL on _PAGE_TABLE entries?

>>
>> You're making a choice of whether to set _PAGE_GLOBAL, and I think
>> you've made the wrong choice.
>>
>> Normally, the only pages with are _PAGE_GLOBAL are those that are in
>> the normal kernel mappings (swapper_pg_dir and normal mm_struct pgds).
>> By allowing _PAGE_GLOBAL to be set in EFI mappings, you're breaking
>> that convention, which forces you to use extra-expensive
>> __flush_tlb_all calls in efi_call_virt.
>>
>> I think you should explicitly *clear* _PAGE_GLOBAL in the EFI mappings
>> instead.  This would allow you to use write_cr3 by itself, which would
>> make the code simpler and faster.
>
> This is interesting.
>
> When I suggested to Sai that he write this patch my main motivation
> was consistency for all mappings. We've got that now, but perhaps it's
> the wrong consistency ;)
>
> If we go with the no-PAGE_GLOBAL approach, we need changes to ensure
> we never set _PAGE_GLOBAL for the EFI mappings, because before this
> patch was applied sometimes we did and sometimes we didn't, depending
> on whether a page was split or not.
>
> I'm racking my brain to think of how your suggestion might have
> unintended consequences because diagnosing stale TLB entry bugs is
> simply the worst job ever. I can't think of anything. The only
> scenarios where we'd see problems is if a) we have new global mappings
> in the EFI page tables or b) we have different global mappings.
>
> Since we reference swapper_pg_dir from the PMD level downwards b)
> shouldn't be a problem, and the only differences between
> swapper_pg_dir and efi_pgd should be the EFI mappings, which saves us
> from a).

:)

Anyway, there's certainly no need to do this right now.

>
>> > This is a valid point. I know that EFI runtime regions persist during
>> > and after boot if we have a UEFI firmware and other commits made EFI
>> > regions have separate page table but I am not clear about the effect of
>> > global flush. I think Matt/Boris could comment on it.
>>
>> It's straightfoward on existing kernels.  If _PAGE_GLOBAL is set, TLB
>> entries persist across cr3 writes.  If _PAGE_GLOBAL is clear, then TLB
>> entries are flushed by cr3 writes.
>
> This is safe for EFI right now because of the big __flush_tlb_all() in
> efi_call_virt().
>
>> With PCID enabled (which is only in a not-quite-ready patch set I
>> have), the story is a bit more complicated, but it works essentially
>> the same way unless you explicitly opt out.
>
> Hmm... is series that posted somewhere?

It's living here:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=x86/pcid

at least until someone figures out how to squash the races in the bookkeeping.

I intentionally never load PCID == 0 with the "don't flush" bit set
specifically so that direct PCID-unaware CR3 loads (like the EFI code
does all over the place) keep working.

>
>> > We touch this code path only when mapping EFI runtime regions to VA
>> > space, i.e. we added pgd field in cpa only as a support for mapping efi
>> > runtime regions.
>>
>> populate_pgd is called from non-EFI code as well though, isn't it?
>
> Nope. The "if (cpa->pgd)" guard ensures that we only call that
> function for the EFI mapping code - no one else sets ->pgd.

OK, although a comment might be nice.

--Andy

[toc] | [prev] | [next] | [standalone]

#1343290 — Re: [tip:efi/core] x86/mm/pat: Use _PAGE_GLOBAL bit for EFI page table mappings

On Wed, 24 Feb, at 08:39:56AM, Andy Lutomirski wrote:
> 
> OK, although a comment might be nice.

Something like this?

---
From ac40fc0269d4d8cc9051982c177ee140d6e1b761 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt@codeblueprint.co.uk>
Date: Thu, 25 Feb 2016 15:54:50 +0000
Subject: [PATCH] x86/mm/pat: Document the (currently) EFI-only code path

It's not at all obvious that populate_pgd() and friends are only
executed when mapping EFI virtual memory regions or that no other
pageattr callers pass a ->pgd value.

Reported-by: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/mm/pageattr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 14c38ae80409..8fee5b6f8f66 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -1125,8 +1125,14 @@ static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
 static int __cpa_process_fault(struct cpa_data *cpa, unsigned long vaddr,
 			       int primary)
 {
-	if (cpa->pgd)
+	if (cpa->pgd) {
+		/*
+		 * Right now, we only execute this code path when mapping
+		 * the EFI virtual memory map regions, no other users
+		 * provide a ->pgd value. This may change in the future.
+		 */
 		return populate_pgd(cpa, vaddr);
+	}
 
 	/*
 	 * Ignore all non primary paths.
-- 
2.6.2

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web