| Message ID | e96b2f318bda5f2e6a20f6a984e671837c9ae216.1599130989.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add support for loading images above 4GB | expand |
Hi Michal, On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote: > > The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which > introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. > Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a > problem to record these addresses in 64bit format. > The patch adds support for systems which need to load images above 4GB. But what about 32-bit systems who read this as a 32-bit number? Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT? > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > common/fdt_support.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/common/fdt_support.c b/common/fdt_support.c > index b8a8768a2147..5ae75df3c658 100644 > --- a/common/fdt_support.c > +++ b/common/fdt_support.c > @@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name, > if (node < 0) > return node; > > - /* > - * We record these as 32bit entities, possibly truncating addresses. > - * However, spl_fit.c is not 64bit safe either: i.e. we should not > - * have an issue here. > - */ > - fdt_setprop_u32(blob, node, "load", load_addr); > + fdt_setprop_u64(blob, node, "load", load_addr); > if (entry_point != -1) > - fdt_setprop_u32(blob, node, "entry", entry_point); > + fdt_setprop_u64(blob, node, "entry", entry_point); > fdt_setprop_u32(blob, node, "size", size); > if (type) > fdt_setprop_string(blob, node, "type", type); > -- > 2.28.0 > Regards, SImon
On 07. 09. 20 3:43, Simon Glass wrote: > Hi Michal, > > On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote: >> >> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which >> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. >> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a >> problem to record these addresses in 64bit format. >> The patch adds support for systems which need to load images above 4GB. > > But what about 32-bit systems who read this as a 32-bit number? > Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT? The code for reading doesn't really care if value is 32bit or 64bit. The fit_image_get_entry() and fit_image_get_load() read number of cells used and based on that read 32 or 64 bit values. Thanks, Michal
+andestech.com On 07. 09. 20 3:43, Simon Glass wrote: > Hi Michal, > > On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote: >> >> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which >> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. >> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a >> problem to record these addresses in 64bit format. >> The patch adds support for systems which need to load images above 4GB. > > But what about 32-bit systems who read this as a 32-bit number? > Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT? > >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> common/fdt_support.c | 9 ++------- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/common/fdt_support.c b/common/fdt_support.c >> index b8a8768a2147..5ae75df3c658 100644 >> --- a/common/fdt_support.c >> +++ b/common/fdt_support.c >> @@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name, >> if (node < 0) >> return node; >> >> - /* >> - * We record these as 32bit entities, possibly truncating addresses. >> - * However, spl_fit.c is not 64bit safe either: i.e. we should not >> - * have an issue here. >> - */ >> - fdt_setprop_u32(blob, node, "load", load_addr); >> + fdt_setprop_u64(blob, node, "load", load_addr); >> if (entry_point != -1) >> - fdt_setprop_u32(blob, node, "entry", entry_point); >> + fdt_setprop_u64(blob, node, "entry", entry_point); >> fdt_setprop_u32(blob, node, "size", size); >> if (type) >> fdt_setprop_string(blob, node, "type", type); >> -- >> 2.28.0 >> riscv: Your code is also using entry-point/load-addr and should be also fixed based on this series. Can you please take a look at this series? I am interested how riscv users are keeping in sync SPL and full U-Boot. Thanks, Michal
Hi Michal, On Mon, 7 Sep 2020 at 02:29, Michal Simek <michal.simek@xilinx.com> wrote: > > > > On 07. 09. 20 3:43, Simon Glass wrote: > > Hi Michal, > > > > On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which > >> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. > >> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a > >> problem to record these addresses in 64bit format. > >> The patch adds support for systems which need to load images above 4GB. > > > > But what about 32-bit systems who read this as a 32-bit number? > > Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT? > > The code for reading doesn't really care if value is 32bit or 64bit. > The fit_image_get_entry() and fit_image_get_load() read number of cells > used and based on that read 32 or 64 bit values. Sorry I wrote that before looking at the function. The functions should have header-file comments that indicate what they do. Regards, Simon
On 07. 09. 20 15:57, Simon Glass wrote: > Hi Michal, > > On Mon, 7 Sep 2020 at 02:29, Michal Simek <michal.simek@xilinx.com> wrote: >> >> >> >> On 07. 09. 20 3:43, Simon Glass wrote: >>> Hi Michal, >>> >>> On Thu, 3 Sep 2020 at 05:03, Michal Simek <michal.simek@xilinx.com> wrote: >>>> >>>> The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which >>>> introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. >>>> Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a >>>> problem to record these addresses in 64bit format. >>>> The patch adds support for systems which need to load images above 4GB. >>> >>> But what about 32-bit systems who read this as a 32-bit number? >>> Perhaps we should write 32-bit if !CONFIG_PHYS_64BIT? >> >> The code for reading doesn't really care if value is 32bit or 64bit. >> The fit_image_get_entry() and fit_image_get_load() read number of cells >> used and based on that read 32 or 64 bit values. > > Sorry I wrote that before looking at the function. The functions > should have header-file comments that indicate what they do. kernel-doc format is in common/image-fit.c already. M
diff --git a/common/fdt_support.c b/common/fdt_support.c index b8a8768a2147..5ae75df3c658 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -611,14 +611,9 @@ int fdt_record_loadable(void *blob, u32 index, const char *name, if (node < 0) return node; - /* - * We record these as 32bit entities, possibly truncating addresses. - * However, spl_fit.c is not 64bit safe either: i.e. we should not - * have an issue here. - */ - fdt_setprop_u32(blob, node, "load", load_addr); + fdt_setprop_u64(blob, node, "load", load_addr); if (entry_point != -1) - fdt_setprop_u32(blob, node, "entry", entry_point); + fdt_setprop_u64(blob, node, "entry", entry_point); fdt_setprop_u32(blob, node, "size", size); if (type) fdt_setprop_string(blob, node, "type", type);
The commit 9f45aeb93727 ("spl: fit: implement fdt_record_loadable") which introduced fdt_record_loadable() state there spl_fit.c is not 64bit safe. Based on my tests on Xilinx ZynqMP zcu102 platform there shouldn't be a problem to record these addresses in 64bit format. The patch adds support for systems which need to load images above 4GB. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- common/fdt_support.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)