Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 7/18/2016 5:00 AM, Mark Rutland wrote:
> Hi,
>
> On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
>> +	if (status == EFI_BUFFER_TOO_SMALL ||
>> +				(*buff_size - *map_size) / sizeof(*m) < 8) {
>>  		efi_call_early(free_pool, m);
>> +		/*
>> +		 * Make sure there is 8 entries worth of headroom so that the
>> +		 * buffer can be reused for a new map after allocations are
>> +		 * no longer permitted.  Its unlikely that the map will grow to
>> +		 * exceed this headroom once we are ready to trigger
>> +		 * ExitBootServices()
>> +		 */
>> +		*map_size += sizeof(*m) * 8;
>
> It's probably worth having something like...
>
>   #define EFI_MMAP_NR_SLACK_SLOTS	8
>
> ... at the top of the file, and changing the comment here to not refer
> to the specific value. That way we avoid duplicating the value, and
> making it easier to modify / backport in future.

You are right, that does make sense.  I'll wait a bit to see if there is 
any other feedback, and add this suggestion to V2.

> As a general note, it's a shame that we don't know desc_size first time
> through. If descs grow, 8 * sizeof(*m) might be too small. That's an
> existing problem though, so I guess it makes sense to be consistent and
> use sizeof(*m) here.

Yep.  Unless I've missed something, it seems like the two options are 
either make a decent attempt to guess at desc_size the first time 
(current approach), or intentionally fail the first time just to get the 
size.  The current approach doesn't seem perfect, but it does have a 
chance at passing with just one attempt.  I couldn't see a better 
solution, but if someone else has an idea, I'd like to hear it.

>
> Otherwise, this looks fine to me.
>
> Thanks,
> Mark.
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.