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

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

[Ard Biesheuvel]

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

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

[Roy Franz]

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.

Roy

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

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

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

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

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

[Matt Fleming]

On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote:
> 
> 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.

Out of curiosity, do you have any runtime numbers to backup the claim
that allocate_*() is substantially more lightweight?

-- 
Matt Fleming, Intel Open Source Technology Center

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

[Ard Biesheuvel]

On 15 April 2015 at 11:55, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 10 Apr, at 07:14:34PM, Ard Biesheuvel wrote:
>>
>> 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.
>
> Out of curiosity, do you have any runtime numbers to backup the claim
> that allocate_*() is substantially more lightweight?
>

No, I do not. But since efi_[high|low]_alloc() call
efi_get_memory_map() internally, which itself calls allocate_pool() at
least twice [typically], and then iterate over the entire memory map
to select a suitable slot which gets allocated using allocate_pages(),
calling either of allocate_[pool|pages] directly is arguably more
lightweight. But claiming they are 'substantially heavier' is
unsubstantiated.

-- 
Ard.