| Message ID | 1428354148-1511-4-git-send-email-sjoerd.simons@collabora.co.uk |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
On 04/06/2015 03:02 PM, Sjoerd Simons wrote: > Add dummy bootz_setup implementation allowing the u-boot sandbox to > run bootz. This recognizes both ARM and x86 zImages to validate a > valid zImage was loaded. > diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c > +int bootz_setup(ulong image, ulong *start, ulong *end) > + *start = 0xdead; > + *end = 0xbeef; > + return 0; Isn't that going to cause the rest of bootz to access or jump to some bogus address and crash? Aside from that, this series looks plausible, so, Acked-by: Stephen Warren <swarren@nvidia.com> (I read through patch 6 most thoroughly, briefly skimmed patch 7, and read the others fairly quickly, so take that as an ack primarily on patch 6 although I didn't see anything obviously wrong elsewhere).
On Mon, 2015-04-06 at 15:40 -0600, Stephen Warren wrote: > On 04/06/2015 03:02 PM, Sjoerd Simons wrote: > > Add dummy bootz_setup implementation allowing the u-boot sandbox to > > run bootz. This recognizes both ARM and x86 zImages to validate a > > valid zImage was loaded. > > > diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c > > > +int bootz_setup(ulong image, ulong *start, ulong *end) > > > + *start = 0xdead; > > + *end = 0xbeef; > > + return 0; > > Isn't that going to cause the rest of bootz to access or jump to some > bogus address and crash? A very good question. I hadn't actually double-checked what these values are used for as things just worked and i got distracted by fixing other bits & pieces. Looking through the code, these values are only used to add an LMB region directly after the kernel entry load address. As the sandbox architecture doesn't define either arch_lmb_reserve nor board_lmb_reserve these bogus values don't cause any issues (as they don't seem to make the generic lmb code blow-up thatis), but it's definitely not pretty. I'll fix up that patch a bit to make those values more meaningful later this. > Aside from that, this series looks plausible, so, > Acked-by: Stephen Warren <swarren@nvidia.com> > > (I read through patch 6 most thoroughly, briefly skimmed patch 7, and > read the others fairly quickly, so take that as an ack primarily on > patch 6 although I didn't see anything obviously wrong elsewhere). Thanks for the review!
Hi Sjoerd, On 7 April 2015 at 01:56, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: > On Mon, 2015-04-06 at 15:40 -0600, Stephen Warren wrote: >> On 04/06/2015 03:02 PM, Sjoerd Simons wrote: >> > Add dummy bootz_setup implementation allowing the u-boot sandbox to >> > run bootz. This recognizes both ARM and x86 zImages to validate a >> > valid zImage was loaded. >> >> > diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c >> >> > +int bootz_setup(ulong image, ulong *start, ulong *end) >> >> > + *start = 0xdead; >> > + *end = 0xbeef; >> > + return 0; >> >> Isn't that going to cause the rest of bootz to access or jump to some >> bogus address and crash? > > A very good question. I hadn't actually double-checked what these values > are used for as things just worked and i got distracted by fixing other > bits & pieces. > > Looking through the code, these values are only used to add an LMB > region directly after the kernel entry load address. As the sandbox > architecture doesn't define either arch_lmb_reserve nor > board_lmb_reserve these bogus values don't cause any issues (as they > don't seem to make the generic lmb code blow-up thatis), but it's > definitely not pretty. In the future we may want to emulate this with sandbox. Do you think instead of this we should read out the correct values from the image file? It seems odd to use fake values. In fact I wonder if this should use a common function with ARM, or perhaps part of it should be common? > > I'll fix up that patch a bit to make those values more meaningful later > this. > >> Aside from that, this series looks plausible, so, >> Acked-by: Stephen Warren <swarren@nvidia.com> >> >> (I read through patch 6 most thoroughly, briefly skimmed patch 7, and >> read the others fairly quickly, so take that as an ack primarily on >> patch 6 although I didn't see anything obviously wrong elsewhere). > > Thanks for the review! > > -- > Sjoerd Simons <sjoerd.simons@collabora.co.uk> > Collabora Ltd. Regards, Simon
On 04/07/2015 02:40 PM, Simon Glass wrote: > Hi Sjoerd, > > On 7 April 2015 at 01:56, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote: >> On Mon, 2015-04-06 at 15:40 -0600, Stephen Warren wrote: >>> On 04/06/2015 03:02 PM, Sjoerd Simons wrote: >>>> Add dummy bootz_setup implementation allowing the u-boot sandbox to >>>> run bootz. This recognizes both ARM and x86 zImages to validate a >>>> valid zImage was loaded. >>> >>>> diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c >>> >>>> +int bootz_setup(ulong image, ulong *start, ulong *end) >>> >>>> + *start = 0xdead; >>>> + *end = 0xbeef; >>>> + return 0; >>> >>> Isn't that going to cause the rest of bootz to access or jump to some >>> bogus address and crash? >> >> A very good question. I hadn't actually double-checked what these values >> are used for as things just worked and i got distracted by fixing other >> bits & pieces. >> >> Looking through the code, these values are only used to add an LMB >> region directly after the kernel entry load address. As the sandbox >> architecture doesn't define either arch_lmb_reserve nor >> board_lmb_reserve these bogus values don't cause any issues (as they >> don't seem to make the generic lmb code blow-up thatis), but it's >> definitely not pretty. > > In the future we may want to emulate this with sandbox. Do you think > instead of this we should read out the correct values from the image > file? It seems odd to use fake values. > > In fact I wonder if this should use a common function with ARM, or > perhaps part of it should be common? I wonder if sandbox could somehow exec the loaded image (presumably it'd only support native executables then, not ARM/x86 zImages), or keep a handle to the file the data was loaded from and fdexec() if such an API exists?
Hi Stephen, On 7 April 2015 at 15:00, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/07/2015 02:40 PM, Simon Glass wrote: >> >> Hi Sjoerd, >> >> On 7 April 2015 at 01:56, Sjoerd Simons <sjoerd.simons@collabora.co.uk> >> wrote: >>> >>> On Mon, 2015-04-06 at 15:40 -0600, Stephen Warren wrote: >>>> >>>> On 04/06/2015 03:02 PM, Sjoerd Simons wrote: >>>>> >>>>> Add dummy bootz_setup implementation allowing the u-boot sandbox to >>>>> run bootz. This recognizes both ARM and x86 zImages to validate a >>>>> valid zImage was loaded. >>>> >>>> >>>>> diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c >>>> >>>> >>>>> +int bootz_setup(ulong image, ulong *start, ulong *end) >>>> >>>> >>>>> + *start = 0xdead; >>>>> + *end = 0xbeef; >>>>> + return 0; >>>> >>>> >>>> Isn't that going to cause the rest of bootz to access or jump to some >>>> bogus address and crash? >>> >>> >>> A very good question. I hadn't actually double-checked what these values >>> are used for as things just worked and i got distracted by fixing other >>> bits & pieces. >>> >>> Looking through the code, these values are only used to add an LMB >>> region directly after the kernel entry load address. As the sandbox >>> architecture doesn't define either arch_lmb_reserve nor >>> board_lmb_reserve these bogus values don't cause any issues (as they >>> don't seem to make the generic lmb code blow-up thatis), but it's >>> definitely not pretty. >> >> >> In the future we may want to emulate this with sandbox. Do you think >> instead of this we should read out the correct values from the image >> file? It seems odd to use fake values. >> >> In fact I wonder if this should use a common function with ARM, or >> perhaps part of it should be common? > > > I wonder if sandbox could somehow exec the loaded image (presumably it'd > only support native executables then, not ARM/x86 zImages), or keep a handle > to the file the data was loaded from and fdexec() if such an API exists? Currently bootm on sandbox does everything except execute the image. I'm sure we could do that to with a bit of fiddling. The function os_jump_to_image() probably does what you want. Regards, Simon
diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c index 8ddf4ef..25c5aa4 100644 --- a/arch/sandbox/lib/bootm.c +++ b/arch/sandbox/lib/bootm.c @@ -8,6 +8,25 @@ DECLARE_GLOBAL_DATA_PTR; +int bootz_setup(ulong image, ulong *start, ulong *end) +{ + uint8_t *zimage = map_sysmem(image, 0); + uint8_t arm_magic[] = { 0x18, 0x28, 0x6f, 0x01 }; + + if (memcmp(zimage + 0x202, "HdrS", 4) == 0) { + printf("setting up x86 zImage\n"); + } else if (memcmp(zimage + 0x24, arm_magic, 4) == 0) { + printf("setting up ARM zImage\n"); + } else { + printf("Unrecognized zImage\n"); + return 1; + } + + *start = 0xdead; + *end = 0xbeef; + return 0; +} + int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) { if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
Add dummy bootz_setup implementation allowing the u-boot sandbox to run bootz. This recognizes both ARM and x86 zImages to validate a valid zImage was loaded. Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk> --- Changes in v2: - Move into sandbox/lib/bootm.c - convert to u-boot coding style - Remove unneeded cast arch/sandbox/lib/bootm.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)