Re: [PATCH 2/5] x86_64 EFI support -v3: EFI boot support

["Huang, Ying" <ying.huang@intel.com>]

On Tue, 2007-07-31 at 13:35 +0200, Andi Kleen wrote:
> >  static unsigned long dma_reserve __initdata;
> > +/* Flag indicating EFI runtime executable code area */
> > +static int efi_runtime_code_area;
> 
> We don't normally use globals to modify function behaviour.
> 
> >  
> >  DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
> >  
> > @@ -199,7 +202,7 @@
> >  static __meminit void unmap_low_page(void *adr)
> >  { 
> >  
> > -	if (after_bootmem)
> > +	if (after_bootmem || efi_runtime_code_area)
> >  		return;
> >  
> >  	early_iounmap(adr, PAGE_SIZE);
> > @@ -259,16 +262,21 @@
> >  		pmd_t *pmd = pmd_page + pmd_index(address);
> >  
> >  		if (address >= end) {
> > -			if (!after_bootmem)
> > +			if (!after_bootmem && !efi_runtime_code_area)
> 
> 
> This one seems also weird. Are you sure this doesn't remove _NX from
> more than the intended area? 
> 
> >  				for (; i < PTRS_PER_PMD; i++, pmd++)
> >  					set_pmd(pmd, __pmd(0));
> >  			break;
> >  		}
> >  
> > -		if (pmd_val(*pmd))
> > +		if (pmd_val(*pmd) && !efi_runtime_code_area)
> >  			continue;
> >  
> > -		entry = _PAGE_NX|_PAGE_PSE|_KERNPG_TABLE|_PAGE_GLOBAL|address;
> > +		if (efi_runtime_code_area) {
> > +			entry = pmd_val(*pmd);
> > +			entry &= ~_PAGE_NX;
> > +		} else
> > +			entry = _PAGE_NX | _PAGE_PSE | _KERNPG_TABLE | \
> > +				_PAGE_GLOBAL | address;
> 
> This doesn't look correct. PSE/KERNPG/GLOBAL/address surely need to be set
> for EFI areas too.
> 
> The changes to this file are quite messy.
> Perhaps it would be better if you just use change_page_attr() afterwards.
> This would make it using 4K pages instead of 2MB, but that wouldn't be a catastrophe.

The memory area mapping must be changed before time_init, where the
first EFI runtime serivce (efi_get_time) is called. But
"change_page_attr" can not be used there, because "alloc_pages" is used
by "change_page_attr".

Should I change "change_page_attr" to make it work before "mem_init"? Or
Should I change "init_memory_mapping" to make it can be used to change
mapping attributes? Which one is better?

> What exactly are you trying to do here? Just remove _NX for some areas?

Yes. I just want to remove _NX for some areas.

Best Regards,
Huang Ying