* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size @ 2018-11-27 0:10 Marek Vasut 2018-11-27 7:03 ` Simon Goldschmidt 2018-11-27 12:17 ` Tom Rini 0 siblings, 2 replies; 19+ messages in thread From: Marek Vasut @ 2018-11-27 0:10 UTC (permalink / raw) To: u-boot Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both fdt_high and initrd_high. This change implies that the FDT and initrd will always be copied into the first 256 MiB of RAM instead of being used in place, which can cause various kinds of inobvious problems. The simpler problems include FDT or initrd being overwritten or being used from unaligned addresses, especially on ARM64. The overhead of copying the FDT to aligned location is negligible and these problems go away, so the benefit is significant. Regarding alignment problems with fitImage. The alignment of DT properties is always 32 bits, which implies that the alignment of the "data" property in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The kernel expects all elements, including DT and initrd, to be aligned to 64 bits on ARM64, thus using them in place may not be possible. Using the bootm_size assures correct alignment, again with negligible overhead. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org> Cc: Tom Rini <trini@konsulko.com> --- include/configs/alt.h | 3 +-- include/configs/gose.h | 3 +-- include/configs/koelsch.h | 3 +-- include/configs/lager.h | 3 +-- include/configs/porter.h | 3 +-- include/configs/rcar-gen3-common.h | 3 +-- include/configs/silk.h | 3 +-- include/configs/stout.h | 3 +-- 8 files changed, 8 insertions(+), 16 deletions(-) diff --git a/include/configs/alt.h b/include/configs/alt.h index cc6a7bf638..3f7f379e06 100644 --- a/include/configs/alt.h +++ b/include/configs/alt.h @@ -39,8 +39,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/gose.h b/include/configs/gose.h index 36ac88a20d..8f0e378488 100644 --- a/include/configs/gose.h +++ b/include/configs/gose.h @@ -35,8 +35,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/koelsch.h b/include/configs/koelsch.h index ef26a144a9..33c8bd4149 100644 --- a/include/configs/koelsch.h +++ b/include/configs/koelsch.h @@ -35,8 +35,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/lager.h b/include/configs/lager.h index 08498c6d81..89c5d01d3c 100644 --- a/include/configs/lager.h +++ b/include/configs/lager.h @@ -36,8 +36,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/porter.h b/include/configs/porter.h index e56dc3f1ec..9950f80afd 100644 --- a/include/configs/porter.h +++ b/include/configs/porter.h @@ -40,8 +40,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/rcar-gen3-common.h b/include/configs/rcar-gen3-common.h index 435d108628..6c2fa6a63c 100644 --- a/include/configs/rcar-gen3-common.h +++ b/include/configs/rcar-gen3-common.h @@ -59,8 +59,7 @@ #define CONFIG_ENV_SIZE_REDUND (CONFIG_ENV_SIZE) #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffffffffffff\0" \ - "initrd_high=0xffffffffffffffff\0" + "bootm_size=0x10000000\0" #define CONFIG_BOOTCOMMAND \ "tftp 0x48080000 Image; " \ diff --git a/include/configs/silk.h b/include/configs/silk.h index a94928bd16..112806c342 100644 --- a/include/configs/silk.h +++ b/include/configs/silk.h @@ -40,8 +40,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 diff --git a/include/configs/stout.h b/include/configs/stout.h index b72b565c33..93d980569c 100644 --- a/include/configs/stout.h +++ b/include/configs/stout.h @@ -44,8 +44,7 @@ #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK #define CONFIG_EXTRA_ENV_SETTINGS \ - "fdt_high=0xffffffff\0" \ - "initrd_high=0xffffffff\0" + "bootm_size=0x10000000\0" /* SPL support */ #define CONFIG_SPL_TEXT_BASE 0xe6300000 -- 2.18.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 0:10 [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size Marek Vasut @ 2018-11-27 7:03 ` Simon Goldschmidt 2018-11-27 12:20 ` Marek Vasut 2018-11-28 1:23 ` Tom Rini 2018-11-27 12:17 ` Tom Rini 1 sibling, 2 replies; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-27 7:03 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both > fdt_high and initrd_high. This change implies that the FDT and initrd > will always be copied into the first 256 MiB of RAM instead of being > used in place, which can cause various kinds of inobvious problems. > > The simpler problems include FDT or initrd being overwritten or being > used from unaligned addresses, especially on ARM64. The overhead of > copying the FDT to aligned location is negligible and these problems > go away, so the benefit is significant. > > Regarding alignment problems with fitImage. The alignment of DT properties > is always 32 bits, which implies that the alignment of the "data" property > in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The > kernel expects all elements, including DT and initrd, to be aligned to > 64 bits on ARM64, thus using them in place may not be possible. Using the > bootm_size assures correct alignment, again with negligible overhead. In my opinion, all of these raw addresses defined in scripts or config should be removed: They are probably vulnerable to overwriting themselves as they only provide an address, not a range. Just out of curiosity: is it required to put fdt and initrd into the first 256 MiB or is this just some 'random' limit to ensure we use lmb but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my series to fix the recent CVE issues improves lmb to not overwrite U-Boot and other reserved addresses and you might be able to remove 'bootm_size', too. The improved lmb code would just allocate an aligned address somewhere in the available RAM. Simon > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org> > Cc: Tom Rini <trini@konsulko.com> > --- > include/configs/alt.h | 3 +-- > include/configs/gose.h | 3 +-- > include/configs/koelsch.h | 3 +-- > include/configs/lager.h | 3 +-- > include/configs/porter.h | 3 +-- > include/configs/rcar-gen3-common.h | 3 +-- > include/configs/silk.h | 3 +-- > include/configs/stout.h | 3 +-- > 8 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/include/configs/alt.h b/include/configs/alt.h > index cc6a7bf638..3f7f379e06 100644 > --- a/include/configs/alt.h > +++ b/include/configs/alt.h > @@ -39,8 +39,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/gose.h b/include/configs/gose.h > index 36ac88a20d..8f0e378488 100644 > --- a/include/configs/gose.h > +++ b/include/configs/gose.h > @@ -35,8 +35,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/koelsch.h b/include/configs/koelsch.h > index ef26a144a9..33c8bd4149 100644 > --- a/include/configs/koelsch.h > +++ b/include/configs/koelsch.h > @@ -35,8 +35,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/lager.h b/include/configs/lager.h > index 08498c6d81..89c5d01d3c 100644 > --- a/include/configs/lager.h > +++ b/include/configs/lager.h > @@ -36,8 +36,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/porter.h b/include/configs/porter.h > index e56dc3f1ec..9950f80afd 100644 > --- a/include/configs/porter.h > +++ b/include/configs/porter.h > @@ -40,8 +40,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/rcar-gen3-common.h b/include/configs/rcar-gen3-common.h > index 435d108628..6c2fa6a63c 100644 > --- a/include/configs/rcar-gen3-common.h > +++ b/include/configs/rcar-gen3-common.h > @@ -59,8 +59,7 @@ > #define CONFIG_ENV_SIZE_REDUND (CONFIG_ENV_SIZE) > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffffffffffff\0" \ > - "initrd_high=0xffffffffffffffff\0" > + "bootm_size=0x10000000\0" > > #define CONFIG_BOOTCOMMAND \ > "tftp 0x48080000 Image; " \ > diff --git a/include/configs/silk.h b/include/configs/silk.h > index a94928bd16..112806c342 100644 > --- a/include/configs/silk.h > +++ b/include/configs/silk.h > @@ -40,8 +40,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > diff --git a/include/configs/stout.h b/include/configs/stout.h > index b72b565c33..93d980569c 100644 > --- a/include/configs/stout.h > +++ b/include/configs/stout.h > @@ -44,8 +44,7 @@ > #define CONFIG_SYS_CLK_FREQ RMOBILE_XTAL_CLK > > #define CONFIG_EXTRA_ENV_SETTINGS \ > - "fdt_high=0xffffffff\0" \ > - "initrd_high=0xffffffff\0" > + "bootm_size=0x10000000\0" > > /* SPL support */ > #define CONFIG_SPL_TEXT_BASE 0xe6300000 > -- > 2.18.0 > > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > https://lists.denx.de/listinfo/u-boot ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 7:03 ` Simon Goldschmidt @ 2018-11-27 12:20 ` Marek Vasut 2018-11-27 12:33 ` Simon Goldschmidt 2018-11-28 1:23 ` Tom Rini 1 sibling, 1 reply; 19+ messages in thread From: Marek Vasut @ 2018-11-27 12:20 UTC (permalink / raw) To: u-boot On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both >> fdt_high and initrd_high. This change implies that the FDT and initrd >> will always be copied into the first 256 MiB of RAM instead of being >> used in place, which can cause various kinds of inobvious problems. >> >> The simpler problems include FDT or initrd being overwritten or being >> used from unaligned addresses, especially on ARM64. The overhead of >> copying the FDT to aligned location is negligible and these problems >> go away, so the benefit is significant. >> >> Regarding alignment problems with fitImage. The alignment of DT properties >> is always 32 bits, which implies that the alignment of the "data" property >> in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The >> kernel expects all elements, including DT and initrd, to be aligned to >> 64 bits on ARM64, thus using them in place may not be possible. Using the >> bootm_size assures correct alignment, again with negligible overhead. > > In my opinion, all of these raw addresses defined in scripts or config > should be removed: They are probably vulnerable to overwriting > themselves as they only provide an address, not a range. This is not an address, it's size. And this one at least assures that the first 256 MiB are reserved for the kernel/FDT/initrd during bootm time. > Just out of curiosity: is it required to put fdt and initrd into the > first 256 MiB or is this just some 'random' limit to ensure we use lmb > but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > series to fix the recent CVE issues improves lmb to not overwrite > U-Boot and other reserved addresses and you might be able to remove > 'bootm_size', too. The improved lmb code would just allocate an > aligned address somewhere in the available RAM. It's just the first 256 MiB from the beginning, so there's enough space between that and U-Boot on all these boards. [...] -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 12:20 ` Marek Vasut @ 2018-11-27 12:33 ` Simon Goldschmidt 2018-11-27 13:09 ` Marek Vasut 0 siblings, 1 reply; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-27 12:33 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > > On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: > >> > >> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both > >> fdt_high and initrd_high. This change implies that the FDT and initrd > >> will always be copied into the first 256 MiB of RAM instead of being > >> used in place, which can cause various kinds of inobvious problems. > >> > >> The simpler problems include FDT or initrd being overwritten or being > >> used from unaligned addresses, especially on ARM64. The overhead of > >> copying the FDT to aligned location is negligible and these problems > >> go away, so the benefit is significant. > >> > >> Regarding alignment problems with fitImage. The alignment of DT properties > >> is always 32 bits, which implies that the alignment of the "data" property > >> in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The > >> kernel expects all elements, including DT and initrd, to be aligned to > >> 64 bits on ARM64, thus using them in place may not be possible. Using the > >> bootm_size assures correct alignment, again with negligible overhead. > > > > In my opinion, all of these raw addresses defined in scripts or config > > should be removed: They are probably vulnerable to overwriting > > themselves as they only provide an address, not a range. > > This is not an address, it's size. And this one at least assures that > the first 256 MiB are reserved for the kernel/FDT/initrd during bootm time. Sorry I did not express myself clear enough. I meant that "fdt_high" and "initrd_high" are bad because they contain an address only, not a range. The 'bootm_size' thing is much better! > > Just out of curiosity: is it required to put fdt and initrd into the > > first 256 MiB or is this just some 'random' limit to ensure we use lmb > > but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > > series to fix the recent CVE issues improves lmb to not overwrite > > U-Boot and other reserved addresses and you might be able to remove > > 'bootm_size', too. The improved lmb code would just allocate an > > aligned address somewhere in the available RAM. > > It's just the first 256 MiB from the beginning, so there's enough space > between that and U-Boot on all these boards. Of course. I wanted to know if it would be good enough if U-Boot would just put it somewhere without overwriting things or do you really need them in the first 256 MiB? Because the revised lmb code would make sure there's nothing overwritten, so there would be no need to trim at 256 MiB. Regards, Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 12:33 ` Simon Goldschmidt @ 2018-11-27 13:09 ` Marek Vasut 2018-11-27 15:26 ` Simon Goldschmidt 0 siblings, 1 reply; 19+ messages in thread From: Marek Vasut @ 2018-11-27 13:09 UTC (permalink / raw) To: u-boot On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: >>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>> >>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both >>>> fdt_high and initrd_high. This change implies that the FDT and initrd >>>> will always be copied into the first 256 MiB of RAM instead of being >>>> used in place, which can cause various kinds of inobvious problems. >>>> >>>> The simpler problems include FDT or initrd being overwritten or being >>>> used from unaligned addresses, especially on ARM64. The overhead of >>>> copying the FDT to aligned location is negligible and these problems >>>> go away, so the benefit is significant. >>>> >>>> Regarding alignment problems with fitImage. The alignment of DT properties >>>> is always 32 bits, which implies that the alignment of the "data" property >>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The >>>> kernel expects all elements, including DT and initrd, to be aligned to >>>> 64 bits on ARM64, thus using them in place may not be possible. Using the >>>> bootm_size assures correct alignment, again with negligible overhead. >>> >>> In my opinion, all of these raw addresses defined in scripts or config >>> should be removed: They are probably vulnerable to overwriting >>> themselves as they only provide an address, not a range. >> >> This is not an address, it's size. And this one at least assures that >> the first 256 MiB are reserved for the kernel/FDT/initrd during bootm time. > > Sorry I did not express myself clear enough. I meant that "fdt_high" > and "initrd_high" are bad because they contain an address only, not a > range. The 'bootm_size' thing is much better! Well the fdt_high and intrd_high can also contain a special ~0 value, which says "use the fdt/initrd in place", which is dangerous. >>> Just out of curiosity: is it required to put fdt and initrd into the >>> first 256 MiB or is this just some 'random' limit to ensure we use lmb >>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my >>> series to fix the recent CVE issues improves lmb to not overwrite >>> U-Boot and other reserved addresses and you might be able to remove >>> 'bootm_size', too. The improved lmb code would just allocate an >>> aligned address somewhere in the available RAM. >> >> It's just the first 256 MiB from the beginning, so there's enough space >> between that and U-Boot on all these boards. > > Of course. I wanted to know if it would be good enough if U-Boot would > just put it somewhere without overwriting things or do you really need > them in the first 256 MiB? Because the revised lmb code would make > sure there's nothing overwritten, so there would be no need to trim at > 256 MiB. You can put them anywhere, you just need to meet the alignment requirements. Can the new LMB code help somehow with that ? And if so, how ? -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 13:09 ` Marek Vasut @ 2018-11-27 15:26 ` Simon Goldschmidt 2018-11-27 15:47 ` Marek Vasut 0 siblings, 1 reply; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-27 15:26 UTC (permalink / raw) To: u-boot On 27.11.2018 14:09, Marek Vasut wrote: > On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: >> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> wrote: >>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: >>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: >>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both >>>>> fdt_high and initrd_high. This change implies that the FDT and initrd >>>>> will always be copied into the first 256 MiB of RAM instead of being >>>>> used in place, which can cause various kinds of inobvious problems. >>>>> >>>>> The simpler problems include FDT or initrd being overwritten or being >>>>> used from unaligned addresses, especially on ARM64. The overhead of >>>>> copying the FDT to aligned location is negligible and these problems >>>>> go away, so the benefit is significant. >>>>> >>>>> Regarding alignment problems with fitImage. The alignment of DT properties >>>>> is always 32 bits, which implies that the alignment of the "data" property >>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The >>>>> kernel expects all elements, including DT and initrd, to be aligned to >>>>> 64 bits on ARM64, thus using them in place may not be possible. Using the >>>>> bootm_size assures correct alignment, again with negligible overhead. >>>> In my opinion, all of these raw addresses defined in scripts or config >>>> should be removed: They are probably vulnerable to overwriting >>>> themselves as they only provide an address, not a range. >>> This is not an address, it's size. And this one at least assures that >>> the first 256 MiB are reserved for the kernel/FDT/initrd during bootm time. >> Sorry I did not express myself clear enough. I meant that "fdt_high" >> and "initrd_high" are bad because they contain an address only, not a >> range. The 'bootm_size' thing is much better! > Well the fdt_high and intrd_high can also contain a special ~0 value, > which says "use the fdt/initrd in place", which is dangerous. > >>>> Just out of curiosity: is it required to put fdt and initrd into the >>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb >>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my >>>> series to fix the recent CVE issues improves lmb to not overwrite >>>> U-Boot and other reserved addresses and you might be able to remove >>>> 'bootm_size', too. The improved lmb code would just allocate an >>>> aligned address somewhere in the available RAM. >>> It's just the first 256 MiB from the beginning, so there's enough space >>> between that and U-Boot on all these boards. >> Of course. I wanted to know if it would be good enough if U-Boot would >> just put it somewhere without overwriting things or do you really need >> them in the first 256 MiB? Because the revised lmb code would make >> sure there's nothing overwritten, so there would be no need to trim at >> 256 MiB. > You can put them anywhere, you just need to meet the alignment > requirements. Can the new LMB code help somehow with that ? And if so, how ? My additions to the LMB code should only ensure nothing gets overwritten so you don't have to limit boom_size to 256MiB (but use the complete RAM when bootm_size is not set). Alignment does not change but should already be OK with LMB as you use it? Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 15:26 ` Simon Goldschmidt @ 2018-11-27 15:47 ` Marek Vasut 2018-11-27 18:31 ` Simon Goldschmidt 0 siblings, 1 reply; 19+ messages in thread From: Marek Vasut @ 2018-11-27 15:47 UTC (permalink / raw) To: u-boot On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > On 27.11.2018 14:09, Marek Vasut wrote: >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> >>> wrote: >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> >>>>> wrote: >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop >>>>>> both >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd >>>>>> will always be copied into the first 256 MiB of RAM instead of being >>>>>> used in place, which can cause various kinds of inobvious problems. >>>>>> >>>>>> The simpler problems include FDT or initrd being overwritten or being >>>>>> used from unaligned addresses, especially on ARM64. The overhead of >>>>>> copying the FDT to aligned location is negligible and these problems >>>>>> go away, so the benefit is significant. >>>>>> >>>>>> Regarding alignment problems with fitImage. The alignment of DT >>>>>> properties >>>>>> is always 32 bits, which implies that the alignment of the "data" >>>>>> property >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role >>>>>> here. The >>>>>> kernel expects all elements, including DT and initrd, to be >>>>>> aligned to >>>>>> 64 bits on ARM64, thus using them in place may not be possible. >>>>>> Using the >>>>>> bootm_size assures correct alignment, again with negligible overhead. >>>>> In my opinion, all of these raw addresses defined in scripts or config >>>>> should be removed: They are probably vulnerable to overwriting >>>>> themselves as they only provide an address, not a range. >>>> This is not an address, it's size. And this one at least assures that >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during >>>> bootm time. >>> Sorry I did not express myself clear enough. I meant that "fdt_high" >>> and "initrd_high" are bad because they contain an address only, not a >>> range. The 'bootm_size' thing is much better! >> Well the fdt_high and intrd_high can also contain a special ~0 value, >> which says "use the fdt/initrd in place", which is dangerous. >> >>>>> Just out of curiosity: is it required to put fdt and initrd into the >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my >>>>> series to fix the recent CVE issues improves lmb to not overwrite >>>>> U-Boot and other reserved addresses and you might be able to remove >>>>> 'bootm_size', too. The improved lmb code would just allocate an >>>>> aligned address somewhere in the available RAM. >>>> It's just the first 256 MiB from the beginning, so there's enough space >>>> between that and U-Boot on all these boards. >>> Of course. I wanted to know if it would be good enough if U-Boot would >>> just put it somewhere without overwriting things or do you really need >>> them in the first 256 MiB? Because the revised lmb code would make >>> sure there's nothing overwritten, so there would be no need to trim at >>> 256 MiB. >> You can put them anywhere, you just need to meet the alignment >> requirements. Can the new LMB code help somehow with that ? And if so, >> how ? > > My additions to the LMB code should only ensure nothing gets overwritten > so you don't have to limit boom_size to 256MiB (but use the complete RAM > when bootm_size is not set). > Alignment does not change but should already be OK with LMB as you use it? If I can use the entire RAM (except U-Boot and fitImage), that'd be nice. What change do I need to do ? -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 15:47 ` Marek Vasut @ 2018-11-27 18:31 ` Simon Goldschmidt 2018-11-27 19:06 ` Marek Vasut 2018-11-28 1:31 ` Tom Rini 0 siblings, 2 replies; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-27 18:31 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > > On 27.11.2018 14:09, Marek Vasut wrote: > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> > >>> wrote: > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> > >>>>> wrote: > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop > >>>>>> both > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd > >>>>>> will always be copied into the first 256 MiB of RAM instead of being > >>>>>> used in place, which can cause various kinds of inobvious problems. > >>>>>> > >>>>>> The simpler problems include FDT or initrd being overwritten or being > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of > >>>>>> copying the FDT to aligned location is negligible and these problems > >>>>>> go away, so the benefit is significant. > >>>>>> > >>>>>> Regarding alignment problems with fitImage. The alignment of DT > >>>>>> properties > >>>>>> is always 32 bits, which implies that the alignment of the "data" > >>>>>> property > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role > >>>>>> here. The > >>>>>> kernel expects all elements, including DT and initrd, to be > >>>>>> aligned to > >>>>>> 64 bits on ARM64, thus using them in place may not be possible. > >>>>>> Using the > >>>>>> bootm_size assures correct alignment, again with negligible overhead. > >>>>> In my opinion, all of these raw addresses defined in scripts or config > >>>>> should be removed: They are probably vulnerable to overwriting > >>>>> themselves as they only provide an address, not a range. > >>>> This is not an address, it's size. And this one at least assures that > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during > >>>> bootm time. > >>> Sorry I did not express myself clear enough. I meant that "fdt_high" > >>> and "initrd_high" are bad because they contain an address only, not a > >>> range. The 'bootm_size' thing is much better! > >> Well the fdt_high and intrd_high can also contain a special ~0 value, > >> which says "use the fdt/initrd in place", which is dangerous. > >> > >>>>> Just out of curiosity: is it required to put fdt and initrd into the > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > >>>>> series to fix the recent CVE issues improves lmb to not overwrite > >>>>> U-Boot and other reserved addresses and you might be able to remove > >>>>> 'bootm_size', too. The improved lmb code would just allocate an > >>>>> aligned address somewhere in the available RAM. > >>>> It's just the first 256 MiB from the beginning, so there's enough space > >>>> between that and U-Boot on all these boards. > >>> Of course. I wanted to know if it would be good enough if U-Boot would > >>> just put it somewhere without overwriting things or do you really need > >>> them in the first 256 MiB? Because the revised lmb code would make > >>> sure there's nothing overwritten, so there would be no need to trim at > >>> 256 MiB. > >> You can put them anywhere, you just need to meet the alignment > >> requirements. Can the new LMB code help somehow with that ? And if so, > >> how ? > > > > My additions to the LMB code should only ensure nothing gets overwritten > > so you don't have to limit boom_size to 256MiB (but use the complete RAM > > when bootm_size is not set). > > Alignment does not change but should already be OK with LMB as you use it? > > If I can use the entire RAM (except U-Boot and fitImage), that'd be > nice. What change do I need to do ? I don't know yet, sorry. I basically asked this question to find out about the usage of 'bootm_size'. It's not really documented and I couldn't find out it's full meaning yet. Because e.g. it is set to 16 MiB for socfpga gen5, which sounds a little low... From reading the code, doesn't it already work when leaving out 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not defining CONFIG_SYS_BOOTMAPSZ?) But I don't really know, finding that out and making it work is one of my goals for that series I'm working on. The series started with allowing all subitems in a FIT to be uncompressed but I found some issues there and it grows... Regards, Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 18:31 ` Simon Goldschmidt @ 2018-11-27 19:06 ` Marek Vasut 2018-11-28 1:31 ` Tom Rini 1 sibling, 0 replies; 19+ messages in thread From: Marek Vasut @ 2018-11-27 19:06 UTC (permalink / raw) To: u-boot On 11/27/2018 07:31 PM, Simon Goldschmidt wrote: > On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: >>> On 27.11.2018 14:09, Marek Vasut wrote: >>>> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: >>>>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> >>>>> wrote: >>>>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: >>>>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> >>>>>>> wrote: >>>>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop >>>>>>>> both >>>>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd >>>>>>>> will always be copied into the first 256 MiB of RAM instead of being >>>>>>>> used in place, which can cause various kinds of inobvious problems. >>>>>>>> >>>>>>>> The simpler problems include FDT or initrd being overwritten or being >>>>>>>> used from unaligned addresses, especially on ARM64. The overhead of >>>>>>>> copying the FDT to aligned location is negligible and these problems >>>>>>>> go away, so the benefit is significant. >>>>>>>> >>>>>>>> Regarding alignment problems with fitImage. The alignment of DT >>>>>>>> properties >>>>>>>> is always 32 bits, which implies that the alignment of the "data" >>>>>>>> property >>>>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role >>>>>>>> here. The >>>>>>>> kernel expects all elements, including DT and initrd, to be >>>>>>>> aligned to >>>>>>>> 64 bits on ARM64, thus using them in place may not be possible. >>>>>>>> Using the >>>>>>>> bootm_size assures correct alignment, again with negligible overhead. >>>>>>> In my opinion, all of these raw addresses defined in scripts or config >>>>>>> should be removed: They are probably vulnerable to overwriting >>>>>>> themselves as they only provide an address, not a range. >>>>>> This is not an address, it's size. And this one at least assures that >>>>>> the first 256 MiB are reserved for the kernel/FDT/initrd during >>>>>> bootm time. >>>>> Sorry I did not express myself clear enough. I meant that "fdt_high" >>>>> and "initrd_high" are bad because they contain an address only, not a >>>>> range. The 'bootm_size' thing is much better! >>>> Well the fdt_high and intrd_high can also contain a special ~0 value, >>>> which says "use the fdt/initrd in place", which is dangerous. >>>> >>>>>>> Just out of curiosity: is it required to put fdt and initrd into the >>>>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb >>>>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my >>>>>>> series to fix the recent CVE issues improves lmb to not overwrite >>>>>>> U-Boot and other reserved addresses and you might be able to remove >>>>>>> 'bootm_size', too. The improved lmb code would just allocate an >>>>>>> aligned address somewhere in the available RAM. >>>>>> It's just the first 256 MiB from the beginning, so there's enough space >>>>>> between that and U-Boot on all these boards. >>>>> Of course. I wanted to know if it would be good enough if U-Boot would >>>>> just put it somewhere without overwriting things or do you really need >>>>> them in the first 256 MiB? Because the revised lmb code would make >>>>> sure there's nothing overwritten, so there would be no need to trim at >>>>> 256 MiB. >>>> You can put them anywhere, you just need to meet the alignment >>>> requirements. Can the new LMB code help somehow with that ? And if so, >>>> how ? >>> >>> My additions to the LMB code should only ensure nothing gets overwritten >>> so you don't have to limit boom_size to 256MiB (but use the complete RAM >>> when bootm_size is not set). >>> Alignment does not change but should already be OK with LMB as you use it? >> >> If I can use the entire RAM (except U-Boot and fitImage), that'd be >> nice. What change do I need to do ? > > I don't know yet, sorry. I basically asked this question to find out > about the usage of 'bootm_size'. It's not really documented and I > couldn't find out it's full meaning yet. Because e.g. it is set to 16 > MiB for socfpga gen5, which sounds a little low... > > From reading the code, doesn't it already work when leaving out > 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not > defining CONFIG_SYS_BOOTMAPSZ?) > > But I don't really know, finding that out and making it work is one of > my goals for that series I'm working on. The series started with > allowing all subitems in a FIT to be uncompressed but I found some > issues there and it grows... Thanks for all that work :-) -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 18:31 ` Simon Goldschmidt 2018-11-27 19:06 ` Marek Vasut @ 2018-11-28 1:31 ` Tom Rini 2018-11-28 8:46 ` Simon Goldschmidt 1 sibling, 1 reply; 19+ messages in thread From: Tom Rini @ 2018-11-28 1:31 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 07:31:24PM +0100, Simon Goldschmidt wrote: > On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > > > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > > > On 27.11.2018 14:09, Marek Vasut wrote: > > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> > > >>> wrote: > > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> > > >>>>> wrote: > > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop > > >>>>>> both > > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd > > >>>>>> will always be copied into the first 256 MiB of RAM instead of being > > >>>>>> used in place, which can cause various kinds of inobvious problems. > > >>>>>> > > >>>>>> The simpler problems include FDT or initrd being overwritten or being > > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of > > >>>>>> copying the FDT to aligned location is negligible and these problems > > >>>>>> go away, so the benefit is significant. > > >>>>>> > > >>>>>> Regarding alignment problems with fitImage. The alignment of DT > > >>>>>> properties > > >>>>>> is always 32 bits, which implies that the alignment of the "data" > > >>>>>> property > > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role > > >>>>>> here. The > > >>>>>> kernel expects all elements, including DT and initrd, to be > > >>>>>> aligned to > > >>>>>> 64 bits on ARM64, thus using them in place may not be possible. > > >>>>>> Using the > > >>>>>> bootm_size assures correct alignment, again with negligible overhead. > > >>>>> In my opinion, all of these raw addresses defined in scripts or config > > >>>>> should be removed: They are probably vulnerable to overwriting > > >>>>> themselves as they only provide an address, not a range. > > >>>> This is not an address, it's size. And this one at least assures that > > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during > > >>>> bootm time. > > >>> Sorry I did not express myself clear enough. I meant that "fdt_high" > > >>> and "initrd_high" are bad because they contain an address only, not a > > >>> range. The 'bootm_size' thing is much better! > > >> Well the fdt_high and intrd_high can also contain a special ~0 value, > > >> which says "use the fdt/initrd in place", which is dangerous. > > >> > > >>>>> Just out of curiosity: is it required to put fdt and initrd into the > > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb > > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > > >>>>> series to fix the recent CVE issues improves lmb to not overwrite > > >>>>> U-Boot and other reserved addresses and you might be able to remove > > >>>>> 'bootm_size', too. The improved lmb code would just allocate an > > >>>>> aligned address somewhere in the available RAM. > > >>>> It's just the first 256 MiB from the beginning, so there's enough space > > >>>> between that and U-Boot on all these boards. > > >>> Of course. I wanted to know if it would be good enough if U-Boot would > > >>> just put it somewhere without overwriting things or do you really need > > >>> them in the first 256 MiB? Because the revised lmb code would make > > >>> sure there's nothing overwritten, so there would be no need to trim at > > >>> 256 MiB. > > >> You can put them anywhere, you just need to meet the alignment > > >> requirements. Can the new LMB code help somehow with that ? And if so, > > >> how ? > > > > > > My additions to the LMB code should only ensure nothing gets overwritten > > > so you don't have to limit boom_size to 256MiB (but use the complete RAM > > > when bootm_size is not set). > > > Alignment does not change but should already be OK with LMB as you use it? > > > > If I can use the entire RAM (except U-Boot and fitImage), that'd be > > nice. What change do I need to do ? > > I don't know yet, sorry. I basically asked this question to find out > about the usage of 'bootm_size'. It's not really documented and I > couldn't find out it's full meaning yet. Because e.g. it is set to 16 > MiB for socfpga gen5, which sounds a little low... > > From reading the code, doesn't it already work when leaving out > 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not > defining CONFIG_SYS_BOOTMAPSZ?) It's all a little funny, yes. common/image.c is where all of this gets laid out and we keep everything being passed to bootm (whatever image type it may be) between bootm_low (either set explicitly or start of first bank of DRAM) and bootm_size (default ends up basically being all of DRAM) and then we do lmb stuff. The main reason, as I replied just now earlier in the thread, is that for Linux we need to make sure everything is in "lowmem" and rather than disabling relocation (which leads to people seeing fail to boots when something overlaps something else, over time, and they spend a bunch of time debugging) via initrd_high/fdt_high=0xffffffff SoCs should instead set a reasonable constraint on bootm_size so that we can make sure nothing overlaps, when we can do that. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181127/59ef6df4/attachment.sig> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-28 1:31 ` Tom Rini @ 2018-11-28 8:46 ` Simon Goldschmidt 2018-11-28 12:19 ` Tom Rini 0 siblings, 1 reply; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-28 8:46 UTC (permalink / raw) To: u-boot On Wed, Nov 28, 2018 at 2:31 AM Tom Rini <trini@konsulko.com> wrote: > > On Tue, Nov 27, 2018 at 07:31:24PM +0100, Simon Goldschmidt wrote: > > On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > > > > > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > > > > On 27.11.2018 14:09, Marek Vasut wrote: > > > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > > > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> > > > >>> wrote: > > > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > > > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> > > > >>>>> wrote: > > > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop > > > >>>>>> both > > > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd > > > >>>>>> will always be copied into the first 256 MiB of RAM instead of being > > > >>>>>> used in place, which can cause various kinds of inobvious problems. > > > >>>>>> > > > >>>>>> The simpler problems include FDT or initrd being overwritten or being > > > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of > > > >>>>>> copying the FDT to aligned location is negligible and these problems > > > >>>>>> go away, so the benefit is significant. > > > >>>>>> > > > >>>>>> Regarding alignment problems with fitImage. The alignment of DT > > > >>>>>> properties > > > >>>>>> is always 32 bits, which implies that the alignment of the "data" > > > >>>>>> property > > > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role > > > >>>>>> here. The > > > >>>>>> kernel expects all elements, including DT and initrd, to be > > > >>>>>> aligned to > > > >>>>>> 64 bits on ARM64, thus using them in place may not be possible. > > > >>>>>> Using the > > > >>>>>> bootm_size assures correct alignment, again with negligible overhead. > > > >>>>> In my opinion, all of these raw addresses defined in scripts or config > > > >>>>> should be removed: They are probably vulnerable to overwriting > > > >>>>> themselves as they only provide an address, not a range. > > > >>>> This is not an address, it's size. And this one at least assures that > > > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during > > > >>>> bootm time. > > > >>> Sorry I did not express myself clear enough. I meant that "fdt_high" > > > >>> and "initrd_high" are bad because they contain an address only, not a > > > >>> range. The 'bootm_size' thing is much better! > > > >> Well the fdt_high and intrd_high can also contain a special ~0 value, > > > >> which says "use the fdt/initrd in place", which is dangerous. > > > >> > > > >>>>> Just out of curiosity: is it required to put fdt and initrd into the > > > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb > > > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > > > >>>>> series to fix the recent CVE issues improves lmb to not overwrite > > > >>>>> U-Boot and other reserved addresses and you might be able to remove > > > >>>>> 'bootm_size', too. The improved lmb code would just allocate an > > > >>>>> aligned address somewhere in the available RAM. > > > >>>> It's just the first 256 MiB from the beginning, so there's enough space > > > >>>> between that and U-Boot on all these boards. > > > >>> Of course. I wanted to know if it would be good enough if U-Boot would > > > >>> just put it somewhere without overwriting things or do you really need > > > >>> them in the first 256 MiB? Because the revised lmb code would make > > > >>> sure there's nothing overwritten, so there would be no need to trim at > > > >>> 256 MiB. > > > >> You can put them anywhere, you just need to meet the alignment > > > >> requirements. Can the new LMB code help somehow with that ? And if so, > > > >> how ? > > > > > > > > My additions to the LMB code should only ensure nothing gets overwritten > > > > so you don't have to limit boom_size to 256MiB (but use the complete RAM > > > > when bootm_size is not set). > > > > Alignment does not change but should already be OK with LMB as you use it? > > > > > > If I can use the entire RAM (except U-Boot and fitImage), that'd be > > > nice. What change do I need to do ? > > > > I don't know yet, sorry. I basically asked this question to find out > > about the usage of 'bootm_size'. It's not really documented and I > > couldn't find out it's full meaning yet. Because e.g. it is set to 16 > > MiB for socfpga gen5, which sounds a little low... > > > > From reading the code, doesn't it already work when leaving out > > 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not > > defining CONFIG_SYS_BOOTMAPSZ?) > > It's all a little funny, yes. common/image.c is where all of this gets > laid out and we keep everything being passed to bootm (whatever image > type it may be) between bootm_low (either set explicitly or start of > first bank of DRAM) and bootm_size (default ends up basically being all > of DRAM) and then we do lmb stuff. > > The main reason, as I replied just now earlier in the thread, is that > for Linux we need to make sure everything is in "lowmem" and rather than > disabling relocation (which leads to people seeing fail to boots when > something overlaps something else, over time, and they spend a bunch of > time debugging) via initrd_high/fdt_high=0xffffffff SoCs should instead > set a reasonable constraint on bootm_size so that we can make sure > nothing overlaps, when we can do that. Right, I spent some time debugging this, too when starting with FIT images containing Kernel, FDT, initrd and FPGA image... I wasn't aware of the requirement to have everything in "lowmem" for Linux though (I'll even have to check what exactly the lowmem range is for my platform/Kernel). And without this requirement, I just failed to see why we need "boom_size" when it also works with defaulting to just find a free memory block just somewhere in the DRAM. Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-28 8:46 ` Simon Goldschmidt @ 2018-11-28 12:19 ` Tom Rini 2018-11-28 20:55 ` Simon Goldschmidt 0 siblings, 1 reply; 19+ messages in thread From: Tom Rini @ 2018-11-28 12:19 UTC (permalink / raw) To: u-boot On Wed, Nov 28, 2018 at 09:46:43AM +0100, Simon Goldschmidt wrote: > On Wed, Nov 28, 2018 at 2:31 AM Tom Rini <trini@konsulko.com> wrote: > > > > On Tue, Nov 27, 2018 at 07:31:24PM +0100, Simon Goldschmidt wrote: > > > On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > > > > > > > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > > > > > On 27.11.2018 14:09, Marek Vasut wrote: > > > > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > > > > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> > > > > >>> wrote: > > > > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > > > > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> > > > > >>>>> wrote: > > > > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop > > > > >>>>>> both > > > > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd > > > > >>>>>> will always be copied into the first 256 MiB of RAM instead of being > > > > >>>>>> used in place, which can cause various kinds of inobvious problems. > > > > >>>>>> > > > > >>>>>> The simpler problems include FDT or initrd being overwritten or being > > > > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of > > > > >>>>>> copying the FDT to aligned location is negligible and these problems > > > > >>>>>> go away, so the benefit is significant. > > > > >>>>>> > > > > >>>>>> Regarding alignment problems with fitImage. The alignment of DT > > > > >>>>>> properties > > > > >>>>>> is always 32 bits, which implies that the alignment of the "data" > > > > >>>>>> property > > > > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role > > > > >>>>>> here. The > > > > >>>>>> kernel expects all elements, including DT and initrd, to be > > > > >>>>>> aligned to > > > > >>>>>> 64 bits on ARM64, thus using them in place may not be possible. > > > > >>>>>> Using the > > > > >>>>>> bootm_size assures correct alignment, again with negligible overhead. > > > > >>>>> In my opinion, all of these raw addresses defined in scripts or config > > > > >>>>> should be removed: They are probably vulnerable to overwriting > > > > >>>>> themselves as they only provide an address, not a range. > > > > >>>> This is not an address, it's size. And this one at least assures that > > > > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during > > > > >>>> bootm time. > > > > >>> Sorry I did not express myself clear enough. I meant that "fdt_high" > > > > >>> and "initrd_high" are bad because they contain an address only, not a > > > > >>> range. The 'bootm_size' thing is much better! > > > > >> Well the fdt_high and intrd_high can also contain a special ~0 value, > > > > >> which says "use the fdt/initrd in place", which is dangerous. > > > > >> > > > > >>>>> Just out of curiosity: is it required to put fdt and initrd into the > > > > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb > > > > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > > > > >>>>> series to fix the recent CVE issues improves lmb to not overwrite > > > > >>>>> U-Boot and other reserved addresses and you might be able to remove > > > > >>>>> 'bootm_size', too. The improved lmb code would just allocate an > > > > >>>>> aligned address somewhere in the available RAM. > > > > >>>> It's just the first 256 MiB from the beginning, so there's enough space > > > > >>>> between that and U-Boot on all these boards. > > > > >>> Of course. I wanted to know if it would be good enough if U-Boot would > > > > >>> just put it somewhere without overwriting things or do you really need > > > > >>> them in the first 256 MiB? Because the revised lmb code would make > > > > >>> sure there's nothing overwritten, so there would be no need to trim at > > > > >>> 256 MiB. > > > > >> You can put them anywhere, you just need to meet the alignment > > > > >> requirements. Can the new LMB code help somehow with that ? And if so, > > > > >> how ? > > > > > > > > > > My additions to the LMB code should only ensure nothing gets overwritten > > > > > so you don't have to limit boom_size to 256MiB (but use the complete RAM > > > > > when bootm_size is not set). > > > > > Alignment does not change but should already be OK with LMB as you use it? > > > > > > > > If I can use the entire RAM (except U-Boot and fitImage), that'd be > > > > nice. What change do I need to do ? > > > > > > I don't know yet, sorry. I basically asked this question to find out > > > about the usage of 'bootm_size'. It's not really documented and I > > > couldn't find out it's full meaning yet. Because e.g. it is set to 16 > > > MiB for socfpga gen5, which sounds a little low... > > > > > > From reading the code, doesn't it already work when leaving out > > > 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not > > > defining CONFIG_SYS_BOOTMAPSZ?) > > > > It's all a little funny, yes. common/image.c is where all of this gets > > laid out and we keep everything being passed to bootm (whatever image > > type it may be) between bootm_low (either set explicitly or start of > > first bank of DRAM) and bootm_size (default ends up basically being all > > of DRAM) and then we do lmb stuff. > > > > The main reason, as I replied just now earlier in the thread, is that > > for Linux we need to make sure everything is in "lowmem" and rather than > > disabling relocation (which leads to people seeing fail to boots when > > something overlaps something else, over time, and they spend a bunch of > > time debugging) via initrd_high/fdt_high=0xffffffff SoCs should instead > > set a reasonable constraint on bootm_size so that we can make sure > > nothing overlaps, when we can do that. > > Right, I spent some time debugging this, too when starting with FIT > images containing Kernel, FDT, initrd and FPGA image... > > I wasn't aware of the requirement to have everything in "lowmem" for > Linux though (I'll even have to check what exactly the lowmem range is > for my platform/Kernel). And without this requirement, I just failed > to see why we need "boom_size" when it also works with defaulting to > just find a free memory block just somewhere in the DRAM. JFYI, the general lowmem limit is 768MB, but since it can be configured at build-time at least, it can actually get tiny (and off the top of my head, I don't think 256MB is as small as it can be, but rather a I think reasonable assumption on my part given the trade-offs in-kernel). -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181128/e52e0b57/attachment.sig> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-28 12:19 ` Tom Rini @ 2018-11-28 20:55 ` Simon Goldschmidt 2019-03-05 18:42 ` Eugeniu Rosca 0 siblings, 1 reply; 19+ messages in thread From: Simon Goldschmidt @ 2018-11-28 20:55 UTC (permalink / raw) To: u-boot On Wed, Nov 28, 2018 at 1:19 PM Tom Rini <trini@konsulko.com> wrote: > > On Wed, Nov 28, 2018 at 09:46:43AM +0100, Simon Goldschmidt wrote: > > On Wed, Nov 28, 2018 at 2:31 AM Tom Rini <trini@konsulko.com> wrote: > > > > > > On Tue, Nov 27, 2018 at 07:31:24PM +0100, Simon Goldschmidt wrote: > > > > On Tue, Nov 27, 2018 at 4:47 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > > > > > > > > > On 11/27/2018 04:26 PM, Simon Goldschmidt wrote: > > > > > > On 27.11.2018 14:09, Marek Vasut wrote: > > > > > >> On 11/27/2018 01:33 PM, Simon Goldschmidt wrote: > > > > > >>> On Tue, Nov 27, 2018 at 1:25 PM Marek Vasut <marek.vasut@gmail.com> > > > > > >>> wrote: > > > > > >>>> On 11/27/2018 08:03 AM, Simon Goldschmidt wrote: > > > > > >>>>> On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> > > > > > >>>>> wrote: > > > > > >>>>>> Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop > > > > > >>>>>> both > > > > > >>>>>> fdt_high and initrd_high. This change implies that the FDT and initrd > > > > > >>>>>> will always be copied into the first 256 MiB of RAM instead of being > > > > > >>>>>> used in place, which can cause various kinds of inobvious problems. > > > > > >>>>>> > > > > > >>>>>> The simpler problems include FDT or initrd being overwritten or being > > > > > >>>>>> used from unaligned addresses, especially on ARM64. The overhead of > > > > > >>>>>> copying the FDT to aligned location is negligible and these problems > > > > > >>>>>> go away, so the benefit is significant. > > > > > >>>>>> > > > > > >>>>>> Regarding alignment problems with fitImage. The alignment of DT > > > > > >>>>>> properties > > > > > >>>>>> is always 32 bits, which implies that the alignment of the "data" > > > > > >>>>>> property > > > > > >>>>>> in fitImage is also 32 bits. The /incbin/ syntax plays no role > > > > > >>>>>> here. The > > > > > >>>>>> kernel expects all elements, including DT and initrd, to be > > > > > >>>>>> aligned to > > > > > >>>>>> 64 bits on ARM64, thus using them in place may not be possible. > > > > > >>>>>> Using the > > > > > >>>>>> bootm_size assures correct alignment, again with negligible overhead. > > > > > >>>>> In my opinion, all of these raw addresses defined in scripts or config > > > > > >>>>> should be removed: They are probably vulnerable to overwriting > > > > > >>>>> themselves as they only provide an address, not a range. > > > > > >>>> This is not an address, it's size. And this one at least assures that > > > > > >>>> the first 256 MiB are reserved for the kernel/FDT/initrd during > > > > > >>>> bootm time. > > > > > >>> Sorry I did not express myself clear enough. I meant that "fdt_high" > > > > > >>> and "initrd_high" are bad because they contain an address only, not a > > > > > >>> range. The 'bootm_size' thing is much better! > > > > > >> Well the fdt_high and intrd_high can also contain a special ~0 value, > > > > > >> which says "use the fdt/initrd in place", which is dangerous. > > > > > >> > > > > > >>>>> Just out of curiosity: is it required to put fdt and initrd into the > > > > > >>>>> first 256 MiB or is this just some 'random' limit to ensure we use lmb > > > > > >>>>> but don't overwrite U-Boot (text, heap, stack, etc)? Because if so, my > > > > > >>>>> series to fix the recent CVE issues improves lmb to not overwrite > > > > > >>>>> U-Boot and other reserved addresses and you might be able to remove > > > > > >>>>> 'bootm_size', too. The improved lmb code would just allocate an > > > > > >>>>> aligned address somewhere in the available RAM. > > > > > >>>> It's just the first 256 MiB from the beginning, so there's enough space > > > > > >>>> between that and U-Boot on all these boards. > > > > > >>> Of course. I wanted to know if it would be good enough if U-Boot would > > > > > >>> just put it somewhere without overwriting things or do you really need > > > > > >>> them in the first 256 MiB? Because the revised lmb code would make > > > > > >>> sure there's nothing overwritten, so there would be no need to trim at > > > > > >>> 256 MiB. > > > > > >> You can put them anywhere, you just need to meet the alignment > > > > > >> requirements. Can the new LMB code help somehow with that ? And if so, > > > > > >> how ? > > > > > > > > > > > > My additions to the LMB code should only ensure nothing gets overwritten > > > > > > so you don't have to limit boom_size to 256MiB (but use the complete RAM > > > > > > when bootm_size is not set). > > > > > > Alignment does not change but should already be OK with LMB as you use it? > > > > > > > > > > If I can use the entire RAM (except U-Boot and fitImage), that'd be > > > > > nice. What change do I need to do ? > > > > > > > > I don't know yet, sorry. I basically asked this question to find out > > > > about the usage of 'bootm_size'. It's not really documented and I > > > > couldn't find out it's full meaning yet. Because e.g. it is set to 16 > > > > MiB for socfpga gen5, which sounds a little low... > > > > > > > > From reading the code, doesn't it already work when leaving out > > > > 'bootm_size'? (And leaving out 'bootm_mapsize' as well and not > > > > defining CONFIG_SYS_BOOTMAPSZ?) > > > > > > It's all a little funny, yes. common/image.c is where all of this gets > > > laid out and we keep everything being passed to bootm (whatever image > > > type it may be) between bootm_low (either set explicitly or start of > > > first bank of DRAM) and bootm_size (default ends up basically being all > > > of DRAM) and then we do lmb stuff. > > > > > > The main reason, as I replied just now earlier in the thread, is that > > > for Linux we need to make sure everything is in "lowmem" and rather than > > > disabling relocation (which leads to people seeing fail to boots when > > > something overlaps something else, over time, and they spend a bunch of > > > time debugging) via initrd_high/fdt_high=0xffffffff SoCs should instead > > > set a reasonable constraint on bootm_size so that we can make sure > > > nothing overlaps, when we can do that. > > > > Right, I spent some time debugging this, too when starting with FIT > > images containing Kernel, FDT, initrd and FPGA image... > > > > I wasn't aware of the requirement to have everything in "lowmem" for > > Linux though (I'll even have to check what exactly the lowmem range is > > for my platform/Kernel). And without this requirement, I just failed > > to see why we need "boom_size" when it also works with defaulting to > > just find a free memory block just somewhere in the DRAM. > > JFYI, the general lowmem limit is 768MB, but since it can be configured > at build-time at least, it can actually get tiny (and off the top of my > head, I don't think 256MB is as small as it can be, but rather a I think > reasonable assumption on my part given the trade-offs in-kernel). OK, thanks. I'll keep that in mind when testing my changes. And 256 MB should probably enough for everything we need to to in U-Boot... Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-28 20:55 ` Simon Goldschmidt @ 2019-03-05 18:42 ` Eugeniu Rosca 2019-03-05 18:56 ` Marek Vasut 0 siblings, 1 reply; 19+ messages in thread From: Eugeniu Rosca @ 2019-03-05 18:42 UTC (permalink / raw) To: u-boot Hello Marek, May I ask for your opinion on Renesas commits [1-2] especially in light of the `bootm_size` changes done in your patch? The topics look orthogonal to me, but I kindly ask for your confirmation. Also, purely related to the `bootm_size` patch, compiling v2019.04-rc3 U-Boot for H3-Salvator-X with the diff [3] applied, I get: common/image.c: In function 'env_get_bootm_mapsize': common/image.c:520:2: error: #error "Take bootm_size from CONFIG" #error "Take bootm_size from CONFIG" ^~~~~ Does this mean the "CONFIG_SYS_BOOTMAPSZ" definition has to be removed from include/configs/rcar-gen3-common.h, to allow the patch to have the desired effect? [1] https://github.com/renesas-rcar/u-boot/commit/86ad30910df818 ("arm: rcar_gen3: Add skip of relocation") [2] https://github.com/renesas-rcar/u-boot/commit/91e27e4cc463e ("arm: rcar_gen3: Add skip of relocation") [3] Brutal way to reveal the active bootm_size: diff --git a/common/image.c b/common/image.c index 4d4248f234fb..d65de506060b 100644 --- a/common/image.c +++ b/common/image.c @@ -517,8 +517,10 @@ phys_size_t env_get_bootm_mapsize(void) } #if defined(CONFIG_SYS_BOOTMAPSZ) +#error "Take bootm_size from CONFIG" return CONFIG_SYS_BOOTMAPSZ; #else +#error "Take bootm_size from env" return env_get_bootm_size(); #endif } Thanks, Eugeniu. ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2019-03-05 18:42 ` Eugeniu Rosca @ 2019-03-05 18:56 ` Marek Vasut 2019-03-05 22:28 ` Eugeniu Rosca 0 siblings, 1 reply; 19+ messages in thread From: Marek Vasut @ 2019-03-05 18:56 UTC (permalink / raw) To: u-boot On 3/5/19 7:42 PM, Eugeniu Rosca wrote: > Hello Marek, Hi, > May I ask for your opinion on Renesas commits [1-2] especially in > light of the `bootm_size` changes done in your patch? > The topics look orthogonal to me, but I kindly ask for your confirmation. What do you want to know ? With that commit, U-Boot is sitting in the middle of your RAM and you're likely to overwrite or corrupt it sooner or later, so that patch is not going upstream. > Also, purely related to the `bootm_size` patch, compiling v2019.04-rc3 > U-Boot for H3-Salvator-X with the diff [3] applied, I get: > > common/image.c: In function 'env_get_bootm_mapsize': > common/image.c:520:2: error: #error "Take bootm_size from CONFIG" > #error "Take bootm_size from CONFIG" > ^~~~~ > > Does this mean the "CONFIG_SYS_BOOTMAPSZ" definition has to be removed > from include/configs/rcar-gen3-common.h, to allow the patch to have > the desired effect? Possibly, if you could study it a bit and if that's the case, send a patch, that'd be nice. > [1] https://github.com/renesas-rcar/u-boot/commit/86ad30910df818 > ("arm: rcar_gen3: Add skip of relocation") > [2] https://github.com/renesas-rcar/u-boot/commit/91e27e4cc463e ("arm: > rcar_gen3: Add skip of relocation") > [3] Brutal way to reveal the active bootm_size: > diff --git a/common/image.c b/common/image.c > index 4d4248f234fb..d65de506060b 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -517,8 +517,10 @@ phys_size_t env_get_bootm_mapsize(void) > } > > #if defined(CONFIG_SYS_BOOTMAPSZ) > +#error "Take bootm_size from CONFIG" > return CONFIG_SYS_BOOTMAPSZ; > #else > +#error "Take bootm_size from env" > return env_get_bootm_size(); > #endif > } > > Thanks, > Eugeniu. > -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2019-03-05 18:56 ` Marek Vasut @ 2019-03-05 22:28 ` Eugeniu Rosca 2019-03-05 22:57 ` Marek Vasut 0 siblings, 1 reply; 19+ messages in thread From: Eugeniu Rosca @ 2019-03-05 22:28 UTC (permalink / raw) To: u-boot On Tue, Mar 05, 2019 at 07:56:39PM +0100, Marek Vasut wrote: > On 3/5/19 7:42 PM, Eugeniu Rosca wrote: > > Hello Marek, > > Hi, > > > May I ask for your opinion on Renesas commits [1-2] especially in > > light of the `bootm_size` changes done in your patch? > > The topics look orthogonal to me, but I kindly ask for your confirmation. > > What do you want to know ? With that commit, U-Boot is sitting in the > middle of your RAM and you're likely to overwrite or corrupt it sooner > or later, so that patch is not going upstream. That's already very helpful. Thanks. > > > Also, purely related to the `bootm_size` patch, compiling v2019.04-rc3 > > U-Boot for H3-Salvator-X with the diff [3] applied, I get: > > > > common/image.c: In function 'env_get_bootm_mapsize': > > common/image.c:520:2: error: #error "Take bootm_size from CONFIG" > > #error "Take bootm_size from CONFIG" > > ^~~~~ > > > > Does this mean the "CONFIG_SYS_BOOTMAPSZ" definition has to be removed > > from include/configs/rcar-gen3-common.h, to allow the patch to have > > the desired effect? > > Possibly, if you could study it a bit and if that's the case, send a > patch, that'd be nice. Submitted https://patchwork.ozlabs.org/patch/1052012/ > > > [1] https://github.com/renesas-rcar/u-boot/commit/86ad30910df818 > > ("arm: rcar_gen3: Add skip of relocation") > > [2] https://github.com/renesas-rcar/u-boot/commit/91e27e4cc463e ("arm: > > rcar_gen3: Add skip of relocation") > > [3] Brutal way to reveal the active bootm_size: > > diff --git a/common/image.c b/common/image.c > > index 4d4248f234fb..d65de506060b 100644 > > --- a/common/image.c > > +++ b/common/image.c > > @@ -517,8 +517,10 @@ phys_size_t env_get_bootm_mapsize(void) > > } > > > > #if defined(CONFIG_SYS_BOOTMAPSZ) > > +#error "Take bootm_size from CONFIG" > > return CONFIG_SYS_BOOTMAPSZ; > > #else > > +#error "Take bootm_size from env" > > return env_get_bootm_size(); > > #endif > > } > > > > Thanks, > > Eugeniu. > > > > > -- > Best regards, > Marek Vasut Best regards, Eugeniu. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2019-03-05 22:28 ` Eugeniu Rosca @ 2019-03-05 22:57 ` Marek Vasut 0 siblings, 0 replies; 19+ messages in thread From: Marek Vasut @ 2019-03-05 22:57 UTC (permalink / raw) To: u-boot On 3/5/19 11:28 PM, Eugeniu Rosca wrote: > On Tue, Mar 05, 2019 at 07:56:39PM +0100, Marek Vasut wrote: >> On 3/5/19 7:42 PM, Eugeniu Rosca wrote: >>> Hello Marek, >> >> Hi, >> >>> May I ask for your opinion on Renesas commits [1-2] especially in >>> light of the `bootm_size` changes done in your patch? >>> The topics look orthogonal to me, but I kindly ask for your confirmation. >> >> What do you want to know ? With that commit, U-Boot is sitting in the >> middle of your RAM and you're likely to overwrite or corrupt it sooner >> or later, so that patch is not going upstream. > > That's already very helpful. Thanks. > >> >>> Also, purely related to the `bootm_size` patch, compiling v2019.04-rc3 >>> U-Boot for H3-Salvator-X with the diff [3] applied, I get: >>> >>> common/image.c: In function 'env_get_bootm_mapsize': >>> common/image.c:520:2: error: #error "Take bootm_size from CONFIG" >>> #error "Take bootm_size from CONFIG" >>> ^~~~~ >>> >>> Does this mean the "CONFIG_SYS_BOOTMAPSZ" definition has to be removed >>> from include/configs/rcar-gen3-common.h, to allow the patch to have >>> the desired effect? >> >> Possibly, if you could study it a bit and if that's the case, send a >> patch, that'd be nice. > > Submitted https://patchwork.ozlabs.org/patch/1052012/ Nice , thanks! -- Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 7:03 ` Simon Goldschmidt 2018-11-27 12:20 ` Marek Vasut @ 2018-11-28 1:23 ` Tom Rini 1 sibling, 0 replies; 19+ messages in thread From: Tom Rini @ 2018-11-28 1:23 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 08:03:04AM +0100, Simon Goldschmidt wrote: > On Tue, Nov 27, 2018 at 1:11 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > > > Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both > > fdt_high and initrd_high. This change implies that the FDT and initrd > > will always be copied into the first 256 MiB of RAM instead of being > > used in place, which can cause various kinds of inobvious problems. > > > > The simpler problems include FDT or initrd being overwritten or being > > used from unaligned addresses, especially on ARM64. The overhead of > > copying the FDT to aligned location is negligible and these problems > > go away, so the benefit is significant. > > > > Regarding alignment problems with fitImage. The alignment of DT properties > > is always 32 bits, which implies that the alignment of the "data" property > > in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The > > kernel expects all elements, including DT and initrd, to be aligned to > > 64 bits on ARM64, thus using them in place may not be possible. Using the > > bootm_size assures correct alignment, again with negligible overhead. > > In my opinion, all of these raw addresses defined in scripts or config > should be removed: They are probably vulnerable to overwriting > themselves as they only provide an address, not a range. > > Just out of curiosity: is it required to put fdt and initrd into the > first 256 MiB or is this just some 'random' limit to ensure we use lmb It's the smallest limit that both gives us room for fairly large ramdisks, kernels and device tree yet will still be within lowmem for Linux and otherwise meet the requirements of both linux/Documentation/arm/Booting and linux/Documentation/arm64/booting.txt -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181127/0aff0888/attachment.sig> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size 2018-11-27 0:10 [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size Marek Vasut 2018-11-27 7:03 ` Simon Goldschmidt @ 2018-11-27 12:17 ` Tom Rini 1 sibling, 0 replies; 19+ messages in thread From: Tom Rini @ 2018-11-27 12:17 UTC (permalink / raw) To: u-boot On Tue, Nov 27, 2018 at 01:10:54AM +0100, Marek Vasut wrote: > Convert all Renesas R-Car boards to bootm_size of 256 MiB and drop both > fdt_high and initrd_high. This change implies that the FDT and initrd > will always be copied into the first 256 MiB of RAM instead of being > used in place, which can cause various kinds of inobvious problems. > > The simpler problems include FDT or initrd being overwritten or being > used from unaligned addresses, especially on ARM64. The overhead of > copying the FDT to aligned location is negligible and these problems > go away, so the benefit is significant. > > Regarding alignment problems with fitImage. The alignment of DT properties > is always 32 bits, which implies that the alignment of the "data" property > in fitImage is also 32 bits. The /incbin/ syntax plays no role here. The > kernel expects all elements, including DT and initrd, to be aligned to > 64 bits on ARM64, thus using them in place may not be possible. Using the > bootm_size assures correct alignment, again with negligible overhead. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org> > Cc: Tom Rini <trini@konsulko.com> Thanks! Reviewed-by: Tom Rini <trini@konsulko.com> -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181127/d2445bb6/attachment.sig> ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-03-05 22:57 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-27 0:10 [U-Boot] [PATCH] ARM: rmobile: Convert to bootm_size Marek Vasut 2018-11-27 7:03 ` Simon Goldschmidt 2018-11-27 12:20 ` Marek Vasut 2018-11-27 12:33 ` Simon Goldschmidt 2018-11-27 13:09 ` Marek Vasut 2018-11-27 15:26 ` Simon Goldschmidt 2018-11-27 15:47 ` Marek Vasut 2018-11-27 18:31 ` Simon Goldschmidt 2018-11-27 19:06 ` Marek Vasut 2018-11-28 1:31 ` Tom Rini 2018-11-28 8:46 ` Simon Goldschmidt 2018-11-28 12:19 ` Tom Rini 2018-11-28 20:55 ` Simon Goldschmidt 2019-03-05 18:42 ` Eugeniu Rosca 2019-03-05 18:56 ` Marek Vasut 2019-03-05 22:28 ` Eugeniu Rosca 2019-03-05 22:57 ` Marek Vasut 2018-11-28 1:23 ` Tom Rini 2018-11-27 12:17 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.