Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Ingo Molnar <mingo@elte.hu>]

* Huang, Ying <ying.huang@intel.com> wrote:

> On Fri, 2008-01-25 at 10:16 +0100, Ingo Molnar wrote:
> > * Huang, Ying <ying.huang@intel.com> wrote:
> > 
> > > This patch fixes some bugs of making EFI runtime code executable.
> > > 
> > > - Use change_page_attr in i386 too. Because the runtime code may be
> > >   mapped not through ioremap.
> > > 
> > > - If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
> > >   is not called.
> > > 
> > > - Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
> > >   code may be mapped through efi_ioremap.
> > 
> > thanks, applied.
> > 
> > note that here:
> > 
> > > -		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > > -				   offset);
> > > +		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > > +			     offset, PAGE_KERNEL_EXEC);
> > 
> > you've changed it from nocache-noexec to cached-exec. I suspect 
> > that's what we want - except if an early EFI area can be 
> > non-prefetchable device memory. Can that ever happen? Would you like 
> > to have PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that 
> > yesterday but did not commit it yet. (see the patch below)
> 
> Yes. EFI area can be non-prefetchable device memory. I should use 
> PAGE_KERNEL_NOCACHE_EXEC.

ok, i fixed that in the patch and reordered the PAGE_KERNEL_NOCACHE_EXEC 
patch to come before yours.

> A question about this:
> 
> The MTRR on x86 should have set the memory area as un-cachable. Why do 
> we bother to set it in page table?

you are right in that there is no immediate correctness reason for it. 
The reason is to eventually get away from any MTRR dependencies. If some 
other OS uses PATs and the BIOS sets up the wrong MTRR, then the other 
OS might still having a working EFI, while Linux might crash and burn. 

So we try to get both the MTRRs and the pagetable attributes match the 
purpose of the area in question. If _both_ levels of attributes agree it 
cannot hurt, but if one of them is wrong, the other one still saves the 
day.

that's why we changed all ioremaps to default to cache-disabled (PCD) in 
latest x86.git as well. For years the x86 architecture set the ioremap 
pagetable entries to cacheable by default and only the MTRRs (and the 
BIOS writers) saved us from trouble. Now we try to be a bit more 
defensive and avoid "BIOS bug causes only Linux to crash and burn while 
other OSs work fine" type of scenarios.

	Ingo