diff mbox series

[2/6] x86: Move work-around out of cpu_jump_to_64bit_uboot()

Message ID 20200405172230.2.I66d0f9172cfa99acc73a670c058bb052f59a78ef@changeid
State Superseded
Headers show
Series x86: efi: Add a 64-bit coreboot payload | expand

Commit Message

Simon Glass April 5, 2020, 11:22 p.m. UTC
At present this function copies U-Boot from the last 1MB of ROM. This is
not the right way to do it. Instead, the binman symbol should provide the
location.

But in any case the code should live in the caller,
spl_board_load_image(), so that the 64-bit jump function can be used
elsewhere. Move it.

Signed-off-by: Simon Glass <sjg at chromium.org>
---

 arch/x86/cpu/i386/cpu.c | 10 ----------
 arch/x86/lib/spl.c      | 13 +++++++++++++
 2 files changed, 13 insertions(+), 10 deletions(-)

Comments

Bin Meng April 23, 2020, 9:16 a.m. UTC | #1
Hi Simon,

On Mon, Apr 6, 2020 at 7:22 AM Simon Glass <sjg at chromium.org> wrote:
>
> At present this function copies U-Boot from the last 1MB of ROM. This is
> not the right way to do it. Instead, the binman symbol should provide the
> location.
>
> But in any case the code should live in the caller,
> spl_board_load_image(), so that the 64-bit jump function can be used
> elsewhere. Move it.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>  arch/x86/cpu/i386/cpu.c | 10 ----------
>  arch/x86/lib/spl.c      | 13 +++++++++++++
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> index c8da7f10e9b..45416a9be5b 100644
> --- a/arch/x86/cpu/i386/cpu.c
> +++ b/arch/x86/cpu/i386/cpu.c
> @@ -611,16 +611,6 @@ int cpu_jump_to_64bit_uboot(ulong target)
>
>         func = (func_t)ptr;
>
> -       /*
> -        * Copy U-Boot from ROM
> -        * TODO(sjg at chromium.org): Figure out a way to get the text base
> -        * correctly here, and in the device-tree binman definition.
> -        *
> -        * Also consider using FIT so we get the correct image length and
> -        * parameters.
> -        */
> -       memcpy((char *)target, (char *)0xfff00000, 0x100000);
> -
>         /* Jump to U-Boot */
>         func((ulong)pgtable, 0, (ulong)target);
>
> diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
> index 90baec2a175..95a89c072d5 100644
> --- a/arch/x86/lib/spl.c
> +++ b/arch/x86/lib/spl.c
> @@ -207,6 +207,19 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
>         spl_image->os = IH_OS_U_BOOT;
>         spl_image->name = "U-Boot";
>
> +       if (!IS_ENABLED(CONFIG_SYS_COREBOOT)) {

For the coreboot case, where does it load the image then?

> +               /*
> +                * Copy U-Boot from ROM
> +                * TODO(sjg at chromium.org): Figure out a way to get the text base
> +                * correctly here, and in the device-tree binman definition.
> +                *
> +                * Also consider using FIT so we get the correct image length
> +                * and parameters.
> +                */
> +               memcpy((char *)spl_image->load_addr, (char *)0xfff00000,
> +                      0x100000);
> +       }
> +
>         debug("Loading to %lx\n", spl_image->load_addr);
>
>         return 0;
> --

Regards,
Bin
Simon Glass April 26, 2020, 7:45 p.m. UTC | #2
Hi Bin,

On Thu, 23 Apr 2020 at 03:17, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Apr 6, 2020 at 7:22 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > At present this function copies U-Boot from the last 1MB of ROM. This is
> > not the right way to do it. Instead, the binman symbol should provide the
> > location.
> >
> > But in any case the code should live in the caller,
> > spl_board_load_image(), so that the 64-bit jump function can be used
> > elsewhere. Move it.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  arch/x86/cpu/i386/cpu.c | 10 ----------
> >  arch/x86/lib/spl.c      | 13 +++++++++++++
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> > index c8da7f10e9b..45416a9be5b 100644
> > --- a/arch/x86/cpu/i386/cpu.c
> > +++ b/arch/x86/cpu/i386/cpu.c
> > @@ -611,16 +611,6 @@ int cpu_jump_to_64bit_uboot(ulong target)
> >
> >         func = (func_t)ptr;
> >
> > -       /*
> > -        * Copy U-Boot from ROM
> > -        * TODO(sjg at chromium.org): Figure out a way to get the text base
> > -        * correctly here, and in the device-tree binman definition.
> > -        *
> > -        * Also consider using FIT so we get the correct image length and
> > -        * parameters.
> > -        */
> > -       memcpy((char *)target, (char *)0xfff00000, 0x100000);
> > -
> >         /* Jump to U-Boot */
> >         func((ulong)pgtable, 0, (ulong)target);
> >
> > diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
> > index 90baec2a175..95a89c072d5 100644
> > --- a/arch/x86/lib/spl.c
> > +++ b/arch/x86/lib/spl.c
> > @@ -207,6 +207,19 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
> >         spl_image->os = IH_OS_U_BOOT;
> >         spl_image->name = "U-Boot";
> >
> > +       if (!IS_ENABLED(CONFIG_SYS_COREBOOT)) {
>
> For the coreboot case, where does it load the image then?

It is already loaded. The combined SPL/U-Boot image is what is loaded.
Then we boot into SPL and jump to U_Boot. This is how tegra works,
since it needs two different toolchains.
>
> > +               /*
> > +                * Copy U-Boot from ROM
> > +                * TODO(sjg at chromium.org): Figure out a way to get the text base
> > +                * correctly here, and in the device-tree binman definition.
> > +                *
> > +                * Also consider using FIT so we get the correct image length
> > +                * and parameters.
> > +                */
> > +               memcpy((char *)spl_image->load_addr, (char *)0xfff00000,
> > +                      0x100000);
> > +       }
> > +
> >         debug("Loading to %lx\n", spl_image->load_addr);
> >
> >         return 0;
> > --
>
> Regards,
> Bin

Regards,
Simon
Bin Meng April 27, 2020, 1:47 a.m. UTC | #3
Hi Simon,

On Mon, Apr 27, 2020 at 3:45 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Thu, 23 Apr 2020 at 03:17, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Mon, Apr 6, 2020 at 7:22 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > At present this function copies U-Boot from the last 1MB of ROM. This is
> > > not the right way to do it. Instead, the binman symbol should provide the
> > > location.
> > >
> > > But in any case the code should live in the caller,
> > > spl_board_load_image(), so that the 64-bit jump function can be used
> > > elsewhere. Move it.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > ---
> > >
> > >  arch/x86/cpu/i386/cpu.c | 10 ----------
> > >  arch/x86/lib/spl.c      | 13 +++++++++++++
> > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> > > index c8da7f10e9b..45416a9be5b 100644
> > > --- a/arch/x86/cpu/i386/cpu.c
> > > +++ b/arch/x86/cpu/i386/cpu.c
> > > @@ -611,16 +611,6 @@ int cpu_jump_to_64bit_uboot(ulong target)
> > >
> > >         func = (func_t)ptr;
> > >
> > > -       /*
> > > -        * Copy U-Boot from ROM
> > > -        * TODO(sjg at chromium.org): Figure out a way to get the text base
> > > -        * correctly here, and in the device-tree binman definition.
> > > -        *
> > > -        * Also consider using FIT so we get the correct image length and
> > > -        * parameters.
> > > -        */
> > > -       memcpy((char *)target, (char *)0xfff00000, 0x100000);
> > > -
> > >         /* Jump to U-Boot */
> > >         func((ulong)pgtable, 0, (ulong)target);
> > >
> > > diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
> > > index 90baec2a175..95a89c072d5 100644
> > > --- a/arch/x86/lib/spl.c
> > > +++ b/arch/x86/lib/spl.c
> > > @@ -207,6 +207,19 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
> > >         spl_image->os = IH_OS_U_BOOT;
> > >         spl_image->name = "U-Boot";
> > >
> > > +       if (!IS_ENABLED(CONFIG_SYS_COREBOOT)) {
> >
> > For the coreboot case, where does it load the image then?
>
> It is already loaded. The combined SPL/U-Boot image is what is loaded.
> Then we boot into SPL and jump to U_Boot. This is how tegra works,
> since it needs two different toolchains.

So if I understand this correctly, coreboot loads U-Boot SPL + 64-bit
U-Boot as one payload all together hence there is no need memcpy? Is
adding a coreboot config check here is to speed up the boot time to
avoid the memcpy? If adding memcpy(), does anything break?

> >
> > > +               /*
> > > +                * Copy U-Boot from ROM
> > > +                * TODO(sjg at chromium.org): Figure out a way to get the text base
> > > +                * correctly here, and in the device-tree binman definition.
> > > +                *
> > > +                * Also consider using FIT so we get the correct image length
> > > +                * and parameters.
> > > +                */
> > > +               memcpy((char *)spl_image->load_addr, (char *)0xfff00000,
> > > +                      0x100000);
> > > +       }
> > > +
> > >         debug("Loading to %lx\n", spl_image->load_addr);
> > >
> > >         return 0;
> > > --
> >

Regards,
Bin
Simon Glass April 27, 2020, 3 a.m. UTC | #4
Hi Bin,

On Sun, 26 Apr 2020 at 19:47, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Apr 27, 2020 at 3:45 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Thu, 23 Apr 2020 at 03:17, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Apr 6, 2020 at 7:22 AM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > At present this function copies U-Boot from the last 1MB of ROM. This is
> > > > not the right way to do it. Instead, the binman symbol should provide the
> > > > location.
> > > >
> > > > But in any case the code should live in the caller,
> > > > spl_board_load_image(), so that the 64-bit jump function can be used
> > > > elsewhere. Move it.
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >
> > > >  arch/x86/cpu/i386/cpu.c | 10 ----------
> > > >  arch/x86/lib/spl.c      | 13 +++++++++++++
> > > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> > > > index c8da7f10e9b..45416a9be5b 100644
> > > > --- a/arch/x86/cpu/i386/cpu.c
> > > > +++ b/arch/x86/cpu/i386/cpu.c
> > > > @@ -611,16 +611,6 @@ int cpu_jump_to_64bit_uboot(ulong target)
> > > >
> > > >         func = (func_t)ptr;
> > > >
> > > > -       /*
> > > > -        * Copy U-Boot from ROM
> > > > -        * TODO(sjg at chromium.org): Figure out a way to get the text base
> > > > -        * correctly here, and in the device-tree binman definition.
> > > > -        *
> > > > -        * Also consider using FIT so we get the correct image length and
> > > > -        * parameters.
> > > > -        */
> > > > -       memcpy((char *)target, (char *)0xfff00000, 0x100000);
> > > > -
> > > >         /* Jump to U-Boot */
> > > >         func((ulong)pgtable, 0, (ulong)target);
> > > >
> > > > diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
> > > > index 90baec2a175..95a89c072d5 100644
> > > > --- a/arch/x86/lib/spl.c
> > > > +++ b/arch/x86/lib/spl.c
> > > > @@ -207,6 +207,19 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
> > > >         spl_image->os = IH_OS_U_BOOT;
> > > >         spl_image->name = "U-Boot";
> > > >
> > > > +       if (!IS_ENABLED(CONFIG_SYS_COREBOOT)) {
> > >
> > > For the coreboot case, where does it load the image then?
> >
> > It is already loaded. The combined SPL/U-Boot image is what is loaded.
> > Then we boot into SPL and jump to U_Boot. This is how tegra works,
> > since it needs two different toolchains.
>
> So if I understand this correctly, coreboot loads U-Boot SPL + 64-bit
> U-Boot as one payload all together hence there is no need memcpy? Is
> adding a coreboot config check here is to speed up the boot time to
> avoid the memcpy? If adding memcpy(), does anything break?

In this case yes, since the U-Boot address is wrong. But I suspect it
would be possible to set it correctly so that copying is possible.

But it would be quite strange. The SPL+U-Boot is in a CBFS file and
not really under control of U-Boot. So I think trying to copy it again
would be quite fragile.

Regards,
Simon
diff mbox series

Patch

diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
index c8da7f10e9b..45416a9be5b 100644
--- a/arch/x86/cpu/i386/cpu.c
+++ b/arch/x86/cpu/i386/cpu.c
@@ -611,16 +611,6 @@  int cpu_jump_to_64bit_uboot(ulong target)
 
 	func = (func_t)ptr;
 
-	/*
-	 * Copy U-Boot from ROM
-	 * TODO(sjg at chromium.org): Figure out a way to get the text base
-	 * correctly here, and in the device-tree binman definition.
-	 *
-	 * Also consider using FIT so we get the correct image length and
-	 * parameters.
-	 */
-	memcpy((char *)target, (char *)0xfff00000, 0x100000);
-
 	/* Jump to U-Boot */
 	func((ulong)pgtable, 0, (ulong)target);
 
diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c
index 90baec2a175..95a89c072d5 100644
--- a/arch/x86/lib/spl.c
+++ b/arch/x86/lib/spl.c
@@ -207,6 +207,19 @@  static int spl_board_load_image(struct spl_image_info *spl_image,
 	spl_image->os = IH_OS_U_BOOT;
 	spl_image->name = "U-Boot";
 
+	if (!IS_ENABLED(CONFIG_SYS_COREBOOT)) {
+		/*
+		 * Copy U-Boot from ROM
+		 * TODO(sjg at chromium.org): Figure out a way to get the text base
+		 * correctly here, and in the device-tree binman definition.
+		 *
+		 * Also consider using FIT so we get the correct image length
+		 * and parameters.
+		 */
+		memcpy((char *)spl_image->load_addr, (char *)0xfff00000,
+		       0x100000);
+	}
+
 	debug("Loading to %lx\n", spl_image->load_addr);
 
 	return 0;