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

[Dave Young]

On 01/18/17 at 07:06pm, Dave Young wrote:
> On 01/18/17 at 07:01pm, Dave Young wrote:
> > On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> > > On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> > > 
> > > > On 16 January 2017 at 02:45, Dave Young <dyoung@xxxxxxxxxx> wrote:
> > > >> efi_mem_reserve cares only about boot services regions, for making sure
> > > >> later efi_free_boot_services does not free areas which are still useful,
> > > >> such as bgrt image buffer.
> > > >>
> > > >> So add a new argument to efi_memmap_insert for this purpose.
> > > >>
> > > >
> > > > So what happens is we try to efi_mem_reserve() a regions that is not
> > > > bootservices code or data?
> > > > We shouldn't simply ignore it, because it is a serious condition.
> > > 
> > > The efi_mem_desc_lookup() call in efi_arch_mem_reserve() wouldn't return
> > > anything and the latter would
> > > 
> > >   pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
> > > 
> > > then.
> > > 
> > > This is so because efi_mem_desc_lookup() searches only for regions that
> > > either
> > > - are of type EFI_BOOT_SERVICES_DATA or EFI_RUNTIME_SERVICES_DATA
> > > - or which have EFI_MEMORY_RUNTIME set already:
> > > 
> > > 	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> > > 	    md->type != EFI_BOOT_SERVICES_DATA &&
> > > 	    md->type != EFI_RUNTIME_SERVICES_DATA) {
> > > 		continue;
> > > 	}
> > > 
> > > For EFI_RUNTIME_SERVICES_DATA and EFI_MEMORY_RUNTIME,
> > > efi_arch_mem_reserve() would be a nop.
> > > 
> > > So we're fine here? Do you want to have a more descriptive error message
> > > than "Failed to lookup EFI memory descriptor"?
> > > 
> > > 
> > > For the other checks you suggested in that other thread, i.e. for the
> > > post-slab_is_available() condition and so: let me wait until Dave's
> > > series has stabilized (or even picked) and I'll submit patches for
> > > what remains to be sanity checked then.
> > > 
> > > Also, since Dave eliminated the need for late efi_mem_reserve()'s,
> > > my 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after
> > > mm_init()") should certainly get reverted at some point.
> > 
> > While testing my patches with latest edk2, I found another thing to be
> > fixed, I will repost bgrt patch according to Ard's comment tomorrow,
> > maybe with below patch as another fix to the memblock_alloc late
> > callback.

Please ignore the comment, your patch already addressed this, that
means it is still necessary after bgrt being moved early because
efi_free_boot_services still need it.

Apologize for the noise..
> > 
> > ---
> >  arch/x86/platform/efi/quirks.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-x86.orig/arch/x86/platform/efi/quirks.c
> > +++ linux-x86/arch/x86/platform/efi/quirks.c
> > @@ -355,7 +355,7 @@ void __init efi_free_boot_services(void)
> >  	}
> >  
> >  	new_size = efi.memmap.desc_size * num_entries;
> > -	new_phys = kzalloc(new_size, GFP_KERNEL);
> 
> Oops, it was memblock_alloc(), this is a middle test code used for
> debugging. Just ignore it.
> 
> > +	new_phys = efi_memmap_alloc(num_entries);
> 
> Maybe a efi_memmap_late_alloc is enough, will see if I can get a better
> version, maybe your previous patch can be dropped partially or just
> kept.
> 
> >  	if (!new_phys) {
> >  		pr_err("Failed to allocate new EFI memmap\n");
> >  		return;
> > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > Nicolai