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

[roy.franz@linaro.org (Roy Franz)]

On Fri, Apr 10, 2015 at 10:14 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> 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'm working on a patch for this, and it also looks like efi_low_alloc() is
not correct for 32 bit builds, as internally it can allocate a >32 bit address,
and then will truncate that to return an 'unsigned long'.

I should have something out next week.

>>
>> 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.