[U-Boot] [PATCH v1 13/15] efi_loader: make pool allocations cacheline aligned

[Rob Clark]

On Fri, Aug 11, 2017 at 2:10 PM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 08/11/2017 07:27 PM, Rob Clark wrote:
>> On Fri, Aug 11, 2017 at 12:58 PM, Heinrich Schuchardt
>> <xypron.glpk at gmx.de> wrote:
>>> On 08/10/2017 08:29 PM, Rob Clark wrote:
>>>> This avoids printf() spam about file reads (such as loading an image)
>>>> into unaligned buffers (and the associated memcpy()).  And generally
>>>> seems like a good idea.
>>>>
>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>> ---
>>>>  lib/efi_loader/efi_memory.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>>> index 9e079f1fa3..2ba8d8b42b 100644
>>>> --- a/lib/efi_loader/efi_memory.c
>>>> +++ b/lib/efi_loader/efi_memory.c
>>>> @@ -43,7 +43,7 @@ void *efi_bounce_buffer;
>>>>   */
>>>>  struct efi_pool_allocation {
>>>>       u64 num_pages;
>>>> -     char data[];
>>>> +     char data[] __attribute__((aligned(ARCH_DMA_MINALIGN)));
>>>>  };
>>>>
>>>>  /*
>>>> @@ -356,7 +356,8 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
>>>>  {
>>>>       efi_status_t r;
>>>>       efi_physical_addr_t t;
>>>> -     u64 num_pages = (size + sizeof(u64) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>>> +     u64 num_pages = DIV_ROUND_UP(size + sizeof(struct efi_pool_allocation),
>>>> +                                  EFI_PAGE_SIZE);
>>>
>>> It seems you missed my mail dated 2017-08-02T01:21Z:
>>>
>>> With DIV_ROUND_UP you introduce a 64bit division. Depending on the
>>> architecture this is only available via stdlib which is not available in
>>> U-Boot.
>>>
>>> Please, use
>>> + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>>> as in the original line.
>>>
>>
>> I didn't miss it.. but I did disagree with it.  It is an unsigned
>> division by a power-of-two.  The compiler turns this into a
>> right-shift.  So in fact both ways generate the same code, but the
>> DIV_ROUND_UP() is more clear.
>>
>> BR,
>> -R
>>
>
> I compiled:
>
> int main(int argc, char *argv[])
> {
>         long long int a = 16;
>         long long int b = 2;
>         long long int c;
>         c = a / b;
>         return c;
> }
>
> on a mips system with gcc 6.3
>
> gcc -O0 -nostdlib test.c > test
>
> and got
>
> /tmp/ccenefOj.o: In function `main':
> test.c:(.text+0x48): undefined reference to `__divdi3'
> test.c:(.text+0x60): undefined reference to `__divdi3'
>

Note that EFI_LOADER does not even compile with -O0.. but try:  c = a / 2;

my guess is with -O0 gcc is not doing constant propagation.

I did already try this on aarch64 with -O0 (but without the variable in between)

BR,
-R