[Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Adam Lackorzynski]

Make smpboot available not only for Linux but for all setups.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/arm_boot.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 620550b..a68b396 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -268,16 +268,17 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
         }
         rom_add_blob_fixed("bootloader", bootloader, sizeof(bootloader),
                            info->loader_start);
-        if (info->nb_cpus > 1) {
-            smpboot[10] = info->smp_priv_base;
-            for (n = 0; n < sizeof(smpboot) / 4; n++) {
-                smpboot[n] = tswap32(smpboot[n]);
-            }
-            rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
-                               info->smp_loader_start);
-        }
         info->initrd_size = initrd_size;
     }
+
+    if (info->nb_cpus > 1) {
+        smpboot[10] = info->smp_priv_base;
+        for (n = 0; n < sizeof(smpboot) / 4; n++) {
+            smpboot[n] = tswap32(smpboot[n]);
+        }
+        rom_add_blob_fixed("smpboot", smpboot, sizeof(smpboot),
+                           info->smp_loader_start);
+    }
     info->is_linux = is_linux;
     qemu_register_reset(main_cpu_reset, env);
 }
-- 
1.7.2.3


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Peter Maydell]

On 15 February 2011 10:48, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> Make smpboot available not only for Linux but for all setups.

I'm not convinced about this. I think if you're providing a raw
image for an SMP system (rather than a Linux kernel) then it's
your job to provide an image which handles the bootup of the
secondary CPUs, the same way it would be if you were providing
a ROM image for real hardware.

-- PMM

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Adam Lackorzynski]

On Tue Feb 15, 2011 at 13:01:08 +0000, Peter Maydell wrote:
> On 15 February 2011 10:48, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> > Make smpboot available not only for Linux but for all setups.
> 
> I'm not convinced about this. I think if you're providing a raw
> image for an SMP system (rather than a Linux kernel) then it's
> your job to provide an image which handles the bootup of the
> secondary CPUs, the same way it would be if you were providing
> a ROM image for real hardware.

Ok, this is one possibility. Another one would be something like this:

 Subject: [PATCH] target-arm: Provide entry vector for non-linux systems

Non-Linux systems must provide their own code for secondary CPU boot-up.
We use the same entry point as on the first CPU.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/realview.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/realview.c b/hw/realview.c
index 6eb6c6a..574bc11 100644
--- a/hw/realview.c
+++ b/hw/realview.c
@@ -112,7 +112,11 @@ static void secondary_cpu_reset(void *opaque)
   /* Set entry point for secondary CPUs.  This assumes we're using
      the init code from arm_boot.c.  Real hardware resets all CPUs
      the same.  */
-  env->regs[15] = SMP_BOOT_ADDR;
+  if (realview_binfo.is_linux) {
+      env->regs[15] = SMP_BOOT_ADDR;
+  } else {
+      env->regs[15] = realview_binfo.entry;
+  }
 }
 
 /* The following two lists must be consistent.  */
-- 
1.7.2.3


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Peter Maydell]

On 15 February 2011 13:12, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>
> On Tue Feb 15, 2011 at 13:01:08 +0000, Peter Maydell wrote:
>> On 15 February 2011 10:48, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
>> > Make smpboot available not only for Linux but for all setups.
>>
>> I'm not convinced about this. I think if you're providing a raw
>> image for an SMP system (rather than a Linux kernel) then it's
>> your job to provide an image which handles the bootup of the
>> secondary CPUs, the same way it would be if you were providing
>> a ROM image for real hardware.
>
> Ok, this is one possibility. Another one would be something like this:

> @@ -112,7 +112,11 @@ static void secondary_cpu_reset(void *opaque)
>   /* Set entry point for secondary CPUs.  This assumes we're using
>      the init code from arm_boot.c.  Real hardware resets all CPUs
>      the same.  */
> -  env->regs[15] = SMP_BOOT_ADDR;
> +  if (realview_binfo.is_linux) {
> +      env->regs[15] = SMP_BOOT_ADDR;
> +  } else {
> +      env->regs[15] = realview_binfo.entry;
> +  }
>  }

Moving in the right direction, but it would be cleaner if the secondary
CPU reset was handled inside arm_boot.c, I think (there is a TODO
in that file to that effect). Then we could get rid of the cpu reset
hook from realview.c.

-- PMM

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Adam Lackorzynski]

On Tue Feb 15, 2011 at 13:37:44 +0000, Peter Maydell wrote:
> On 15 February 2011 13:12, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> >
> > On Tue Feb 15, 2011 at 13:01:08 +0000, Peter Maydell wrote:
> >> On 15 February 2011 10:48, Adam Lackorzynski <adam@os.inf.tu-dresden.de> wrote:
> >> > Make smpboot available not only for Linux but for all setups.
> >>
> >> I'm not convinced about this. I think if you're providing a raw
> >> image for an SMP system (rather than a Linux kernel) then it's
> >> your job to provide an image which handles the bootup of the
> >> secondary CPUs, the same way it would be if you were providing
> >> a ROM image for real hardware.
> >
> > Ok, this is one possibility. Another one would be something like this:
> 
> > @@ -112,7 +112,11 @@ static void secondary_cpu_reset(void *opaque)
> >   /* Set entry point for secondary CPUs.  This assumes we're using
> >      the init code from arm_boot.c.  Real hardware resets all CPUs
> >      the same.  */
> > -  env->regs[15] = SMP_BOOT_ADDR;
> > +  if (realview_binfo.is_linux) {
> > +      env->regs[15] = SMP_BOOT_ADDR;
> > +  } else {
> > +      env->regs[15] = realview_binfo.entry;
> > +  }
> >  }
> 
> Moving in the right direction, but it would be cleaner if the secondary
> CPU reset was handled inside arm_boot.c, I think (there is a TODO
> in that file to that effect). Then we could get rid of the cpu reset
> hook from realview.c.

Like the following?

 Subject: [PATCH] target-arm: Integrate secondary CPU reset in arm_boot

Integrate secondary CPU reset into arm_boot, removing it from realview.c.
On non-Linux systems secondary CPUs start with the same entry as the boot
CPU.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/arm_boot.c |   23 +++++++++++++++--------
 hw/realview.c |   14 --------------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 620550b..41e99d1 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -175,7 +175,7 @@ static void set_kernel_args_old(struct arm_boot_info *info,
     }
 }
 
-static void main_cpu_reset(void *opaque)
+static void do_cpu_reset(void *opaque)
 {
     CPUState *env = opaque;
     struct arm_boot_info *info = env->boot_info;
@@ -187,16 +187,20 @@ static void main_cpu_reset(void *opaque)
             env->regs[15] = info->entry & 0xfffffffe;
             env->thumb = info->entry & 1;
         } else {
-            env->regs[15] = info->loader_start;
-            if (old_param) {
-                set_kernel_args_old(info, info->initrd_size,
+            if (env == first_cpu) {
+                env->regs[15] = info->loader_start;
+                if (old_param) {
+                    set_kernel_args_old(info, info->initrd_size,
+                                        info->loader_start);
+                } else {
+                    set_kernel_args(info, info->initrd_size,
                                     info->loader_start);
+                }
             } else {
-                set_kernel_args(info, info->initrd_size, info->loader_start);
+                env->regs[15] = info->smp_loader_start;
             }
         }
     }
-    /* TODO:  Reset secondary CPUs.  */
 }
 
 void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
@@ -217,7 +221,6 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
 
     if (info->nb_cpus == 0)
         info->nb_cpus = 1;
-    env->boot_info = info;
 
 #ifdef TARGET_WORDS_BIGENDIAN
     big_endian = 1;
@@ -279,5 +282,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
         info->initrd_size = initrd_size;
     }
     info->is_linux = is_linux;
-    qemu_register_reset(main_cpu_reset, env);
+
+    for (; env; env = env->next_cpu) {
+        env->boot_info = info;
+        qemu_register_reset(do_cpu_reset, env);
+    }
 }
diff --git a/hw/realview.c b/hw/realview.c
index 6eb6c6a..fae444a 100644
--- a/hw/realview.c
+++ b/hw/realview.c
@@ -104,17 +104,6 @@ static struct arm_boot_info realview_binfo = {
     .smp_loader_start = SMP_BOOT_ADDR,
 };
 
-static void secondary_cpu_reset(void *opaque)
-{
-  CPUState *env = opaque;
-
-  cpu_reset(env);
-  /* Set entry point for secondary CPUs.  This assumes we're using
-     the init code from arm_boot.c.  Real hardware resets all CPUs
-     the same.  */
-  env->regs[15] = SMP_BOOT_ADDR;
-}
-
 /* The following two lists must be consistent.  */
 enum realview_board_type {
     BOARD_EB,
@@ -176,9 +165,6 @@ static void realview_init(ram_addr_t ram_size,
         }
         irqp = arm_pic_init_cpu(env);
         cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
-        if (n > 0) {
-            qemu_register_reset(secondary_cpu_reset, env);
-        }
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
         if (is_mpcore) {
-- 
1.7.2.3


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Vincent Palatin]

Hi Adam,

>> Moving in the right direction, but it would be cleaner if the secondary
>> CPU reset was handled inside arm_boot.c, I think (there is a TODO
>> in that file to that effect). Then we could get rid of the cpu reset
>> hook from realview.c.
>
> Like the following?

This assumes that all the ARM SMP platforms are booting their
secondary CPU the same way as the emulated Realview.
For example, I'm currently writing a Tegra2 (dual A9) SoC emulation
and the second CPU is halted when the platform starts and I cannot
re-use the current smpboot firmware chunk. My current workaround is to
use "info->nb_cpus = 1" and do the init in the board code. Forcing the
reset function will probably not help.

>  Subject: [PATCH] target-arm: Integrate secondary CPU reset in arm_boot
>
> Integrate secondary CPU reset into arm_boot, removing it from realview.c.
> On non-Linux systems secondary CPUs start with the same entry as the boot
> CPU.
>
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/arm_boot.c |   23 +++++++++++++++--------
>  hw/realview.c |   14 --------------
>  2 files changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 620550b..41e99d1 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -175,7 +175,7 @@ static void set_kernel_args_old(struct arm_boot_info *info,
>     }
>  }
>
> -static void main_cpu_reset(void *opaque)
> +static void do_cpu_reset(void *opaque)
>  {
>     CPUState *env = opaque;
>     struct arm_boot_info *info = env->boot_info;
> @@ -187,16 +187,20 @@ static void main_cpu_reset(void *opaque)
>             env->regs[15] = info->entry & 0xfffffffe;
>             env->thumb = info->entry & 1;
>         } else {
> -            env->regs[15] = info->loader_start;
> -            if (old_param) {
> -                set_kernel_args_old(info, info->initrd_size,
> +            if (env == first_cpu) {
> +                env->regs[15] = info->loader_start;
> +                if (old_param) {
> +                    set_kernel_args_old(info, info->initrd_size,
> +                                        info->loader_start);
> +                } else {
> +                    set_kernel_args(info, info->initrd_size,
>                                     info->loader_start);
> +                }
>             } else {
> -                set_kernel_args(info, info->initrd_size, info->loader_start);
> +                env->regs[15] = info->smp_loader_start;
>             }
>         }
>     }
> -    /* TODO:  Reset secondary CPUs.  */
>  }
>
>  void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
> @@ -217,7 +221,6 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
>
>     if (info->nb_cpus == 0)
>         info->nb_cpus = 1;
> -    env->boot_info = info;
>
>  #ifdef TARGET_WORDS_BIGENDIAN
>     big_endian = 1;
> @@ -279,5 +282,9 @@ void arm_load_kernel(CPUState *env, struct arm_boot_info *info)
>         info->initrd_size = initrd_size;
>     }
>     info->is_linux = is_linux;
> -    qemu_register_reset(main_cpu_reset, env);
> +
> +    for (; env; env = env->next_cpu) {
> +        env->boot_info = info;
> +        qemu_register_reset(do_cpu_reset, env);
> +    }
>  }
> diff --git a/hw/realview.c b/hw/realview.c
> index 6eb6c6a..fae444a 100644
> --- a/hw/realview.c
> +++ b/hw/realview.c
> @@ -104,17 +104,6 @@ static struct arm_boot_info realview_binfo = {
>     .smp_loader_start = SMP_BOOT_ADDR,
>  };
>
> -static void secondary_cpu_reset(void *opaque)
> -{
> -  CPUState *env = opaque;
> -
> -  cpu_reset(env);
> -  /* Set entry point for secondary CPUs.  This assumes we're using
> -     the init code from arm_boot.c.  Real hardware resets all CPUs
> -     the same.  */
> -  env->regs[15] = SMP_BOOT_ADDR;
> -}
> -
>  /* The following two lists must be consistent.  */
>  enum realview_board_type {
>     BOARD_EB,
> @@ -176,9 +165,6 @@ static void realview_init(ram_addr_t ram_size,
>         }
>         irqp = arm_pic_init_cpu(env);
>         cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
> -        if (n > 0) {
> -            qemu_register_reset(secondary_cpu_reset, env);
> -        }
>     }
>     if (arm_feature(env, ARM_FEATURE_V7)) {
>         if (is_mpcore) {
> --
> 1.7.2.3


-- 
Vincent

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Adam Lackorzynski]

Hi,

On Tue Feb 15, 2011 at 10:02:05 -0500, Vincent Palatin wrote:
> >> Moving in the right direction, but it would be cleaner if the secondary
> >> CPU reset was handled inside arm_boot.c, I think (there is a TODO
> >> in that file to that effect). Then we could get rid of the cpu reset
> >> hook from realview.c.
> >
> > Like the following?
> 
> This assumes that all the ARM SMP platforms are booting their
> secondary CPU the same way as the emulated Realview.
> For example, I'm currently writing a Tegra2 (dual A9) SoC emulation
> and the second CPU is halted when the platform starts and I cannot
> re-use the current smpboot firmware chunk. My current workaround is to
> use "info->nb_cpus = 1" and do the init in the board code. Forcing the
> reset function will probably not help.

The smpboot code also halts the CPUs, i.e. they are waiting in wfi.
What would you like to have instead? Maybe it's not so different...


Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH 1/3] target-arm: Setup smpboot code in all setups

[Peter Maydell]

On 15 February 2011 15:02, Vincent Palatin
<vincent.palatin_qemu@polytechnique.org> wrote:
> This assumes that all the ARM SMP platforms are booting their
> secondary CPU the same way as the emulated Realview.
> For example, I'm currently writing a Tegra2 (dual A9) SoC emulation
> and the second CPU is halted when the platform starts and I cannot
> re-use the current smpboot firmware chunk. My current workaround is to
> use "info->nb_cpus = 1" and do the init in the board code. Forcing the
> reset function will probably not help.

My instinct here is to say "models should follow the hardware". So
if the Tegra2 SOC holds secondary cores in reset until you do something
magic to a reset controller, the model should behave the same way.
Realview boards just start all the cores at once, and that's how the
board model ought to behave.

The code in hw/arm_boot.c is really to my mind a debug convenience
for passing a bare kernel. It's a secondary thing and shouldn't drive
how we model what happens at reset.

(I'd expect that a tegra2 model would probably not use arm_boot.c;
the omap3 beagle model in meego doesn't. There's just too much
stuff that the boot ROM and the boot loader need to do to go around
emulating it all in qemu, so it's better to just pass an sd image or a
ROM image that includes the boot loader and the kernel, I think.)

-- PMM