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

Submitted by Rob Clark on Aug. 10, 2017, 6:29 p.m.

Details

Message ID 20170810182948.27653-14-robdclark@gmail.com
State New
Delegated to: Alexander Graf
Headers show

Commit Message

Rob Clark Aug. 10, 2017, 6:29 p.m.
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@gmail.com>
---
 lib/efi_loader/efi_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

xypron.glpk@gmx.de Aug. 11, 2017, 4:58 p.m.
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@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.

Best regards

Heinrich

>  
>  	if (size == 0) {
>  		*buffer = NULL;
>
Rob Clark Aug. 11, 2017, 5:27 p.m.
On Fri, Aug 11, 2017 at 12:58 PM, Heinrich Schuchardt
<xypron.glpk@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@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
xypron.glpk@gmx.de Aug. 11, 2017, 5:47 p.m.
On 08/11/2017 07:27 PM, Rob Clark wrote:
> On Fri, Aug 11, 2017 at 12:58 PM, Heinrich Schuchardt
> <xypron.glpk@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@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.

Regards

Heinrich
xypron.glpk@gmx.de Aug. 11, 2017, 6:10 p.m.
On 08/11/2017 07:27 PM, Rob Clark wrote:
> On Fri, Aug 11, 2017 at 12:58 PM, Heinrich Schuchardt
> <xypron.glpk@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@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'

Best regards

Heinrich
Rob Clark Aug. 11, 2017, 6:11 p.m.
On Fri, Aug 11, 2017 at 1:47 PM, Heinrich Schuchardt <xypron.glpk@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@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@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
Rob Clark Aug. 11, 2017, 6:13 p.m.
On Fri, Aug 11, 2017 at 2:10 PM, Heinrich Schuchardt <xypron.glpk@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@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@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
xypron.glpk@gmx.de Aug. 11, 2017, 7:04 p.m.
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@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);
>  
>  	if (size == 0) {
>  		*buffer = NULL;
> 

All other divisions by EFI_PAGE_SIZE are consistently handled in the
same way:

lib/efi_loader/efi_runtime.c:328:u64 pages = (len + EFI_PAGE_SIZE - 1)
>> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:123:
     >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:126:                return (carve_end -
carve_start) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:140:        newmap->desc.num_pages =
(map_end - carve_start) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:145:        map_desc->num_pages =
(carve_start - map_start) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:331:        uint64_t pages = (len +
EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:359:        u64 num_pages = (size +
sizeof(u64) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:448:                u64 pages = (ram_size +
EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:465:        uboot_pages = (gd->ram_top -
uboot_start) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:472:        runtime_pages = (runtime_end -
runtime_start) >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_memory.c:481:                               (64 *
1024 * 1024) >> EFI_PAGE_SHIFT,
lib/efi_loader/efi_image_loader.c:34:                   int type =
*relocs >> EFI_PAGE_SHIFT;
lib/efi_loader/efi_image_loader.c:170:
(virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
arch/arm/cpu/armv8/fsl-layerscape/cpu.c:814:            pages =
(ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
arch/arm/cpu/armv8/fsl-layerscape/fdt.c:112:
ALIGN(*boot_code_size, EFI_PAGE_SIZE) >> EFI_PAGE_SHIFT,
cmd/bootefi.c:162:      fdt_pages = fdt_size >> EFI_PAGE_SHIFT;
cmd/bootefi.c:243:              fdt_pages = fdt_size >> EFI_PAGE_SHIFT;

With you patch we are even inconsistent within the same .c-file.

Regards

Heinrich

Patch hide | download patch | download mbox

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);
 
 	if (size == 0) {
 		*buffer = NULL;