[PATCH] efi: stub: use a pool allocation for the cmdline

[ard.biesheuvel@linaro.org (Ard Biesheuvel)]

On 10 April 2015 at 18:43, Roy Franz <roy.franz@linaro.org> wrote:
> On Fri, Apr 10, 2015 at 5:56 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> This changes the allocation for the ASCII-converted command
>> line to use an ordinary memory pool rather than a separate
>> page based allocation.
>>
>> Pool allocations are generally preferred over page based
>> allocations due to the fact that they cause less fragmentation,
>> but in the particular case of arm64, where page allocations are
>> rounded up to 64 KB and where this allocation happens to be the
>> only explicit low allocation, it results in the lowest 64 KB of
>> memory to always be taken up by this particular allocation.
>>
>> So allocate from the EFI_LOADER_DATA pool instead.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/firmware/efi/libstub/efi-stub-helper.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index f07d4a67fa76..c95a567ca132 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -684,7 +684,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
>>
>>         options_bytes++;        /* NUL termination */
>>
>> -       status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
>> +       status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>> +                               options_bytes, (void **)&cmdline_addr);
>>         if (status != EFI_SUCCESS)
>>                 return NULL;
>>
>> --
>> 1.8.3.2
>>
>
> Hi Ard,
>
> We can't do this without also changing the frees on the error paths in
> arm-stub.c (line 289)
> and eboot.c (line 1148) to be a pool free as well.
> Also, for x86/x86_64 the address of the command line is put into a
> boot_params structure,
> which only has a __u32 field for the address of the command line, so
> we could get an address
> that won't fit if we use a pool allocation.
> Looking at that bit of if it, I don't think we handle the case of no
> 32 bit addressable memory
> existing at the time of the efi_low_alloc() for x86_64 systems, so if
> a >32 bit address is returned here,
> this won't be detected as a failure, and the cmdline address will be
> the 32 bit truncated address.
>
> I don't think that there is a way to control the address range
> returned by pool allocations,
> so I think we are stuck with the page based allocations if we have
> address restrictions.
>

Ah yes, I wondered about the reason for the low_alloc(). I guess I
could have looked a bit further myself :-)

It is not such a big deal: the memory is reclaimed anyway, I was just
trying to reduce the fragmentation a bit, and trying to avoid
efi_xxx_alloc() which are substantially heavier than calling
allocate_pool() or allocate_pages() directly.

I'll drop this patch then. It's not really worth the effort as its
primarily cosmetics anyway.

-- 
Ard.