Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 24 Feb 2016 08:39:56 -0800 | Subject | 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
| |