Re: [PATCH 5/6] x86/efi: Build our own page table structures

[Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>]

On Thu, 12 Nov, at 07:38:13PM, Borislav Petkov wrote:
> > @@ -831,6 +831,12 @@ static void __init __efi_enter_virtual_mode(void)
> >  	efi.systab = NULL;
> >  
> >  	efi_merge_regions();
> > +	if (efi_alloc_page_tables()) {
> > +		pr_err("Failed to allocate EFI page tables\n");
> > +		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> > +		return;
> > +	}
> 
> This should happen before efi_merge_regions() - no need to merge if we
> can't alloc PGT.
 
Fair point.

> > +
> >  	new_memmap = efi_map_regions(&count, &pg_shift);
> >  	if (!new_memmap) {
> >  		pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> > @@ -888,29 +894,12 @@ static void __init __efi_enter_virtual_mode(void)
> >  
> >  	efi_runtime_mkexec();
> >  
> > -	/*
> > -	 * We mapped the descriptor array into the EFI pagetable above but we're
> > -	 * not unmapping it here. Here's why:
> > -	 *
> > -	 * We're copying select PGDs from the kernel page table to the EFI page
> > -	 * table and when we do so and make changes to those PGDs like unmapping
> > -	 * stuff from them, those changes appear in the kernel page table and we
> > -	 * go boom.
> > -	 *
> > -	 * From setup_real_mode():
> > -	 *
> > -	 * ...
> > -	 * trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
> > -	 *
> > -	 * In this particular case, our allocation is in PGD 0 of the EFI page
> > -	 * table but we've copied that PGD from PGD[272] of the EFI page table:
> > -	 *
> > -	 *	pgd_index(__PAGE_OFFSET = 0xffff880000000000) = 272
> > -	 *
> > -	 * where the direct memory mapping in kernel space is.
> > -	 *
> > -	 * new_memmap's VA comes from that direct mapping and thus clearing it,
> > -	 * it would get cleared in the kernel page table too.
> > +        /*
> 
> ERROR: code indent should use tabs where possible
> #149: FILE: arch/x86/platform/efi/efi.c:897:
> +        /*$
> 
> ...
 
Dammit vim. I'll fix this.

> >  void efi_sync_low_kernel_mappings(void)
> >  {
> > -	unsigned num_pgds;
> > -	pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
> > +	unsigned num_entries;
> > +	pgd_t *pgd_k, *pgd_efi;
> > +	pud_t *pud_k, *pud_efi;
> >  
> >  	if (efi_enabled(EFI_OLD_MEMMAP))
> >  		return;
> >  
> > -	num_pgds = pgd_index(MODULES_END - 1) - pgd_index(PAGE_OFFSET);
> > +	/*
> > +	 * We can share all PGD entries apart from the one entry that
> > +	 * covers the EFI runtime mapping space.
> > +	 *
> > +	 * Make sure the EFI runtime region mappings are guaranteed to
> > +	 * only span a single PGD entry and that the entry also maps
> > +	 * other important kernel regions.
> > +	 */
> > +	BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> > +	BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> > +				(EFI_VA_END & PGDIR_MASK));
> 
> You can align them in a more readable way:
> 
> 	BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> 		       (EFI_VA_END & PGDIR_MASK));

Sure.