Re: [U-Boot] [PATCH] efi_loader: make pool allocations cacheline aligned

On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt xypron.debian@gmx.de wrote:
On 08/01/2017 10:00 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;
NAK
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.
The divisor is a constant power of two so compiler should turn this into a shift
Please, use
- EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
as in the original line.
This was actually incorrect (missing a "- 1"), which is why I decided to stop open-coding DIV_ROUND_UP().
BR, -R

On Mittwoch, 2. August 2017 11:28:37 CEST Rob Clark wrote:
On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt
[...]
@@ -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;
NAK
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.
The divisor is a constant power of two so compiler should turn this into a shift
Please, use
- EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
as in the original line.
This was actually incorrect (missing a "- 1"), which is why I decided to stop open-coding DIV_ROUND_UP().
EFI_PAGE_MASK == EFI_PAGE_SIZE - 1
Kind regards,
Stefan

On Wed, Aug 2, 2017 at 11:24 AM, Brüns, Stefan Stefan.Bruens@rwth-aachen.de wrote:
On Mittwoch, 2. August 2017 11:28:37 CEST Rob Clark wrote:
On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt
[...]
@@ -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;
NAK
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.
The divisor is a constant power of two so compiler should turn this into a shift
Please, use
- EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
as in the original line.
This was actually incorrect (missing a "- 1"), which is why I decided to stop open-coding DIV_ROUND_UP().
EFI_PAGE_MASK == EFI_PAGE_SIZE - 1
Oh, you're right, I was reading that as PAGE_SIZE, not PAGE_MASK. All the more reason we should use DIV_ROUND_UP().
BR, -R
participants (2)
-
Brüns, Stefan
-
Rob Clark