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

[Rob Clark]

On Fri, Aug 11, 2017 at 1:47 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.
>
> This conversion is not enforced by the define in include/linux/kernel.h.
>
> This is not required by ISO/IEC 9899. So you should not rely on it.
> We surely do not want compiler specific coding in U-Boot.

I'm not sure why a standard would require this.  But in practice, it
is one of the easier things a compiler will do.  (Trust me, I've
implemented the same optimization in mesa's shader compiler.)

We elsewhere rely on DCE (dead code elimination) to avoid unresolved
symbols for stuff inside an 'if (CONFIG_IS_ENABLED(FOO))' and that is
a harder thing for a compiler to do.

So unless someone is trying to use a complete toy compiler, it
shouldn't be a problem.  And if they are, they have bigger issues ;-)

BR,
-R