Re: [PATCH 1/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

[Dave Young]

On 01/12/17 at 12:15pm, Nicolai Stange wrote:
> Hi Dave,
> 
> On Thu, Jan 12 2017, Dave Young wrote:
> 
> > efi_mem_reserve cares only about boot services regions and maybe loader areas.
> > So add a new argument to efi_memmap_insert for this purpose.
> 
> Please see below.
> 
> 
> > --- linux-x86.orig/drivers/firmware/efi/memmap.c
> > +++ linux-x86/drivers/firmware/efi/memmap.c
> > @@ -213,7 +213,7 @@ int __init efi_memmap_split_count(efi_me
> >   * to see how large @buf needs to be.
> >   */
> >  void __init efi_memmap_insert(struct efi_memory_map *old_memmap, void *buf,
> > -			      struct efi_mem_range *mem)
> > +			      struct efi_mem_range *mem, bool boot_only)
> >  {
> >  	u64 m_start, m_end, m_attr;
> >  	efi_memory_desc_t *md;
> > @@ -246,6 +246,12 @@ void __init efi_memmap_insert(struct efi
> >  		start = md->phys_addr;
> >  		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1;
> >  
> > +		if (boot_only && !(md->type == EFI_LOADER_DATA ||
> > +		    md->type == EFI_LOADER_CODE ||
> > +		    md->type == EFI_BOOT_SERVICES_CODE ||
> > +		    md->type == EFI_BOOT_SERVICES_DATA))
> > +			continue;
> > +
> 
> 
> Actually, the efi_mem_desc_lookup() called from
> efi_arch_memmap_reserve() will only return mds not satisfying the
> following condition:
> 
>   if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
>       md->type != EFI_BOOT_SERVICES_DATA &&
>       md->type != EFI_RUNTIME_SERVICES_DATA) {
>   	continue;
>   }
> 
> Furthermore, efi_arch_mem_reserve() will only accept ranges fully
> contained within such a region.
> 
> I think we can make efi_arch_mem_reserve() return early if
> EFI_MEMORY_RUNTIME has been set already and thus, neglect this case in
> efi_memmap_insert().
> 
> I suppose that we don't want to reserve within EFI_RUNTIME_SERVICES_DATA
> regions in efi_mem_reserve() either -- these won't ever get made
> available as general memory anyway [1]. So efi_arch_mem_reserve() should
> return early here as well imo.
> 
> So, what would remain to be handled from efi_memmap_insert() in case of
> boot_only would be EFI_BOOT_SERVICES_DATA only?

It sounds reasonable though I'm still not sure about EFI_LOADER*.

The main purpose of this patch is to address the invalid mem ranges
case. As Ard mentioned I will test with Peter's patch first, if it works
fine I would like to either drop this patch as a future improvement or add
it at the end of the next post.

Matt, what's your opinion about the boot_only check and the EFI_LOADERS*
question?

> 
> (As a sidenote, Matt pointed out at [1] that the EFI_LOADER_* regions
>  should be reserved early through memblock_reserve() and not through
>  efi_mem_reserve()).
> 
> Thanks,
> 
> Nicolai
> 
> 
> [1] http://lkml.kernel.org/r/20170109130702.GI16838@xxxxxxxxxxxxxxxxxxx

Thanks
Dave