Re: efi_memmapwalk re-write

[Khalid Aziz <khalid_aziz@hp.com>]

On Wed, 2005-08-03 at 15:45 -0700, Luck, Tony wrote:
> >one on x86. This patch is relative to 2.6.13-rc3 and applies on top of 
> >the EFI memory map walk rewrite patch at 
> ><http://free.linux.hp.com/~khalid/ia64/efi_memmapwalk_2.6.13rc3.patch>
> >(Tony, this EFI memory map walk patch is same as the one I sent you this
> >morning).
> 
> Khalid,
> 
> Thanks for working on this.  I'm sorry it has taken this long to look
> at it.
> 
> I think some areas can still benefit from more simplification.  The only
> place I see you split a kern_memdesc_t is in efi_trim_memory() in order
> to limit memory according to either of mem_limit or max_addr.  Wouldn't
> it be simpler to just adjust num_pages element of the element instead
> of splitting?
> 
> If you do that ... then you don't need to have a linked list of kern_memdesc
> structures, you can treat them just like an array, nor do you need
> MEM_DESC_SAFETY_MARGIN
> 
> Likewise the granule alignment functions.  The original trim_top() and
> trim_bottom() are insanely complex ... and perhaps you were led astray
> trying to duplicate their behaivour?  I believe that you should end up
> with the desired behaivour if you just do any coalescing of memory blocks
> that are WB and have one of the allowable types, then round the base
> addresses up to granule boundaries and the tops down.  All that scanning
> around looking for holes or non-WB sections of memory looks pointless to
> me ... perhaps I'm missing some incredible subtlety in the original?
> 
> By only copying from "is_available()" types into kern_memdesc_t structures
> you can avoid calling is_available() in your new efi_memmap_walk(), and
> indeed drop the "type" field from the structure.
> 
> find_memmap_space() should be in efi.c ... just pass a pointer to space
> for it to fill in the address of the block it allocated and have it
> return the size so that reserve_memory() can fill these into an entry
> in rsvd_region[].  It shouldn't use is_available_memory() to decide
> which blocks are candidates for the allocation.  This could result in
> choosing memory that is still in use by EFI, command line arguments, or
> even overwriting the kernel.  EFI_CONVENTIONAL_MEMORY is definitely
> safe for this, perhaps EFI_LOADER_CODE too?
> 
> It is not OK to call "machine_restart()" if you couldn't allocate space.
> This would put the machine into a loop ... rebooting and failing, with
> no oportunity to read the error message.  Just use panic().
> 
> -Tony
> 

Tony,

Thanks for feedback. I will take a look at these and see if I can
simplify it further.

-- 
Khalid

==================================
Khalid Aziz                       Open Source and Linux Organization
(970)898-9214                                        Hewlett-Packard
khalid.aziz@hp.com                                  Fort Collins, CO

"The Linux kernel is subject to relentless development" 
                                - Alessandro Rubini