| Message ID | 20170810182948.27653-14-robdclark@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Alexander Graf |
| Headers | show |
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; >
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
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
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
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
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
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
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;
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(-)