[Buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Alexey Brodkin]

Back in the day we relied on a default value that used to be 8KiB
and it worked perfectly fine for ARC's default 8KiB page as well as
4 KiB ones, but not for 16 KiB, see [1] for more details.

So that we fixed by setting "max-page-size" if 16KiB pages are in use by
commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages").

But as Yann very rightfully mentioned here [2] we should be setting this
thing explicitly for all page sizes because:
 1. Defaults might change unexpectedly
 2. Explicitly set stuff is better understood
 3. We act similarly to all settings but not only addressing some corner cases

[1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0
[2] https://patchwork.ozlabs.org/patch/1212544/#2330647

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>
---
 arch/arch.mk.arc | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arch.mk.arc b/arch/arch.mk.arc
index 9c04a85a8d..7eb1b95aa7 100644
--- a/arch/arch.mk.arc
+++ b/arch/arch.mk.arc
@@ -5,8 +5,15 @@ ifeq ($(BR2_ARC_ATOMIC_EXT),y)
 ARCH_TOOLCHAIN_WRAPPER_OPTS = -matomic
 endif
 
-# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages
-# it needs to be overridden.
+# Explicitly set LD's "max-page-size" instead of relying on some defaults
+ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
+ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
+endif
+
+ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
+ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
+endif
+
 ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
 ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
 endif
-- 
2.16.2

[Buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Thomas Petazzoni]

On Tue, 24 Dec 2019 17:10:32 +0300
Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:

> Back in the day we relied on a default value that used to be 8KiB
> and it worked perfectly fine for ARC's default 8KiB page as well as
> 4 KiB ones, but not for 16 KiB, see [1] for more details.
> 
> So that we fixed by setting "max-page-size" if 16KiB pages are in use by
> commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages").
> 
> But as Yann very rightfully mentioned here [2] we should be setting this
> thing explicitly for all page sizes because:
>  1. Defaults might change unexpectedly
>  2. Explicitly set stuff is better understood
>  3. We act similarly to all settings but not only addressing some corner cases
> 
> [1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0
> [2] https://patchwork.ozlabs.org/patch/1212544/#2330647
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>

I have applied to master, with one change (see below), and I have one
suggestion (see below as well).


> -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages
> -# it needs to be overridden.
> +# Explicitly set LD's "max-page-size" instead of relying on some defaults
> +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
> +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
> +endif
> +
> +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
> +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
> +endif
> +
>  ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
>  ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
>  endif

Since these options are mutually exclusive:

ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
endif

is a bit more compact, so I changed to use this before committing.

Also, we already have an option called BR2_ARC_PAGE_SIZE, which
contains 4K, 8K or 16K, so I thought if we could use it here as well. I
think we can, but I'd like to hear from Yann about this, because it
uses bash arithmetic, which I'm not sure we're allowed to use:

ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024)))

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Yann E. MORIN]

Thomas, Alexey, All,

On 2019-12-25 22:36 +0100, Thomas Petazzoni spake thusly:
> On Tue, 24 Dec 2019 17:10:32 +0300
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
> > Back in the day we relied on a default value that used to be 8KiB
> > and it worked perfectly fine for ARC's default 8KiB page as well as
> > 4 KiB ones, but not for 16 KiB, see [1] for more details.
> > 
> > So that we fixed by setting "max-page-size" if 16KiB pages are in use by
> > commit d024d369b82d2 ("arch/arc: Accommodate 16 KiB MMU pages").
> > 
> > But as Yann very rightfully mentioned here [2] we should be setting this
> > thing explicitly for all page sizes because:
> >  1. Defaults might change unexpectedly
> >  2. Explicitly set stuff is better understood
> >  3. We act similarly to all settings but not only addressing some corner cases
> > 
> > [1] https://git.buildroot.org/buildroot/commit/?id=d024d369b82d2d3d9d4d75489c19e9488202bca0
> > [2] https://patchwork.ozlabs.org/patch/1212544/#2330647
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> 
> > -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages
> > -# it needs to be overridden.
> > +# Explicitly set LD's "max-page-size" instead of relying on some defaults
> > +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
> > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
> > +endif
> > +
> > +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
> > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
> > +endif
> > +
> >  ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
> >  ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
> >  endif
> 
> Since these options are mutually exclusive:
> 
> ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
> ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
> else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
> ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
> else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
> ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
> endif
> 
> is a bit more compact, so I changed to use this before committing.

Agreed.

> Also, we already have an option called BR2_ARC_PAGE_SIZE, which
> contains 4K, 8K or 16K, so I thought if we could use it here as well. I
> think we can, but I'd like to hear from Yann about this, because it
> uses bash arithmetic, which I'm not sure we're allowed to use:
> 
> ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024)))

We can do bash arithmetics, because we are guaranteed we use bash as our
shell, see: https://git.buildroot.org/buildroot/tree/Makefile#n30

But BR2_ARC_PAGE_SIZE is a string, so it is double-quoted, which would
break the above.

I was also contemplating a similar solution, but I was not too happy
with this trick to drop the suffix. I would have prefered something
like:

    config BR2_ARC_PAGE_SIZE_KB
        int
        default 4 if BR2_ARC_PAGE_SIZE_4K
        default 8 if BR2_ARC_PAGE_SIZE_8K
        default 16 if BR2_ARC_PAGE_SIZE_16K

and then:

    ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024)))

and in package/uclibc/uclibc.mk:

    UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K

... and even that intermediate variable could have been dropped
altogether, to directly write:

        $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config)

And that would have been a bit nicer, I believe.

Regards,
Yann E. MORIN.

> Best regards,
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [arc-buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Alexey Brodkin]

Hi Yann, Thomas,

[snip]

> > > -# By default MAXPAGESIZE for ARC is 8192 so for larger MMU pages
> > > -# it needs to be overridden.
> > > +# Explicitly set LD's "max-page-size" instead of relying on some defaults
> > > +ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
> > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
> > > +endif
> > > +
> > > +ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
> > > +ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
> > > +endif
> > > +
> > >  ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
> > >  ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
> > >  endif
> >
> > Since these options are mutually exclusive:
> >
> > ifeq ($(BR2_ARC_PAGE_SIZE_4K),y)
> > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=4096
> > else ifeq ($(BR2_ARC_PAGE_SIZE_8K),y)
> > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=8192
> > else ifeq ($(BR2_ARC_PAGE_SIZE_16K),y)
> > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=16384
> > endif
> >
> > is a bit more compact, so I changed to use this before committing.
> 
> Agreed.
> 
> > Also, we already have an option called BR2_ARC_PAGE_SIZE, which
> > contains 4K, 8K or 16K, so I thought if we could use it here as well. I
> > think we can, but I'd like to hear from Yann about this, because it
> > uses bash arithmetic, which I'm not sure we're allowed to use:
> >
> > ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(patsubst
> %K,%,$(BR2_ARC_PAGE_SIZE)) * 1024)))
> 
> We can do bash arithmetics, because we are guaranteed we use bash as our
> shell, see: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__git.buildroot.org_buildroot_tree_Makefile-
> 23n30&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=Z74rKOsEpOUneCxYxwKcXHwrAmW8mJhNZjcWnDDJI2U&m=G0xH-
> XYcb2iYOg2bibx_AguTrgrtfR3aD7pVbOW09Q0&s=_K73mNAFEXxJcdkCnKJRzs-VXkSvcFKryO2ZYmXj1LI&e=
> 
> But BR2_ARC_PAGE_SIZE is a string, so it is double-quoted, which would
> break the above.
> 
> I was also contemplating a similar solution, but I was not too happy
> with this trick to drop the suffix. I would have prefered something
> like:
> 
>     config BR2_ARC_PAGE_SIZE_KB
>         int
>         default 4 if BR2_ARC_PAGE_SIZE_4K
>         default 8 if BR2_ARC_PAGE_SIZE_8K
>         default 16 if BR2_ARC_PAGE_SIZE_16K
> 
> and then:
> 
>     ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024)))
> 
> and in package/uclibc/uclibc.mk:
> 
>     UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K
> 
> ... and even that intermediate variable could have been dropped
> altogether, to directly write:
> 
>         $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config)
> 
> And that would have been a bit nicer, I believe.

Agree. So, Yann [as usually] are you OK if I implement that improvement?

-Alexey

[Buildroot] [arc-buildroot] [PATCH] arch/arc: Explicitly set "max-page-size" for GNU LD

[Yann E. MORIN]

Alexey, All,

On 2019-12-27 09:00 +0000, Alexey Brodkin spake thusly:
[--SNIP--]
> > I was also contemplating a similar solution, but I was not too happy
> > with this trick to drop the suffix. I would have prefered something
> > like:
> > 
> >     config BR2_ARC_PAGE_SIZE_KB
> >         int
> >         default 4 if BR2_ARC_PAGE_SIZE_4K
> >         default 8 if BR2_ARC_PAGE_SIZE_8K
> >         default 16 if BR2_ARC_PAGE_SIZE_16K
> > 
> > and then:
> > 
> >     ARCH_TOOLCHAIN_WRAPPER_OPTS += -Wl,-z,max-page-size=$(shell echo $$(($(BR2_ARC_PAGE_SIZE)*1024)))
> > 
> > and in package/uclibc/uclibc.mk:
> > 
> >     UCLIBC_ARC_PAGE_SIZE = CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K
> > 
> > ... and even that intermediate variable could have been dropped
> > altogether, to directly write:
> > 
> >         $(call KCONFIG_ENABLE_OPT,CONFIG_ARC_PAGE_SIZE_$(BR2_ARC_PAGE_SIZE)K,$(@D)/.config)
> > 
> > And that would have been a bit nicer, I believe.
> 
> Agree. So, Yann [as usually] are you OK if I implement that improvement?

As you wish! ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'