[PATCH 0/2] Fix secondary CPU reset for Xilinx Zynq 7000

[PATCH 0/2] Fix secondary CPU reset for Xilinx Zynq 7000

[Sebastian Huber]

I recently added the support for CPU1 to the xilinx-zynq-a9 machine
(hw/arm/xilinx_zynq.c). However, the reset behaviour doesn't match exactly with
the hardware. After a system reset (SRST), the CPU1 should execute a wfe
instruction and then load the start address from 0xfffffff0:

https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM/Starting-Code-on-CPU-1

Sebastian Huber (2):
  hw/arm/boot: Use hooks if PSCI is disabled
  hw/arm/xilinx_zynq: Add CPU1 reset

 hw/arm/boot.c        | 30 +++++++++++++++++++-----------
 hw/arm/xilinx_zynq.c | 25 +++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 11 deletions(-)

-- 
2.35.3

[PATCH 1/2] hw/arm/boot: Use hooks if PSCI is disabled

[Sebastian Huber]

In arm_load_kernel(), use the secondary boot hooks provided by the
platform if PSCI is disabled also while booting a non-Linux kernel.
While booting Linux with PSCI disabled, provide default hooks if needed.

In do_cpu_reset(), use the secondary CPU reset hook provided by the
platform for resetting a non-Linux kernel.

This change allows a more accurate simulation of the platform reset
behaviour.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 hw/arm/boot.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 5301d8d318..cad7f41f46 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -720,7 +720,11 @@ static void do_cpu_reset(void *opaque)
                 g_assert_not_reached();
             }
 
-            cpu_set_pc(cs, entry);
+            if (cs == first_cpu || !info->secondary_cpu_reset_hook) {
+                cpu_set_pc(cs, entry);
+            } else {
+                info->secondary_cpu_reset_hook(cpu, info);
+            }
         } else {
             /*
              * If we are booting Linux then we might need to do so at:
@@ -1299,20 +1303,24 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
         }
     }
 
-    if (info->psci_conduit == QEMU_PSCI_CONDUIT_DISABLED &&
-        info->is_linux && nb_cpus > 1) {
+    if (info->psci_conduit == QEMU_PSCI_CONDUIT_DISABLED && nb_cpus > 1) {
         /*
-         * We're booting Linux but not using PSCI, so for SMP we need
-         * to write a custom secondary CPU boot loader stub, and arrange
-         * for the secondary CPU reset to make the accompanying initialization.
+         * We're not using PSCI, so for SMP we may need to write a custom
+         * secondary CPU boot loader stub, and arrange for the secondary CPU
+         * reset to make the accompanying initialization.
          */
-        if (!info->secondary_cpu_reset_hook) {
-            info->secondary_cpu_reset_hook = default_reset_secondary;
+        if (info->is_linux) {
+            /* For the Linux boot, use default hooks if needed */
+            if (!info->secondary_cpu_reset_hook) {
+                info->secondary_cpu_reset_hook = default_reset_secondary;
+            }
+            if (!info->write_secondary_boot) {
+                info->write_secondary_boot = default_write_secondary;
+            }
         }
-        if (!info->write_secondary_boot) {
-            info->write_secondary_boot = default_write_secondary;
+        if (info->write_secondary_boot) {
+            info->write_secondary_boot(cpu, info);
         }
-        info->write_secondary_boot(cpu, info);
     } else {
         /*
          * No secondary boot stub; don't use the reset hook that would
-- 
2.35.3

[PATCH 2/2] hw/arm/xilinx_zynq: Add CPU1 reset

[Sebastian Huber]

After a system reset (SRST), the CPU1 should execute a wfe instruction
and then load the start address from 0xfffffff0:

https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM/Starting-Code-on-CPU-1

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 hw/arm/xilinx_zynq.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 37c234f5ab..0ee4a39a28 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -64,6 +64,8 @@ static const int dma_irqs[8] = {
 
 #define BOARD_SETUP_ADDR        0x100
 
+#define SECONDARY_SETUP_ADDR    0xFFFFFFE8
+
 #define SLCR_LOCK_OFFSET        0x004
 #define SLCR_UNLOCK_OFFSET      0x008
 #define SLCR_ARM_PLL_OFFSET     0x100
@@ -112,6 +114,28 @@ static void zynq_write_board_setup(ARMCPU *cpu,
                        sizeof(board_setup_blob), BOARD_SETUP_ADDR);
 }
 
+static void zynq_secondary_cpu_reset(ARMCPU *cpu,
+                                     const struct arm_boot_info *info)
+{
+    /*
+     * After a system reset (SRST), the CPU1 should execute a wfe instruction
+     * and then load the start address from 0xfffffff0:
+     *
+     * https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM/Starting-Code-on-CPU-1
+     */
+    uint32_t secondary_setup_blob[] = {
+        0xe320f002, /* wfe */
+        0xe51ff004, /* ldr pc, [pc, #-4] */
+        SECONDARY_SETUP_ADDR
+    };
+    for (int n = 0; n < ARRAY_SIZE(secondary_setup_blob); n++) {
+        secondary_setup_blob[n] = tswap32(secondary_setup_blob[n]);
+    }
+    rom_add_blob_fixed("secondary-setup", secondary_setup_blob,
+                       sizeof(secondary_setup_blob), SECONDARY_SETUP_ADDR);
+    cpu_set_pc(CPU(cpu), SECONDARY_SETUP_ADDR);
+}
+
 static struct arm_boot_info zynq_binfo = {};
 
 static void gem_init(uint32_t base, qemu_irq irq)
@@ -378,6 +402,7 @@ static void zynq_init(MachineState *machine)
     zynq_binfo.loader_start = 0;
     zynq_binfo.board_setup_addr = BOARD_SETUP_ADDR;
     zynq_binfo.write_board_setup = zynq_write_board_setup;
+    zynq_binfo.secondary_cpu_reset_hook = zynq_secondary_cpu_reset;
 
     arm_load_kernel(zynq_machine->cpu[0], machine, &zynq_binfo);
 }
-- 
2.35.3

Re: [PATCH 1/2] hw/arm/boot: Use hooks if PSCI is disabled

[Peter Maydell]

On Mon, 23 Sept 2024 at 04:57, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> In arm_load_kernel(), use the secondary boot hooks provided by the
> platform if PSCI is disabled also while booting a non-Linux kernel.
> While booting Linux with PSCI disabled, provide default hooks if needed.
>
> In do_cpu_reset(), use the secondary CPU reset hook provided by the
> platform for resetting a non-Linux kernel.
>
> This change allows a more accurate simulation of the platform reset
> behaviour.

So, the difficulty with this is that it's effectively
introducing an extra way of booting. At the moment we
have two boot approaches for Arm guests:

(1) Booting Linux -- the boot.c code simulates what the BIOS,
boot rom etc, does, both to set up the 1st CPU for the kernel
boot entry, and to set up the secondaries in whatever way
the bootrom does that the kernel expects to release them from.

(2) Booting bare-metal -- boot.c assumes the guest code is going
to do whatever the BIOS/bootrom does, so you get what you get
for real-hardware CPU reset. (Either the secondaries start
in power-off state and the primary will release them via some
kind of power controller device, or else all the CPUs start at
once at the reset vector and the bootrom is going to sort the
secondaries out and put them in a pen.)

What you want is a third thing:

(3) Booting not-a-kernel but not 100% bare-metal: emulate what
the bootrom does for primary and secondary CPUs but don't
boot the guest binary as if it was a Linux kernel.

The problem with adding that is that we don't have any
way to distinguish whether the user wanted that or our
existing type (2), because both are "user gave us a binary
that isn't a Linux kernel". (It also has a bit of a
"continuously expanding job" problem because the bootrom
could do arbitrarily complicated things, like boot directly
from SD cards, which we have historically not wanted to
emulate within QEMU itself.)

There are other platforms where the real hardware's bootrom
has a particular "this is what a bare-metal-under-the-bootrom
startup looks like" definition, notably the raspberry pi
boards. There too we don't currently implement that, and
instead effectively tell users "pick one of the two boot
paradigms we do support"...

thanks
-- PMM

Re: [PATCH 1/2] hw/arm/boot: Use hooks if PSCI is disabled

[Sebastian Huber]

----- Am 30. Sep 2024 um 17:16 schrieb Peter Maydell peter.maydell@linaro.org:

> On Mon, 23 Sept 2024 at 04:57, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>>
>> In arm_load_kernel(), use the secondary boot hooks provided by the
>> platform if PSCI is disabled also while booting a non-Linux kernel.
>> While booting Linux with PSCI disabled, provide default hooks if needed.
>>
>> In do_cpu_reset(), use the secondary CPU reset hook provided by the
>> platform for resetting a non-Linux kernel.
>>
>> This change allows a more accurate simulation of the platform reset
>> behaviour.
> 
> So, the difficulty with this is that it's effectively
> introducing an extra way of booting. At the moment we
> have two boot approaches for Arm guests:
> 
> (1) Booting Linux -- the boot.c code simulates what the BIOS,
> boot rom etc, does, both to set up the 1st CPU for the kernel
> boot entry, and to set up the secondaries in whatever way
> the bootrom does that the kernel expects to release them from.
> 
> (2) Booting bare-metal -- boot.c assumes the guest code is going
> to do whatever the BIOS/bootrom does, so you get what you get
> for real-hardware CPU reset. (Either the secondaries start
> in power-off state and the primary will release them via some
> kind of power controller device, or else all the CPUs start at
> once at the reset vector and the bootrom is going to sort the
> secondaries out and put them in a pen.)
> 
> What you want is a third thing:
> 
> (3) Booting not-a-kernel but not 100% bare-metal: emulate what
> the bootrom does for primary and secondary CPUs but don't
> boot the guest binary as if it was a Linux kernel.
> 
> The problem with adding that is that we don't have any
> way to distinguish whether the user wanted that or our
> existing type (2), because both are "user gave us a binary
> that isn't a Linux kernel". (It also has a bit of a
> "continuously expanding job" problem because the bootrom
> could do arbitrarily complicated things, like boot directly
> from SD cards, which we have historically not wanted to
> emulate within QEMU itself.)
> 
> There are other platforms where the real hardware's bootrom
> has a particular "this is what a bare-metal-under-the-bootrom
> startup looks like" definition, notably the raspberry pi
> boards. There too we don't currently implement that, and
> instead effectively tell users "pick one of the two boot
> paradigms we do support"...

Ok, I understand your concerns. What I would like to do is running unmodified executables on Qemu so that I can test exactly the same program which would run on the real hardware. To properly initialize an SMP system, you have to do certain things in a proper order. Currently, when I start the Zynq machine it immediately executes the ELF entry on both cores. This conflicts with the normal system start sequence which assumes that initially the second core waits in an idle loop. For example, the second core is normally released after the GIC distributor is initialized.

Changing all the existing machines to use this third way is probably a bad idea, but would it be possible to make it configurable though the platform info or a command line option?

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH 1/2] hw/arm/boot: Use hooks if PSCI is disabled

[Peter Maydell]

On Fri, 4 Oct 2024 at 02:24, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>
> ----- Am 30. Sep 2024 um 17:16 schrieb Peter Maydell peter.maydell@linaro.org:
>
> > On Mon, 23 Sept 2024 at 04:57, Sebastian Huber
> > <sebastian.huber@embedded-brains.de> wrote:
> >>
> >> In arm_load_kernel(), use the secondary boot hooks provided by the
> >> platform if PSCI is disabled also while booting a non-Linux kernel.
> >> While booting Linux with PSCI disabled, provide default hooks if needed.
> >>
> >> In do_cpu_reset(), use the secondary CPU reset hook provided by the
> >> platform for resetting a non-Linux kernel.
> >>
> >> This change allows a more accurate simulation of the platform reset
> >> behaviour.
> >
> > So, the difficulty with this is that it's effectively
> > introducing an extra way of booting. At the moment we
> > have two boot approaches for Arm guests:
> >
> > (1) Booting Linux -- the boot.c code simulates what the BIOS,
> > boot rom etc, does, both to set up the 1st CPU for the kernel
> > boot entry, and to set up the secondaries in whatever way
> > the bootrom does that the kernel expects to release them from.
> >
> > (2) Booting bare-metal -- boot.c assumes the guest code is going
> > to do whatever the BIOS/bootrom does, so you get what you get
> > for real-hardware CPU reset. (Either the secondaries start
> > in power-off state and the primary will release them via some
> > kind of power controller device, or else all the CPUs start at
> > once at the reset vector and the bootrom is going to sort the
> > secondaries out and put them in a pen.)
> >
> > What you want is a third thing:
> >
> > (3) Booting not-a-kernel but not 100% bare-metal: emulate what
> > the bootrom does for primary and secondary CPUs but don't
> > boot the guest binary as if it was a Linux kernel.
> >
> > The problem with adding that is that we don't have any
> > way to distinguish whether the user wanted that or our
> > existing type (2), because both are "user gave us a binary
> > that isn't a Linux kernel". (It also has a bit of a
> > "continuously expanding job" problem because the bootrom
> > could do arbitrarily complicated things, like boot directly
> > from SD cards, which we have historically not wanted to
> > emulate within QEMU itself.)
> >
> > There are other platforms where the real hardware's bootrom
> > has a particular "this is what a bare-metal-under-the-bootrom
> > startup looks like" definition, notably the raspberry pi
> > boards. There too we don't currently implement that, and
> > instead effectively tell users "pick one of the two boot
> > paradigms we do support"...
>
> Ok, I understand your concerns. What I would like to do is running unmodified executables on Qemu so that I can test exactly the same program which would run on the real hardware. To properly initialize an SMP system, you have to do certain things in a proper order. Currently, when I start the Zynq machine it immediately executes the ELF entry on both cores. This conflicts with the normal system start sequence which assumes that initially the second core waits in an idle loop. For example, the second core is normally released after the GIC distributor is initialized.
>
> Changing all the existing machines to use this third way is probably a bad idea, but would it be possible to make it configurable though the platform info or a command line option?

Image loading is already weird and inconsistent across
architectures and across machines. I'm not really
enthusiastic about adding an extra thing that only
applies to one machine type.

You might be able to get the effect you want by writing a
guest binary that does the things the boot-rom does before
starting the main executable, and telling QEMU to load
both that "bootrom/bios" ELF file and the main ELF file.

thanks
-- PMM