Skip to contentSkip to conversation

efi_loader: make pool allocations cacheline aligned

PATCH New Rob Clark 3 participants 5 messages Aug 1, 2017 – Aug 2, 2017 1 message not in the archive

lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/1 efi_loader: make pool allocations cacheline aligned

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 <[email protected]>
---
 lib/efi_loader/efi_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/efi_loader/efi_memory.c +3 -2
index 9e079f1fa3..2ba8d8b42b 100644
@@ -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;
Signature · 1 line
2.13.0
NAK
#1 lore raw

PATCH 1/1 efi_loader: make pool allocations cacheline aligned

Quoted 24 lines · Rob Clark

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 <[email protected]>
---
 lib/efi_loader/efi_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/efi_loader/efi_memory.c +1 -1
index 9e079f1fa3..2ba8d8b42b 100644
@@ -43,7 +43,7 @@ void *efi_bounce_buffer;
  */
 struct efi_pool_allocation {
 	u64 num_pages;
-	char data[];
+	char data[] __attribute__((aligned(ARCH_DMA_MINALIGN)));
 };
 
 /*

lib/efi_loader/efi_memory.c efi_status_t efi_allocate_pool(int pool_type, unsigned long size, · line 363

Quoted 9 lines
lib/efi_loader/efi_memory.c +2 -1
@@ -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.

Please, use
+ EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
as in the original line.

Could you, please, use
scripts/getmaintainer.pl
to determine the addressees of patches.

Best regards

Heinrich

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#3 lore raw

PATCH 1/1 efi_loader: make pool allocations cacheline aligned

Quoted 31 lines · Heinrich Schuchardt
Quoted 30 lines · Rob Clark

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 <[email protected]>
---
 lib/efi_loader/efi_memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/efi_loader/efi_memory.c +3 -2
index 9e079f1fa3..2ba8d8b42b 100644
@@ -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);
Quoted 8 lines

lib/efi_loader/efi_memory.c efi_status_t efi_allocate_pool(int pool_type, unsigned long size, · line 363

Quoted 3 lines
lib/efi_loader/efi_memory.c +0 -0
      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

Quoted 4 lines

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

#4 lore raw

PATCH 1/1 efi_loader: make pool allocations cacheline aligned

Quoted 1 line · On Mittwoch, 2. August 2017 11:28:37 CEST Rob Clark wrote:

On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt

[...]

Quoted 22 lines
Quoted 14 lines
Quoted 14 lines
(quoted diff) +0 -0
@@ -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) {
Quoted 7 lines
Quoted 1 line
              *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.

Quoted 8 lines

The divisor is a constant power of two so compiler should turn this into a shift

Quoted 3 lines

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

author
#5 lore raw

PATCH 1/1 efi_loader: make pool allocations cacheline aligned

Quoted 27 lines · Stefan
Quoted 1 line · On Mittwoch, 2. August 2017 11:28:37 CEST Rob Clark wrote:

On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt

[...]

Quoted 24 lines
Quoted 14 lines
Quoted 14 lines
(quoted diff) +0 -0
@@ -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) {
Quoted 7 lines
Quoted 1 line
              *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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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